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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

SCUMM: GUI: Implement original GUI and menus for v4-5-6-7 games #4206

Merged
merged 67 commits into from Sep 8, 2022

Conversation

AndywinXp
Copy link
Contributor

@AndywinXp AndywinXp commented Aug 18, 2022

Hi everyone! Continuing my GUI saga (see #4033), here's my PR for v4-5-6-7 games.

These changes implement the GUI and menus for the full and demo* versions of:

  • Passport to Adventure;
  • LOOM (SCUMM v4 VGA version);
  • The Secret of Monkey Island (SCUMM v4 and v5 versions);
  • Monkey Island 2: LeChuck's Revenge;
  • Indiana Jones and the Fate of Atlantis;
  • Day of the Tentacle;
  • Sam & Max: Hit the Road;
  • Full Throttle;
  • The Dig.

*some demos might not allow for a menu to appear at all.

Here are some screenshots of the changes in action: https://imgur.com/a/R6tINDB

Other info:

  • Saving and loading games is totally interchangeable with the GMM; in order to ensure a tidy experience, the thumbnail of games saved from the original menu is obtained by taking a screengrab just before the menu is drawn. Just to clarify: this doesn't save two thumbnails like COMI, it just creates it at a different point in time.
  • Of course, text speed and volumes are also synced with the GMM.
  • There are a lot of versions for the interpreters and their menus. As a rule of thumb I tried to always implement the most recent ones for each game (e.g. this means that v6 games have volume sliders on the menu);
  • You might notice that some strings might be in english in some localized games. This is expected: when present, those strings are hardcoded in english in the executable in the first place, and not fetched from the game scripts.
  • FM-Towns games gave me a very hard time... their menu is disabled for this PR, but the GUI banners work fine. I'll do my best to properly implement those menus in the next PR.
  • Amiga versions appear to work fine, but I don't have Mac games or MI1 SEGA CD; if anyone wants to test those, it would be great!
  • The music, voice, sfx volume controls originally are mapped to keys like {, }, [, ], \ and ;. I don't need to tell you that would have gone south very fast had I implemented it this way, with all the different keyboards layouts. 馃槢 So I mapped them to SHIFT-O/SHIFT-P, SHIFT-K/SHIFT-L and SHIFT-N/SHIFT-M, while the text speed buttons remain the same old + and -.

That's all, I think.

Ciao!

@AndywinXp AndywinXp force-pushed the SCUMMv5-6-7GUI branch 2 times, most recently from cb3bebe to 89cafb1 Compare Aug 18, 2022
@athrxx
Copy link
Member

athrxx commented Aug 18, 2022

You were much faster than you said. So I think I have now messed up your dialogs code a bit :-(
I hope it isn't too bad, though

@AndywinXp
Copy link
Contributor Author

AndywinXp commented Aug 18, 2022

You were much faster than you said. So I think I have now messed up your dialogs code a bit :-( I hope it isn't too bad, though

No problem at all, I'll fix it tomorrow, thanks 馃槃

@dwatteau
Copy link
Contributor

dwatteau commented Aug 18, 2022

Wow, amazing and lovely work!! I'll test this with what I have there, including some Macintosh releases :)

I see that commit 8b2855c contains some sprintf_s -> sprintf fixes for non-C11 systems. Could this particular change be squashed into the earlier commit eea3965 which introduced them, instead? So that we won't hit a build failure on most systems if a git bisect happens there. Thanks!

@AndywinXp AndywinXp force-pushed the SCUMMv5-6-7GUI branch 2 times, most recently from 1a484c9 to 76fe16a Compare Aug 18, 2022
@AndywinXp
Copy link
Contributor Author

AndywinXp commented Aug 18, 2022

Wow, amazing and lovely work!! I'll test this with what I have there, including some Macintosh releases :)

I see that commit 8b2855c contains some sprintf_s -> sprintf fixes for non-C11 systems. Could this particular change be squashed into the earlier commit eea3965 which introduced them, instead? So that we won't hit a build failure on most systems if a git bisect happens there. Thanks!

Thanks, I'll try to do that, unfortunately my git skillset is very lacking in this very precise regard 馃

@dwatteau
Copy link
Contributor

dwatteau commented Aug 18, 2022

Thanks, I'll try to do that, unfortunately my git skillset is very lacking in this very precise regard 馃

Ah yes, Git's UX is not that great鈥 :/

I'd say, as long you don't have any unstaged files or commits:

git stash
git rebase -i eea3965~1
[go to the eea3965 commit line, change the first word in front of it to "edit", quit the rebase text editor]
[do the sprintf_s replacement again]
git add engines/scumm/gfx_gui.cpp
git commit --amend
git rebase --continue # hopefully, Git manages to integrate the change without any problem
git push --force your-upstream
git stash pop

but I don't want to break your tree if you're not used to doing this with Git, though

@dwatteau
Copy link
Contributor

dwatteau commented Aug 18, 2022

Marvellous work!

Here's what I could test and where I didn't see any issue:

  • Monkey1 CD/DOS/English
  • Monkey1 CD/DOS/French
  • Monkey1 Ultimate Talkie/English
  • Monkey2 Amiga/English
  • Monkey2 DOS/English
  • Monkey2 DOS/French
  • Monkey2 Macintosh/English
  • Monkey2 Ultimate Talkie/English
  • Indy4 CD/DOS/English
  • Indy4 CD/Macintosh/English
  • Indy4 Floppy/DOS/French
  • DOTT CD/DOS/English
  • DOTT CD/DOS/French
  • Full Throttle DOS/French
  • Full Throttle Macintosh/French
  • The Dig DOS/French
  • The Dig Demo/Macintosh/English
  • The Dig Macintosh/French

and some versions where I saw some issues:

  • Monkey2 Demo/DOS/English: SPACE shows a black bar and the menus are glitched. But that's the demo which is not supposed to be interactive. Maybe the original GUI option should just not be available for monkey2-demo?

scummvm-monkey2-demo-00000

  • Indy4 FM-TOWNS/Japanese demo: I got it to crash in getStringWidth() while pressing F8. Demo is here. Ah sorry, this is probably just because it can't find the encoding.dat file for CJK when doing a local build.

  • Sam & Max CD/DOS/English or French: if I load an older savegame where I was holding an object, it has a permanent blue background around it

scummvm-samnmax-00000

  • COMI Windows/English and French: ok from a new game, but if I load a savegame from 2 months ago, the integrated UI text becomes black

image

  • Also, if I try to call the Alt-S shortcut in order to take a screenshot within ScummVM, this unpauses the game (at least for COMI). So I can't take a screenshot of the pause menu.

@AndywinXp
Copy link
Contributor Author

AndywinXp commented Aug 18, 2022

Thanks for the thorough tests! 鉂わ笍 Glad to see that most of the games are working fine, I'll work on fixing every issue you mentioned!

@AndywinXp
Copy link
Contributor Author

AndywinXp commented Aug 18, 2022

There's a caveat though: the COMI issue is known and it comes from the fact that if you load the game from the GMM, the SCUMM engine won't start the boot script, which contains the initializations of the banner colors. This means I can't do anything outside of the script scope; this is not as bad as it looks though: just reopen the main menu or change the room and it fixes itself 馃槃

@dwatteau
Copy link
Contributor

dwatteau commented Aug 18, 2022

Happy to test nice stuff like that! Ignore my comment above about the Japanese Indy4, though; I think that's just because my local build can't find the encoding.dat file for CJK.

@athrxx
Copy link
Member

athrxx commented Aug 18, 2022

Maybe the Samnmax cursor can be fixed with the same postload processing that I used for EGA/VGA changes. But that's just an idea. I am not at home to actually check anything. So I don't even know if it glitches for the same reason...

@AndywinXp
Copy link
Contributor Author

AndywinXp commented Aug 18, 2022

Maybe the Samnmax cursor can be fixed with the same postload processing that I used for EGA/VGA changes. But that's just an idea. I am not at home to actually check anything. So I don't even know if it glitches for the same reason...

I might have a precise idea about it: I have some kind of cursor substitution in place both when the menu is opened, and when it is closed. It might be that I messed up the transparency color for this edge case.

EDIT: @dwatteau I can confirm that the FMTowns demo of Indy4 doesn't crash 馃憤 it disables GUI options from scripts, so F8 calls the ScummVM GUI, which still doesn't crash for me

@athrxx
Copy link
Member

athrxx commented Aug 18, 2022

Very nice! Seems to work well most of the time.

In DOTT and MI2, it seems to be rather easy to have an "convertMessageToString: buffer overflow!" error.
I just load a savegame, press F5 and then click on "Quit", press "N", click on "Quit", press "N"...
After 3 - 4 times it usually produces the error, but it is not always the same. Sometimes the error appears, when you click "Play" afterwards...

@athrxx
Copy link
Member

athrxx commented Aug 18, 2022

2022-08-18 (1)
Okay, that would have been too good, if that worked right out of the box...

@AndywinXp
Copy link
Contributor Author

AndywinXp commented Aug 18, 2022

Thanks for the report @athrxx !

As a matter of fact I have code in place for the CJK version of DIG. Maybe I forgot to update the GUI banners themselves? The menu should work fine though. Can you confirm that?

I'll investigate the buffer overflow issue...

@athrxx
Copy link
Member

athrxx commented Aug 18, 2022

2022-08-18
Yes, it works, it just has kind of the same layout glitches due to the font size.

@AndywinXp
Copy link
Contributor Author

AndywinXp commented Aug 18, 2022

Nice! Most of it seems to be spaced as it should (do you have a picture of the original to compare?) but I'm a bit worried as why the buttons on the right are rendered that way 馃 I'll see what I can do...

@athrxx
Copy link
Member

athrxx commented Aug 18, 2022

2022-08-18 (2)
Weird, the okay button here... I guess, I should fire up DOSBox and see how it looks... ;-)

@athrxx
Copy link
Member

athrxx commented Aug 18, 2022

2022-08-18 (4)
Looks like we're VERY close already...

@athrxx
Copy link
Member

athrxx commented Aug 18, 2022

2022-08-18 (3)

@AndywinXp
Copy link
Contributor Author

AndywinXp commented Aug 18, 2022

Great! I have an idea about the menu buttons:
is this piece of code even executing?

if ((_game.id == GID_DIG && _useCJKMode) &&
    (((byte)ctrl->label[0] >= 128 && (byte)ctrl->label[0] <= 159) || ((byte)ctrl->label[0] >= 224 && (byte)ctrl->label[0] <= 253))) {
     yOffset = 16;
}

(in ScummEngine::drawInternalGUIControl())

EDIT: also there seems to be a similar check in the getStringWidth() method which it seems we're missing but I can't figure out what it does unfortunately :(

@athrxx
Copy link
Member

athrxx commented Aug 18, 2022

No, it never triggers.
The string seems to be the non-translated form, it start with e. g. /BOOT.008/Save

@AndywinXp
Copy link
Contributor Author

AndywinXp commented Aug 18, 2022

No, it never triggers. The string seems to be the non-translated form, it start with e. g. /BOOT.008/Save

Hmm... so it seems to need the translated string, even though the disasm doesn't seem to do that...
What happens if you try to temporarily translate the label just for that check?

@athrxx
Copy link
Member

athrxx commented Aug 18, 2022

So, when I add a convertMessageToString() it looks a bit more promising, but still fails.
The condition (((byte)transBuf[0] >= 128 && (byte)transBuf[0] <= 159) || ((byte)transBuf[0] >= 224 && (byte)transBuf[0] <= 253)))
is not met. It also makes no sense for Chinese. This seems to be a check for Japanese SJIS.
Why not just use the is2ByteCharacter() method? Which will work for Japanese, Korean and Chinese...

@AndywinXp
Copy link
Contributor Author

AndywinXp commented Aug 18, 2022

So, when I add a convertMessageToString() it looks a bit more promising, but still fails. The condition (((byte)transBuf[0] >= 128 && (byte)transBuf[0] <= 159) || ((byte)transBuf[0] >= 224 && (byte)transBuf[0] <= 253))) is not met. It also makes no sense for Chinese. This seems to be a check for Japanese SJIS. Why not just use the is2ByteCharacter() method? Which will work for Japanese, Korean and Chinese...

Okay, if you're sure that it is2ByteCharacter() is the equivalent check for that one, I will use it. Please let me know if it works as intended, so I can commit it.

AndywinXp and others added 18 commits Sep 1, 2022
This replaces the fixed vertical coordinates with coordinates based on the font
height (taken from the original CJK DIG interpreter).

It also influences the appearance of the non-CJK dialog, but not necessarily for
the worse (and the one we had is again different from the one in the original
7.3.5/7.5.0 interpreters, so there really is not one definite dialog).

There is more in this commit, like:
- clippint the save names at the end of the line
- getting rid of all sprintfs and similiar cleanup to be better in line with our
coding conventions.
(from my DIG CJK menu experiments)
Instead of printing the "normal" arrow glyphs, the DIG CJK interpreter draws slightly larger arrows.
鈥ompts

These changes should make life easier for Android (and more...) users.
Make sure the wheel catches, even if the cursor is exactly between two save slots (and not actually over one of them).
Regression from recent text clipping update. Now, the bogus values must at least make a valid rect, even if they're not used.
@athrxx
Copy link
Member

athrxx commented Sep 3, 2022

The PR should now be ready.
@AndywinXp has tackled all issues that we found.
So, if there aren't any objections we're going to merge this in a couple of days.

@orgads
Copy link
Contributor

orgads commented Sep 8, 2022

Gong?

@athrxx
Copy link
Member

athrxx commented Sep 8, 2022

Okay, let's do it...

@athrxx athrxx merged commit e0a2dcc into scummvm:master Sep 8, 2022
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants