[WIP] SCUMM: COMI [RFC]: Implement original GUI and Save/Load menu

[AndywinXp]

Ciao, while not being 100% complete, I'd say this feature is ready enough for some testing and opinions about the quality of the code! While this setting is active, the ScummVM main menu is disabled: this is by design, since COMI menu does everything that the ScummVM menu does (except for getting back to the launcher).

Here are some screenshots of what this PR achieves (the palette being different in SMUSH videos is exactly what should happen):

Known issues or currently missing features which are responsible for the WIP tag:

• Manually assign the menu script number and menu script key when starting a game from a savefile from the ScummVM launcher (otherwise it's not possible to open the main menu in those instances);
• Assign correct black and white colors for the system cursor in SMUSH videos;
• I still have to implement the remaining information banners (like the one which controls voice mode);
• Savegame thumbnail handling is currently not implemented;
• COMI Demo is currently not supported;
• Sometimes clicks are not being registered in the Main Menu (in the save/load pages); the precise issue seems to be that a click, if not held long enough, will not be registered, but there's yet to understand why;
• I have to be sure that everything is being rendered in a pixel perfect way.
• Disable autosaving when the original GUI is active;
• Use the Graphics::PaletteLookup class for... you guessed it, palette lookup on the GUI colors :-)
• Use the ScummVM thumbnails for the main menu when an older savegame is detected, and viceversa, find a way to use the new SCUMM thumbnail for the ScummVM launcher;
• Every text not related to the GUI item is being deleted (or rather, the relative rects are marked as dirty) when the GUI is visible;
• (Related to the one above) blastTexts and blastObjects removal appears to happen one frame late (particularily visible when going though the main menu);

The implementation of the GUI is not necessarily tied to COMI (while it is only enabled for it): this is because the building blocks of the GUI found in the disasm of COMI seem to be conceptually the same being used within v5-6-7, except for the main menu which would need a couple more (easy) functions to be implemented.

[sev-]

I welcome this improvement, it is always cool to see the original dialogs and even functioning.

I made couple of notes, questions and suggestions

[sev-]

I would recommend implementing caching here. E.g. if palette is not changed, then return the colors from a hash.

In general, this functionality is needed in so many places, so I would implement something like

class Graphics::PaletteLookup;

PaletteLookup();
PaletteLookup(byte *palete, int len);

// This method replaces palette and resets the cache
// It also _compares_ given palette with the current one and
// resets cache only when their contents is different.
void PaletteLookup::setPalette(byte *palete, int len);

// This method returns closest color from the palette
// and it uses cache for faster lookups
byte PaletteLookup::lookupColor(byte r, byte g, byte b);

Common::Hash<int, byte> _paletteLookup;

Sample implementation of the cached palette lookup could be found here:
https://github.com/scummvm/scummvm/blob/master/graphics/macgui/macwindowmanager.cpp#L1305

You may just make it as part of this PR, just in separate commits.

[AndywinXp]

Thanks, in the following days I'll take a look at this and build a system like this!

[sev-]

@AndywinXp coincidentally, I need exactly this functionality for Director, so I will implement this class today, so then you can just reuse it.

[sev-]

Done, it now lives in graphics/palette.h

[AndywinXp]

Thanks sev!

[sev-]

Will we have two thumbnails in the file now? One for the ScummVM GUI and another one for the original dialog?

[AndywinXp]

Yes, for COMI. It doesn't have to be this way of course, but my graphic programming abilities are currently non-existent so I couldn't find a good solution which prioritizes the original 256 colors thumbnail over the ScummVM generated thumbnail (which has a lower resolution and a higher color depth).

[AndywinXp]

Thanks sev for the thorough review, I appreciate that! I'll answer to each question separatedly in each review box

[AndywinXp]

The PR is currently stalled in its WIP state because of me hitting a dead end after several days of work on the last two points. I have a question for somebody who knows the COMI disasm better than me (@athrxx maybe?), and in particular about blastTexts and blastObjects:

In the disasm this seems to be the flow for the main loop:

