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

SCUMM: Fix the navigator head text color in MI1 CD (WORKAROUND) #4271

Merged

Conversation

dwatteau
Copy link
Contributor

@dwatteau dwatteau commented Sep 15, 2022

Once more, the v5 release of Monkey Island 1 (likely) didn't use the intended color in some places. This is similar to what was found in PR #4266 or #4168. The root cause appears to be identical: the palette was reordered between the v4 and v5 VGA releases, but some color parameters weren't updated (except in the official SegaCD release, and in the fan-made Ultimate Talkie Edition).

Description

When the navigator head speaks, the v4 VGA floppy release properly kept its text brown, but the v5 VGA release forgot to adjust the Color(6) parameters for the v5 palette changes, meaning that the text was in dark purple.

Here's the behavior in the VGA floppy release:
https://www.youtube.com/watch?v=UAvuZsflglw&t=12326s

and here's what happens in the VGA CD release:
https://www.youtube.com/watch?v=u8JGiyPuZ6A&t=12080s

This PR just puts the brown color back again, since that's what used in every other version.

*(The dialogue options may look a bit wrong, too, but we have no consensus on what could have been the original intent for it — see below. So I'm just fixing the navigator head subtitle color.)

Test

  1. Start a v5 release of Monkey Island 1 (different from the SegaCD and Ultimate Talkie Edition, since they already fix this)
  2. Run it with boot param 999 (boot params 1212, 4313 and 6342 can also be used, but don't allow as many interactions)
  3. Try using the necklace in your inventory: look at the color of the navigator head's reaction
  4. Talk to the navigator head: look at the color of his subtitles

The navigator's lines should be brown (as in every other release), and not dark purple anymore.

Tested with the CD/DOS/English and CD/DOS/French releases. Macintosh and FM-TOWNS tests very welcome!

@AndywinXp
Copy link
Contributor

AndywinXp commented Sep 15, 2022

I'm not at home right now but IIRC the gui takes its colors from the same "array". Could you check that nothing is broken on that end?

@dwatteau
Copy link
Contributor Author

dwatteau commented Sep 15, 2022

I'm not at home right now but IIRC the gui takes its colors from the same "array". Could you check that nothing is broken on that end?

Do you mean before or after my patch? Is that array the vs->color one or the _string[textSlot].color one? Is this GUI the one that's been restored recently (= original pause bar and so on)?

Thanks

@dwatteau dwatteau force-pushed the fix/scumm-monkey1-v5-navigator-head-text-color branch from 6bafe83 to 5a0d090 Compare Sep 15, 2022
@AndywinXp
Copy link
Contributor

AndywinXp commented Sep 15, 2022

I'm not at home right now but IIRC the gui takes its colors from the same "array". Could you check that nothing is broken on that end?

Do you mean before or after my patch? Is that array the vs->color one or the _string[textSlot].color one? Is this GUI the one that's been restored recently (= original pause bar and so on)?

Thanks

Sorry, I mean the recently added original gui, does it change after your patch? The idea is to keep the gui as faithful to the original exe as possible even if the original intepreter changes its colors by mistake/limitation constraints

@dwatteau
Copy link
Contributor Author

dwatteau commented Sep 15, 2022

Sorry, I mean the recently added original gui, does it change after your patch? The idea is to keep the gui as faithful to the original exe as possible even if the original intepreter changes its colors by mistake/limitation constraints

OK thanks! (I just wanted to be sure we were speaking of the same thing). I'm not seeing any GUI regression when using original_gui=true during this workaround, no.

@AndywinXp
Copy link
Contributor

AndywinXp commented Sep 15, 2022

Nice, thanks! 😄

@eriktorbjorn
Copy link
Member

eriktorbjorn commented Sep 15, 2022

The patch works with my Mac version, so that's good.

I have to wonder, though... Since the CD version makes all conversation options dark purple, isn't it a bit inconsistent to turn them green here? (Even if I think the CD version is a bit harder to read.)

VGA floppy example:

scummvm-monkey-vga-00007

VGA CD example:

scummvm-monkey-00016

@AndywinXp
Copy link
Contributor

AndywinXp commented Sep 15, 2022

Hmm... I agree, the dark purple dialog choices seem deliberate artistic choices (both v5 Monkey Islands use this palette), and I frankly wouldn't change those...

There's a certain limit to which the CD release should resemble the Floppy release IMHO (even if the changes are regarded as enhancements), and over that limit one could just play the floppy version

@dwatteau
Copy link
Contributor Author

dwatteau commented Sep 15, 2022

Yeah, I'm undecided on this. Talking to the navigator head (and also to Elaine just before the end credits) triggers a different, bigger font:

scummvm-monkey-00000

(here with my workaround).

The Ultimate Talkie Edition matches the usual CD text color:

scummvm-monkey-ultimate-00000

But note that the original CD version picked… white for that same font, in the ending:

scummvm-monkey-00001

So there's no consistent VGA color for that particular font. So I don't know what was the original intent for it. I personally think that, for this font, the purple one is the hardest one to read, but yeah, that's a personal preference.

Now, if most people prefer the purple font for consistency, I'm fine with this, especially since that's what the Ultimate Talkie Edition chose to do, too.

@dwatteau dwatteau force-pushed the fix/scumm-monkey1-v5-navigator-head-text-color branch from 5a0d090 to 08cb745 Compare Sep 15, 2022
@dwatteau dwatteau changed the title SCUMM: Fix the navigator head text color in MI1 CD (WORKAROUND) SCUMM: Fix text color for the navigator head and the last scene in MI1 CD (WORKAROUND) Sep 15, 2022
@dwatteau
Copy link
Contributor Author

dwatteau commented Sep 15, 2022

Well, the purple text remains legible since it's highlighted in pink when you move the mouse over it. So I've changed this PR to use the same consistent colors as the Ultimate Talkie Edition.

@AndywinXp
Copy link
Contributor

AndywinXp commented Sep 15, 2022

Again I'm in doubt about this particular one :( The white dialog text in the final scene might be a design choice, since it fits with the rest of the palette, just like purple fits the navigator's head background. And in general I'm starting to think that changes which make a version too similar to another one are beginning to clash with changes which, in turn, fix very evident oversights or bugs.

Maybe we should start thinking about some categories of enhancements (we could have, restoration enhancements, easier-to-read text enhancements, original bugs enhancements...), since placing them under a single umbrella variable is starting to become a mixture of additions and changes which might be a tad too different from each other. What do you think?

I mean, I'm willing to help relabeling every single change if you think this can work.

@dwatteau
Copy link
Contributor Author

dwatteau commented Sep 15, 2022

Well, personal preferences are a can of worms and that's what I'm usually trying to avoid. I like finding issues where the bug and original intent are (nearly) unquestionable.

For example, for the brown text of the navigator head:

  • it's brown in the EGA version
  • it's brown in the VGA floppy version
  • brown follows the unwritten rule of subtitles very frequently matching the main distinct color of each character
  • but the text becomes dark purple in the v5 CD version (known to have several color errors)
  • and it's brown again in the v5 SegaCD version.

The v4 VGA release did this:

(14)   print(255,[Color(6),Pos(Var[140],Var[141]),Text("Hello.")]);
                  ^^^^^^^^

The v5 VGA release did this:

(14)   print(255,[Color(6),Pos(Var[141],Var[142]),Text("Hello.")]);
                  ^^^^^^^^

i.e. the palette changed in v5, but they just forgot to fix the Color(6) value for it. So I'm nearly sure that it's a small oversight and the brown text fix is right (but of course, it's still just an enhancement) and then it's easy.

Fixing glitches, continuity errors and dead-ends is also uncontroversial, so I like submitting that.

But then in this PR, there are the dialogue options. It looks like something's maybe wrong, but they are inconsistent in the v5 releases (purple, green or white), so nobody really knows the real original intent. I'm really less interested in that second type of enhancements, because if it's just a matter of personal preferences, then (IMO) it just becomes fan modding and I don't think that should be part of the main ScummVM code. It's endless.

As for having different categories of enhancements, I thought a bit about it several times, but I fear that it's going to be a maintenance and test burden. Now I tend to think that the single enhancement ON/OFF option is maybe the best way to deal with this, because it forces us to submit uncontroversial enhancements. I think that this is actually a nice safeguard against any "abuse" of very personal preferences (and that's why I almost always submit PRs for SCUMM enhancements).

So, if we think that the brown text issue is uncontroversial, while the dialogue options lead to different personal perceptions, then I'd rather limit my PR to the first part only. I'm really striving for zero-controversy enhancements 😁

@AndywinXp
Copy link
Contributor

AndywinXp commented Sep 15, 2022

Well yes, you do have a good point: forcing the enhancements to be a single toggle forces us to work "for the cause" instead of being driven by personal preferences, I agree with that 🙂

Given that, I personally do find the brown text change to be uncontroversial, while my opinion on the dialogue options would be to leave them as-is since they seem to be more akin to be artistic choices

When the navigator head speaks, the v5 release forgot to adjust the
`Color(6)` parameter for the v5 palette changes, meaning that the text
wasn't displayed with the intended brown color, as in all other versions
(this was fixed in the v5 SegaCD release, though).
@dwatteau dwatteau force-pushed the fix/scumm-monkey1-v5-navigator-head-text-color branch from 08cb745 to a063806 Compare Sep 15, 2022
@dwatteau dwatteau changed the title SCUMM: Fix text color for the navigator head and the last scene in MI1 CD (WORKAROUND) SCUMM: Fix the navigator head text color in MI1 CD (WORKAROUND) Sep 15, 2022
@dwatteau
Copy link
Contributor Author

dwatteau commented Sep 15, 2022

Thank you very much for your input; I'm really looking to please every fan of these games as much as I can! 😊

So I've edited my PR again in order to only focus on the brown text part with the navigator head.

@athrxx
Copy link
Member

athrxx commented Sep 18, 2022

Again I'm in doubt about this particular one :( The white dialog text in the final scene might be a design choice, since it fits with the rest of the palette, just like purple fits the navigator's head background. And in general I'm starting to think that changes which make a version too similar to another one are beginning to clash with changes which, in turn, fix very evident oversights or bugs.

Maybe we should start thinking about some categories of enhancements (we could have, restoration enhancements, easier-to-read text enhancements, original bugs enhancements...), since placing them under a single umbrella variable is starting to become a mixture of additions and changes which might be a tad too different from each other. What do you think?

I mean, I'm willing to help relabeling every single change if you think this can work.

I believe sub categories for (objective and subjective) enhancements would be very helpful. We have started to serve way too many different types of patches under the label "enhancement". Sometimes we fix real bugs, in other cases we just streamline versions to look more alike or to better match someone's personal taste. Maybe having categories like "Fix original gameplay bugs", "Enable hidden content", "Fix obvious original graphics bugs", "Minor visual improvements" or similar would help. Then we could have these recoloring patches (maybe also the typo fixes, or we could have another category for these) under the last category and people could still have the more non-controversial fixes without that.

@eriktorbjorn
Copy link
Member

eriktorbjorn commented Sep 21, 2022

I have no problem with different categories of enhancements. In fact, in my first attempt there was one checkbox for each enhancement because I imagine there wouldn't be a lot of them. (How wrong I was!)

Of course, someone has to go through and decide which category the current ones belong in.

And should there be a feature flag for each category? Whatever we decide on, don't forget that the Loom settings have to be adjusted too, since the enhancements checkbox isn't added automatically there.

@athrxx
Copy link
Member

athrxx commented Sep 21, 2022

Of course, someone has to go through and decide which category the current ones belong in.

I would do it like this: If you can see the bug without comparing different versions or analyzing scripts, I would label it an "obvious original graphics bugs", while the things that come from comparison and analysis could be called "minor improvement". The typos are kind of special, since they're at the same time obvious and unimportant, so maybe an extra category.

And should there be a feature flag for each category? Whatever we decide on, don't forget that the Loom settings have to be adjusted too, since the enhancements checkbox isn't added automatically there.

I would replace our bool variable with an int and turn the categories into flags. I find that more satisfcatory than extra bool vars for every category. But that's totally personal taste...
Or is the question here whether we should have a GF_ flag to determine whether to enable the checkbox in the launcher options? Well, we still have some bits left, Might be a reasonable thing to try...
Or we could just display all checkboxes for all games, since the user doesn't get any real feedback on what exactly he/she is ordering, anyway...

@eriktorbjorn
Copy link
Member

eriktorbjorn commented Sep 22, 2022

"Or is the question here whether we should have a GF_ flag to determine whether to enable the checkbox in the launcher options? Well, we still have some bits left, Might be a reasonable thing to try..."

I meant if the GUIO_ENHANCEMENTS flag should be split as well, so a game can have only one of the two checkboxes. (I'm inclined to think that's not worth the trouble.)

@athrxx
Copy link
Member

athrxx commented Sep 22, 2022

Yes, I also tend towards not splitting the GUIO flag. The user doesn't know what exactly he activates, anyway. And sooner or later the games will probably all have fixes for all types, anyway.

Also, having thought about it some more, I wouldn't make an extra category of the typos. Since it would imply that we're actively trying to hunt these down, while in reality we just have 2 or 3 such fixes for the most obvious things. Better to just put them in the "obvious graphic glitches" category, I guess they're pretty non-controversial.

@bluegr
Copy link
Member

bluegr commented Sep 25, 2022

Clean and simple workaround, thanks for your work! We can merge this now, and do the work of dplitting the enhancements checkbox into subcategories in another PR

Thanks!

@bluegr bluegr merged commit e16c30d into scummvm:master Sep 25, 2022
8 checks passed
@AndywinXp
Copy link
Contributor

AndywinXp commented Sep 25, 2022

So, wait, have we effectively decided that we want multiple checkboxes? I'm kind of out of the loop on this one

@dwatteau dwatteau deleted the fix/scumm-monkey1-v5-navigator-head-text-color branch Sep 25, 2022
@dwatteau
Copy link
Contributor Author

dwatteau commented Sep 25, 2022

Thanks for the merge!

So, wait, have we effectively decided that we want multiple checkboxes? I'm kind of out of the loop on this one

I'm personally not a big fan of over-configurable things, especially since our current UI makes this a bit messy IMO, but I don't see any problem with multiple checkboxes if there's a general desire for it.

I'd suggest having a dedicated Enhancements tab if we go for this though, with a top groupLeader setting which lets one disable all the checkboxes at once, so that things remain clear.

I don't know the UI and GUIO parts well, but I can help with categorizing all the existing game enhancements.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants