[Scummvm-tracker] [ScummVM :: Bugs] #15403: SHERLOCK: Mouse clicks may be ignored

[ScummVM :: Bugs]

#15403: SHERLOCK: Mouse clicks may be ignored
-------------------------+-------------------------------------------------
Reporter:  antoniou79    |      Owner:  (none)
    Type:  defect        |     Status:  new
Priority:  normal        |  Component:  Engine: Sherlock
 Version:                |   Keywords:  Rose Tattoo, Serrated Scalpel,
    Game:                |  unresponsive, interaction, user interface
-------------------------+-------------------------------------------------
 Tested on Windows 10 x64, and Android 12 (smartphone) with ScummVM 2.8.1
 (2.8.1.1 on Android) and also ScummVM 2.9.0git from recent master HEAD.

 This is an issue with both "The Lost Files of Sherlock Holmes" games: "The
 Case of the Serrated Scalpel" and "The Case of the Rose Tattoo". It can
 occasionally be observed in the demo we have on the ScummVM website for
 the Serrated Scalpel.

 This does not happen consistently but it happens frequent enough to be
 annoying (I've only tested with early parts of the games, maybe later on
 it becomes more of an issue, since these games feature mini games and
 various user interface modes).

 On Android this tends to happen more frequently (maybe due to the special
 way we have in place to differentiate taps from long touches or double
 taps etc).

 How to reproduce:
 Start a new game, reach proper gameplay (after the intro cutscenes) and
 start interacting with the interface and the screen hotspots. *Sometimes*
 the game will ignore the users clicks even though user has control (ie.
 it's not during a cutscene or during a script that disabled user
 controls).

 From a look through the engine code, I think I've spotted the culprit but
 fixing it will require someone very familiar with the engine -- hopefully
 not a complete overhaul of the input handling system.

 The culprit seems to *mainly* be this method's "Events::delay()"
 implementation:
 https://github.com/scummvm/scummvm/blob/d0336f568e3101dc480363451a76257dc2d3d4ce/engines/sherlock/events.cpp#L333
 The delay method implements a blocking delay. It distinguishes between a
 short delay and a longer delay, but in both cases it calls the method
 Events::pollEvents(). For the longer delays (10 ms or more), it breaks the
 delay in 10ms segments and iterates them while looping for the total
 duration with each iteration calling pollEvents().

 This method Events::delay() seems to be called in numerous places in the
 engine code, either directly or via the "Events::wait(int numFrames)"
 method.

 The method Events::pollEvents():
 - Handles screen updates via checkForNextFrameCounter() (this is also an
 issue when attempting to fix the situation)
 - Consumes the next pending event
 - Updates the mouse position

 Calling pollEvents() frequently results in smooth mouse movement (that the
 original games *do not have*). But since in this case pollEvents is called
 while in a blocking wait, it can consume click events without the engine
 ever properly handling them. (The keyboard events are pushed in a special
 queue (_pendingKeys) so presumably those are properly handled eventually;
 I am only focusing on missing mouse clicks in this report).

 During my debugging the repeated calls to pollEvents during executing a
 blocking "wait" would consume the mouse down and up events before the
 blocking wait would conclude, thus the rest of the code remained
 unsuspecting that a click event occured.

 Since for the early scenes of the games the events.wait()
 ScalpelScene::doBgAnim() and TattooScene::doBgAnim() seemed to be mainly
 "eating up" the mouse clicks, I've attempted a partial fix by replacing
 those waits with a similar method that would not call pollEvents(). But,
 this also required manually placing pollEvents() in other parts of the
 engine code because it expected pollEvents() to have been run after a
 doBgAnim() call, and doing a checkForNextFrameCounter() during the wait to
 maintain how the engine updated the screen. And as an expected side-
 effect, the mouse movement also became sluggish. However, this did make
 the engine not miss those clicks (tested in Windows and Android).

 This would not be a complete fix, since there's other places where wait()
 and delay() are used and would need to be fixed. (For example in "Serrated
 Scalpel" this would need to be done at least for the Journal, Setup Menu,
 maybe the map, darts game(?) and perhaps elsewhere). I suspect that direct
 calls for delay() won't need "fixing" because practically the issue comes
 from the longer delays that are cause by wait() (since wait has a number
 of frames arguments).
-- 
Ticket URL: <https://bugs.scummvm.org/ticket/15403>
ScummVM :: Bugs <https://bugs.scummvm.org>
ScummVM