• Process keyboard and mouse input (which includes showing GUI banners and contextually halting the engine at that point when needed);
• Handle savegame queue (we don't do this here, but instead a little bit below; this doesn't appear to be an issue though);
• Reset the blastObjects queue (this is equivalent, for us, to just doing _blastObjectQueuePos = 0);
• Busy timer wait (we don't do that here but we do it in waitForTimer(), which happens before processing the input;
• Update SCUMM vars (equivalent to our scummLoop_updateScummVars());
• ...
• Swap the main screen buffer with the backbuffer (we appear to do that in _system->updateScreen(), maybe? Which is by the way done inside of waitForTimer());
• ...
• redrawBGAreas();
• (Depending on VAR_BLAST_ABOVE_TEXT the order of the following is swapped):
  • Call drawBlastTexts(); in their implementation the queue is cleared at the end of the loop which draws each blastText, while we do it at the end of ScummEngine_v6::drawDirtyScreenParts().
  • Call drawBlastObjects(); their implementation doesn't clear the objects at all in this phase, but we do that in ScummEngine_v6::drawDirtyScreenParts().
  • Update the palette and the display.

Now: what am I missing? I have been rearranging these blocks for several days but I couldn't solve the problem, which is, blastText and blastObjects disappear whenever a GUI element is displayed on screen. Also their implementation doesn't explicitly restore the background rects which hosted those blast entities, but they just clear the respective queues: how are they doing it?

[athrxx]

Okay, since you have pinged me I have checked out this branch to test it and help debug.

The load screen really looks weird with the empty thumbnails (the old savegames). I think we really should manage to display our own thumbnails in a converted for here instead of bumping the savegame version and having two different thumbnails. It will just make all the old savegames look unpleasant in the original Gui and it shouldn't be too much of a deal to convert (scale it a bit and use sev's color lookup for the CLUT colors).

For the perceived issue with the blast texts, I haven't been able to understand the precise point of your unhappiness. The "subtitles" in the main menu seem to appear and disappear as they should. I haven't seen a difference compared to running the original interpreter. So, you might explain more precisely what we have to do to reproduce some glitch here.

What I did notice, is something with the actor dialogue lines: When an actor is having an active dialogue text line and you press F1 for the menu and then leave the menu again, the original will restore the dialogue text line at the the exact original position. If I do that in ScummVM, the text line will be restored, but in a wrong position.

[AndywinXp]

Thanks for the answer athrxx!

The load screen really looks weird with the empty thumbnails (the old savegames). I think we really should manage to display our own thumbnails in a converted for here instead of bumping the savegame version and having two different thumbnails. It will just make all the old savegames look unpleasant in the original Gui and it shouldn't be too much of a deal to convert (scale it a bit and use sev's color lookup for the CLUT colors).

As soon as I understand how to do that, that seems like a viable solution; but I would still keep the proper (SCUMM based) thumbnails for new saves (hence the new savegame version) instead of mangling the ScummVM ones even when we're able to offer the proper ones for this game.

For the perceived issue with the blast texts, I haven't been able to understand the precise point of your unhappiness. The "subtitles" in the main menu seem to appear and disappear as they should. I haven't seen a difference compared to running the original interpreter. So, you might explain more precisely what we have to do to reproduce some glitch here.

You're right, I should have been clearer about that, sorry. The issue doesn't concern the main menu per se, but the new GUI elements (the pause banner, the quit dialog, etc). Try activating those in any situation in which a blastText or a blastObject is on screen, and the latters will be wiped away.

I believe this to be directly connected to an issue for which there have been workarounds in the past (i.e. loading the recipe book and closing it leaks the recipes text on the next screen for a game frame; going in the main menu, going to a save page in which there's at least an unused slot, and then going back to the main page of the menu leaks the graphic of the empty slots - blastObjects - on the screen for a game frame). This is why I believe there's a discrepancy between our blast render pipeline and the original one. Fixing this has a very high chance of rendering certain related workarounds obsolete (one of which was commited by me recently...).

What I did notice, is something with the actor dialogue lines: When an actor is having an active dialogue text line and you press F1 for the menu and then leave the menu again, the original will restore the dialogue text line at the the exact original position. If I do that in ScummVM, the text line will be restored, but in a wrong position.

True, I have noticed this: this is an issue with me forgetting to save the flag data (and possibly another field or two) when we did the blastText revamp some months ago. I will address this in a separate PR, after this PR is closed and merged, unless a good reason shows up to consider those changes related to the ones in this PR.