#5879 closed defect (fixed)
CRUISE: Slow / unresponsive game behaviour
| Reported by: | SF/ifo | Owned by: | digitall |
|---|---|---|---|
| Priority: | normal | Component: | Engine: Cruise |
| Version: | Keywords: | ||
| Cc: | Game: | Cruise for a Corpse |
Description
Problem: The gameplay of the ScummVM version feels too slow, especially when comparing the game to the original (DOS) version or other ScummVM / cine games. This is especially notable when moving the mouse: It seems the cursor is updated only a few times per second, resulting in a very choppy movement.
Other examples: 1. Judging from the Delphine logo in the beginning and a few animations during the game I would say the game is about 25% too slow on my PC. 2. Character animations look okay, but the original DOS version also seems a bit smoother (probably also those 25%). 3. Probably a different problem, but when clicking on a locked door, the complete game (animations, mouse cursor movement) hangs for a few seconds.
The ScummVM process takes only about 10% CPU time. If it's preferred to split the above examples into separate tickets I can do so.
Tested versions: ScummVM 1.3.1 and git snapshot from 15.10.2011 Platform: Linux (Slackware64 13.37) Version: Cruise for a Corpse from Classic Collection, German CD version CPU: Intel Core i7, 4x2,8 GHz
Ticket imported from: #3423955. Ticket imported from: bugs/5879.
Attachments (5)
Change History (29)
comment:1 by , 13 years ago
by , 13 years ago
| Attachment: | cruise-0000.mpeg added |
|---|
Slow mouse movement and freeze when opening the door example
comment:2 by , 13 years ago
I've attached a savegame of Raoul standing in front of a locked door - when you try to open it, the game freezes for about two seconds. I've also attached a short video to demonstrate what I mean with choppy mouse movement: The mouse cursor jumps around instead of moving fluidly. The video also demonstrates the freeze when trying to open the door. (The video framerate is very low due to SF's size limit, but it still perfectly represents the behavior on my system.)
I've also done a comparison of the time needed to execute the logo (counting from when the first character appears until "Delphine Software Cinematique" is complete): PC (Win98, 1,3 GHz): 9s DOSBox (300 cycles): 17s ScummVM: 35s 9s are probably too short, but 35s just feel way to long for a simple logo animation, I can't imagine it was intended that way...
comment:3 by , 13 years ago
While playing around with GAME_FRAME_DELAY_* I noticed the possibility to switch the game speed during the game with the + and - keys on the keypad. The more I increase the mouse speed, the more responsive the mouse - but of course the game also gets too fast then. That leads me to think that maybe the mouse and the game cycles should be independent of each other.
However increasing the speed doesn't fix the problem with the delay after some actions - I've just discovered that the delay does not only occur when trying to open a locked door, it's just most visible there. Another example where this happens is when leaving a room: In the original version the mouse cursor can still be moved, while in the ScummVM implementation the cursor just hangs.
comment:4 by , 13 years ago
Ifo: Thank you for your prompt response to this.
I do concur that the game script loop seems to block mouse movement/updates which does differ from the original.
However, Cruise does not really have an active development team as it is considered complete. There has been some attempts to refactor this engine recently, but these have not yet been completed: http://wiki.scummvm.org/index.php/OpenTasks/Engine/Objectify_CruisE https://github.com/enginmanap/scummvm/tree/gsoc2011-objectify_cruise
The current code is not amenable to large scale changes without causing behaviour regressions such as this, and I wouldn't like to fix this only to break the game more severely.
But since you seem to be technically capable, if you want to provide a patch to try to fix this or fork ScummVM on github, then look at this and issue a pull request, the team will review and accept a fix for this.
Otherwise, we will look at this, but it may take a while...
by , 13 years ago
| Attachment: | cine_fix_unresponsive_mouse.patch added |
|---|
comment:5 by , 13 years ago
Thanks a lot for your responses!
The following trivial patch may fix the problem with sloppy mouse movement. As the mouse cursor is controlled using ScummVM core functionality ScummVM has to get a chance to update the display for the new mouse position. This patch adds a call to updateScreen() to the speed limititation loop, ensuring that the current mouse position is updated.
This patch doesn't change the delay after some of the actions.
comment:6 by , 13 years ago
Tested patch and seems fine. Applied in fca8ac60ded82ed6c34c0260199d4ac572a85acb. Thanks for this.
I now see what you mean about the "pause" after the locked door. The message displayed causes the game loop to freeze which causes the sea/boat movement in the background to freeze.. Will need to look at this as well.
comment:7 by , 13 years ago
The following patch should also fix the problem with blocking when the userDelay was triggered by an opcode.
What the patch does: - Incorporate the previous patch - Move the mouse cursor update to the frame speed limiter loop so that the user always has the correct cursor icon. - invert the loop logic, as the code has to be executed also in fastMode - try to suppress further mouse clicks when userDelay is active (there is still a race condition: When clicking on another hotspot very quickly, the menu for the item will still appear. However this bug is also present in the original game ;-)) - don't break when a userDelay was triggered, thus continuing animation and mouse movement ability; though the mouse cursor changes you just can't do anything any more, like in the original game.
by , 13 years ago
| Attachment: | cruise_dont_block_during_userwait.diff added |
|---|
comment:8 by , 13 years ago
ifo: Thanks for this second patch. However, this is a far larger change than the previous patch and we are currently in feature freeze for v1.4.0, so I'm reluctant to apply this and possibly cause a serious regression in Cruise For A Corpse.
Once v1.4.0 branches and master is unfrozen, I will apply this patch and then do full play test of Cruise to ensure that it is still completable. However, since you seem to interested in working on improving the Cruise engine, I strongly urge you to fork our repo on Github and then issue a pull request on any improvements. If you want to talk with the development team, I would suggest our IRC channel #scummvm on freenode.
comment:9 by , 13 years ago
ifo: Can you explain why you removed the screen update when DisplayOn is false in your first patch please?
See wjp's query here: https://github.com/scummvm/scummvm/commit/fca8ac60ded82ed6c34c0260199d4ac572a85acb
I am going to revert this part of the patch for now as I think this may cause a regression.
comment:10 by , 13 years ago
Restored screen update when DisplayOn is false in 3b71a3a7fbfc5a1c7d039610b4fc73476651c24e.
comment:11 by , 13 years ago
updateScreen was removed from the end of the main loop as it is now called shortly after at the beginning of the new loop cycle. As far as I can see nothing will interrupt the loop in the meantime. But you are right, it isn't called now any more when fastMode is set and displayOn is false (though fastMode seems to be for debugging only). As this indeed could have caused a regression, I've fixed this in the second patch already. However I agree that that patch is too risky to add it for 1.4, so the current solution to re-add display update at the end of the loop is perfectly fine.
comment:12 by , 13 years ago
| Owner: | set to |
|---|
comment:14 by , 13 years ago
thbluegr: Sorry, mea culpa. This is on my TODO list, delayed until after the v1.4.0 release and got buried: "Once v1.4.0 branches and master is unfrozen, I will apply this patch and then do full play test of Cruise to ensure that it is still completable."
The patches on this bug should be reviewed and the relevant portions applied, then I can do a full playtest...
comment:15 by , 13 years ago
Attaching updated patch which applies cleanly to the latest Git master HEAD. If you compare this with the previous patch, apart from line numbering differences, have removed the updateScreen call from the start of the main do loop, but the patch does a call just after the first subroutine block so this should be fine, and restored the final updateScreen if in fastMode block...
Still very unsure about this patch as it is a fairly complex change and I would prefer to have a good description/narrative from the submitter of exactly why the various blocks, continue etc. have been added.. It would be also a good idea to check against the original interepreter code if possible.
by , 13 years ago
| Attachment: | cruise_dont_block_during_userwait_updated.patch added |
|---|
Updated Patch For Git Master 2012-02-17
comment:16 by , 13 years ago
The second patch isn't as bad as it seems on the first sight: It does (almost) not add anything, it (almost) only rearranges the existing parts. I'll try to comment the changes below in more detail:
Summary: - Update the mouse cursor icon more often - Draw background animations during user interaction
The biggest change is moving the mouse handling to the upper loop (I'll call it idle loop as it's original intention was to delay the game to avoid running too fast). This is necessary as now that the cursor position is updated in this loop the icon also has to be updated, which is currently done _after_ the idle loop. Hotspots passed during the idle loop do _not_ give any user feedback without this patch.
As the mouse handling has to be done for all of the if statements the inner do-while-loop is now outside of the if statements to avoid code duplication. The if statements were adjusted accordingly. Note: The logic is as it was before, only the mouse handling is called more often now. (This rearrangement also fixes the missing call to updateScreen() from the first patch.)
Last but not least the addition of the "if (userDelay) {" and the removal of continue in the "if (userDelay && !userWait) {" statement will continue drawing the background during a userDelay - to reproduce this go to the upper deck of the ship and try to open one of the locked doors. The background will continue to be drawn and not freeze like in the current implementation. As previously mentioned you may make it to open the menu now if you click fast enough on another hotspot after trying to open the door. This bug (caused by the removal of the continue statement) is also present in the original game so I decided to ignore it. To prevent the user from opening the menu mouse events have to be suppressed.
I don't have access to the original interpreter code, but as far as I can tell the behavior matches the original's behavior now. Apart from being closer to the original the patch also gives better feedback to the user.
I hope that makes the motivation of the patch more clear...
comment:17 by , 13 years ago
ifo: Thank you for the explanation... I'll try to sit down this evening, check this over, then will commit and playtest...
comment:18 by , 13 years ago
Of course I'll also join the playtest (trying a cold playthrough ;-)). Are there any other major changes planned for the engine? If so I'd wait until those have been merged...
comment:20 by , 13 years ago
Sorry... Still haven't got round to looking at this... Will give it some work soon.
comment:21 by , 13 years ago
Committed patch to Git master in 3 parts (to aid any future bisection) ending in a5745434d30f0039db0f5ca7b5ff6781b777d819
Will do a playtest to confirm that this has not broken completion.
comment:22 by , 13 years ago
| Resolution: | → fixed |
|---|---|
| Status: | new → closed |
comment:23 by , 13 years ago
Thanks a lot for committing the patches!
One note about the comment and the commit message of the third patch (a5745434d30f0039db0f5ca7b5ff6781b777d819) however: I meant to say that the line "currentMouseButton = 0;" is required to suppress the menus, but it does _not_ prevent the bug from happening - instead it has the same problem as the original engine: The user _will_ make it to open a hotspot menu if he is clicking fast enough. If I remember correctly this is due to the engine calling several other functions before "Op_UserDelay" is finally executed, so it's basically a flaw of the original engine design...
ifo: Firstly, can you attach a savegame and replication instructions for the locked door issue as I'm not sure where you mean in the game.
I have done some basic checks comparing our current development ScummVM Git Master build against the original interpreter under DOSBox (You need to force fixed cycles or the game may run far too quickly). The time to complete the initial logo animation is identical (to within a small amount), but I suspect the issue you are describing is not that the game is running 25% slow on ScummVM compared to the original interpreter, but that the _framerate_ i.e. screen/mouse refresh is 25% slower on ScummVM compared to the original interpreter.
However, the ScummVM version of the engine does have a loop to hold a defined frame rate (See _gameSpeed, GAME_FRAME_DELAY_1 and GAME_FRAME_DELAY_2 in the code) so that the engine does not run at a speed defined by the host hardware i.e. absurdly fast with modern machines, and if the original interpreter lacked this, it may have run at a faster rate on your original hardware (DOSBox also has a cycle limiter which ensures sane behaviour by emulating the execution rate of the older machines).
Under DOSBox, I am having some trouble with the mouse driver to run the original interpreter, so could you try this and confirm if you observe a significant i.e. 25% variance in frame rate update of the mouse.
I checked the relevant ScummVM Cruise engine code including adding debug output to the mouse.cpp to see if the mouse pointer was flickering due to repeated updates to CursorMan, but this is not the case.
To deal with this, I suspect we will need to be able to observe a behaviour difference between DOSBox with original interpreter and ScummVM, so any help you can give to do this...