Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SCI32: fix bug #9752 (for all games) #2053

Merged
merged 1 commit into from Feb 12, 2020
Merged

SCI32: fix bug #9752 (for all games) #2053

merged 1 commit into from Feb 12, 2020

Conversation

@ZvikaZ
Copy link
Contributor

ZvikaZ commented Feb 11, 2020

Some SCI32 games have an issue that the saved games thumbnails is useless because it's hidden
by the configuration panel (QFG4) or by the keyboard (Shivers).
Maybe this problems appears on other SCI32 games as well, should be checked.

We avoid that by keeping the thumbnail everytime the menu bar is hidden,
and when time comes to save the game, we use that thumbnail instead of taking
a new one (which would be with the control panel/keyboard).

This fix is for QfG4 only.

@ZvikaZ ZvikaZ force-pushed the ZvikaZ:z_thumb branch from d12ab1d to 9cf7c2b Feb 11, 2020
@tsoliman

This comment has been minimized.

Copy link
Member

tsoliman commented Feb 11, 2020

Currently the bug doesn't exist if you save by pressing F5 to save since that doesn't bring up the control panel.

How does this behave if I press F5 to save the game or using the GMM or some other way that doesn't involve the menu bar hide action?

I'm guessing a stale image will be used from when the menu bar was last hidden. That would be actually worse IMO as it is silently broken.

If that's the case, then a better solution might be a script patch for the save game handing script to hide the control panel just before saving (since SCI32 saving in ScummVM seem to be script driven and not like SCI16 which explicitly hides the menu before saving)

@tsoliman

This comment has been minimized.

Copy link
Member

tsoliman commented Feb 11, 2020

I've actually verified that this approach introduces a new bug when saving with F5.

To reproduce:

  • Move the mouse to show the menubar
  • Leave the room
  • Press F5 to save
  • Check out the thumbnail and find that it is of the old room

The fact that the thumbnail is silently wrong makes this new bug much worse IMO (the old thumbnail was of the correct viewport state, just with a dialog on it, whereas this one is of the last time you happened to show the menubar)

@tsoliman tsoliman requested a review from sluicebox Feb 11, 2020
@ZvikaZ

This comment has been minimized.

Copy link
Contributor Author

ZvikaZ commented Feb 11, 2020

Nice catch @tsoliman , thanks for noticing this bug...
Investigating your idea now.

@ZvikaZ ZvikaZ force-pushed the ZvikaZ:z_thumb branch from 9cf7c2b to e41f0a6 Feb 11, 2020
@ZvikaZ

This comment has been minimized.

Copy link
Contributor Author

ZvikaZ commented Feb 11, 2020

@tsoliman , I've implemented your idea, thanks. It's simpler and better, and we also avoid adding a new kernel command.

@tsoliman

This comment has been minimized.

Copy link
Member

tsoliman commented Feb 12, 2020

This seems to work in the one version of QFG4 I tested with (the GOG version)

I'm looking forward to feedback on the script code by an SCI dev.

Good work! I'm excited that this is finally getting fixed.

@tsoliman

This comment has been minimized.

Copy link
Member

tsoliman commented Feb 12, 2020

I've tested this with both the GOG version and the Anthology CD version. Both of these are CD versions.

It's likely that this script patch will work on the standalone CD version.
It's unlikely that this script patch will work on the floppy version.

It might be safer to mark this as a QFG4CD patch rather than a generic QFG4 patch unless you've tested the floppy.

@ZvikaZ ZvikaZ force-pushed the ZvikaZ:z_thumb branch 2 times, most recently from fcf3800 to 6878c33 Feb 12, 2020
@ZvikaZ

This comment has been minimized.

Copy link
Contributor Author

ZvikaZ commented Feb 12, 2020

You were correct, it indeed didn't work on floppy version.
But now I've added support for floppy as well ;-)

@ZvikaZ

This comment has been minimized.

Copy link
Contributor Author

ZvikaZ commented Feb 12, 2020

If it's possible to consolidate the CD and Floppy patches to one patch with some magic, I'd be happy if someone will explain how to.

@ZvikaZ

This comment has been minimized.

Copy link
Contributor Author

ZvikaZ commented Feb 12, 2020

I think I have a better idea, let's put it on hold.

@ZvikaZ ZvikaZ force-pushed the ZvikaZ:z_thumb branch from 6878c33 to 5ac1d34 Feb 12, 2020
@ZvikaZ ZvikaZ changed the title SCI32: fix bug #9752, for QFG4 only SCI32: fix bug #9752 (for all games) Feb 12, 2020
@ZvikaZ

This comment has been minimized.

Copy link
Contributor Author

ZvikaZ commented Feb 12, 2020

I'm getting concise from commit to commit...
Now this PR contains only one additional line:
kFrameOut at the beginning of kSaveGame32.

This solves the problem for QFG4 CD/Floppy and Shivers, and probably for other games, if this bug exists for other games (as @csnover suspected at bug #9752 )

I could have put that line under if and make it run only for QFG4 and Shivers; however, I don't think that it makes any harm, and it makes sure that we won't encounter this problem in any other game.

However, I would like some SCI expert (@sluicebox ?) to make sure that it's OK.

@bluegr

This comment has been minimized.

Copy link
Member

bluegr commented Feb 12, 2020

Excellent work! I've tried this with QFG4, Shivers and SQ6, among others, and they all work correctly. The fix is quite straightforward, and the issue is resolved. Thanks!

Merging

@bluegr bluegr merged commit 190e47b into scummvm:master Feb 12, 2020
2 checks passed
2 checks passed
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ZvikaZ

This comment has been minimized.

Copy link
Contributor Author

ZvikaZ commented Feb 12, 2020

Thanks. But you had a typo in the commit header, in the bug number fixed by this...

@sluicebox

This comment has been minimized.

Copy link
Member

sluicebox commented Feb 16, 2020

Sorry I'm so late the party! Everything happens while on vacation...

@ZvikaZ, I am amazed that you fixed this with a single call to FrameOut, I know others have looked at this and I was pessimistic it could be resolved at all. Great job!

@ZvikaZ ZvikaZ deleted the ZvikaZ:z_thumb branch Feb 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.