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,GUI: Add checkbox to toggle COMI's object_labels (Do not merge) #736

Closed
wants to merge 3 commits into from

Conversation

@salty-horse
Copy link
Member

commented Mar 31, 2016

Note: This is a work in progress. I am submitting it to get feedback.

Curse of Monkey Island has a "Show Object Line" setting in its options that changes the way the "verb" line is displayed. It is currently configurable in ScummVM only through the command line and by manually editing the configuration file. This also means it's only configurable before starting the game, and not while it's running.

This is an attempt to expose the setting in the various graphical option dialogs, specifically the launcher's "Edit Game" and GMM. It had to change some of the GUI internals, since Scumm uses the global options menu.

The first task was to extend the GUI Options to GMM, taking the Launcher code that allows engine to inject their own settings in the "Edit Game" dialog and generalizing it to also add those settings to the in-game "Config" dialog. This was accomplished by moving all of the handling of the GUI elements to OptionsDialog, which both the Launcher's EditGameDialog and the in-game ConfigDialog inherit from. This allows the new setting to appear both in the "Edit Game" dialog and the in-game GMM options dialog.

I then hooked the setting into Scumm, and made sure it's only visible for COMI. I disassembled the scripts to find the variable that is controlled by that setting. (The current support for the setting uses Scumm's "registry" and doesn't reference the variable explicitly)

Things remaining to do:

  • Add the settings to all themes. At the moment I have only added a single custom checkbox to the "Modern" theme. It might not be aligned nicely. I also need to check how many "placeholder" widgets can be crammed into the options dialog, as they take up room even when they're not rendered AFAICT.
  • Figure out if any other engines that have custom settings, and use ScummVM's in-engine options menu are affected.
  • Add the new file with translatable strings to POTFILES.
  • Should the new Scumm var have a named constant? A similar setting for Backyard Baseball 2003 (right above my change) doesn't use a constant.
  • I added the setting to ScummEngine::syncSoundSettings. That's the logical place for it, but maybe the method should be renamed? :)
  • Split this into two commits, for GUI and SCUMM?
@sev-

This comment has been minimized.

Copy link
Member

commented Mar 31, 2016

Hi @salty-horse. I really wonder, why not use GUI Options for that? Their goal is specifically to allow per-game settings like this.

@salty-horse

This comment has been minimized.

Copy link
Member Author

commented Mar 31, 2016

I'm not sure which "GUI Options" you are referring to. Care to reference the code so I could have a look?

(EDIT: This has been clarified in IRC. We're talking about the same thing. The PR description has been edited to be clearer)

true
};

const ExtraGuiOptions ScummMetaEngine::getExtraGuiOptions(const Common::String &target) const {

This comment has been minimized.

Copy link
@lordhoto

lordhoto Mar 31, 2016

Member

We have some requirement in this API that an empty target shall return all options. You will need to implement that too.

This comment has been minimized.

Copy link
@salty-horse

salty-horse Mar 31, 2016

Author Member

Will do so. Note that this isn't documented in engines/advancedDetector.h.

This comment has been minimized.

Copy link
@lordhoto

lordhoto Mar 31, 2016

Member

You need to look at engines/metaengine.h. engines/advancedDetector.h is not the generic interface (SCUMM doesn't even use the AdvancedDetector code).

@lordhoto

This comment has been minimized.

Copy link
Member

commented Mar 31, 2016

I added the setting to ScummEngine::syncSoundSettings. That's the logical place for it, but maybe the method should be renamed? :)

I think that's rename Engine::syncSoundSettings then (or introduce a new method).

Split this into two commits, for GUI and SCUMM?

Very good idea. That we can split the two distinct changes. We can easily merge the COMI option for now (after you implemented the extra GUI options API correctly). Adapting the GUI can then be done and reviewed separately.

salty-horse added some commits Mar 31, 2016

GUI: Show custom engine options in GMM Options dialog
Custom engine options have previously only been visible in the Launcher's
"Edit Game" dialog. The Launcher and GMM now share the same code.

@salty-horse salty-horse force-pushed the salty-horse:comi_object_labels branch from 4af3590 to a3cb73d Mar 31, 2016

@salty-horse

This comment has been minimized.

Copy link
Member Author

commented Mar 31, 2016

I have split the PR into three commits. The first one can be merged now. The third one is also stand-alone but doesn't do anything unless the setting is exposed in the GMM Options dialog.

I made the change to getExtraGuiOptions. An empty target will return the COMI-only setting.

If Engine::syncSoundSettings is to be renamed, I suggest doing so in a separate PR. It affects all engines. It could be just syncSettings or syncGameSettings.

Here's how the setting looks in the modern theme. Do you think it needs top/left padding?

settings

@lordhoto

This comment has been minimized.

Copy link
Member

commented Mar 31, 2016

I have split the PR into three commits. The first one can be merged now. The third one is also stand-alone but doesn't do anything unless the setting is exposed in the GMM Options dialog.

Great, in this case I would suggest to open a new PR just with the addition of the option for COMI. (And maybe rename scummComiObjectLabelsOption to comiObjectLabelsOption, it is static anyway so the scumm prefix seems out of place).

@salty-horse

This comment has been minimized.

Copy link
Member Author

commented Apr 1, 2016

The first commit (w/ modifications) has been split into #739. However, I will leave it in this PR until that one has been merged, so I could base this PR on it.


There's an issue with the themes that I'm not sure how to resolve. I want to add placeholders in the GMM Options menu for all possible checkboxes (7, according to the "Edit Game" dialog), since if an engine adds a custom checkbox and the GUI can't display it, it will throw an error and refuse to run at all. But there are some problems:

  1. The modern and classic themes don't have room for the same amount of checkboxes in the Options dialog. The low-res themes, in particular, only have room for 1 checkbox.
  2. Even if an engine only uses 1 checkbox, the theme reserves space for all of them, so the Options window becomes very tall with lots of empty space. Even disregarding checkboxes it can be a problem. e.g. BASS doesn't show the subtitle controls, so there's a big gap between volume settings and the custom setting (which is cut off at the bottom due to lack of space):
    sky

The simplest initial solution is to reduce the number of checkboxes to the max actually used (3, by Neverhood).

A more advanced solution would be to create a theme layout that only expands as widgets are added to it through code.


Another issue: Existing engines with custom settings that don't currently expect them to change mid-game. (I assume it's usually ignored and doesn't cause a crash, but I haven't checked all of them).

Should they be modified to support changing those settings, or is this something that each maintainer can do on their own time after this patch lands?

Here's a summary of engines with custom settings, and their details:

  • drascula - 1 - "Use original save/load screens"
  • neverhood - 3 - "Use original save/load screens", "Skip the Hall of Records", "full-screen making-of videos"
  • scumm - 1 - this new COMI setting
  • sky - 1 - "Show intro from floppy version"
  • sword2 - 1 - "Show object labels" (same as the new COMI setting)
  • sword25 - 1 - "Use English speech"
  • toltecs - 1 - "Use original save/load screens"

Only scumm uses the GMM as its options screen, and tsage (not listed) uses ConfigDialog independently just to display volume controls (so it's not affected by this change).

@bluegr

This comment has been minimized.

Copy link
Member

commented Apr 1, 2016

@salty-horse: there are two ways to add custom options: per engine, or per game. SCI has the most per-game options AFAIK (seven). In your analysis you only check for per-engine custom options

@salty-horse

This comment has been minimized.

Copy link
Member Author

commented Apr 1, 2016

@bluegr: I was not aware of that feature at all! That "seven" doesn't seem related to the seven checkboxes in the theme files, though, since it doesn't look like the game options are visible on any GUI...?

I see the "game options" are not injected into the GMM Options dialog. They just enable/disable the common controls that are already there. Which is odd. SCI has an "originalsaveload" option on some games that's not exposed by the GUI. Other engines expose that same flag an an "engine option" so it WILL have a checkbox after this flag.

  • Should COMI's setting be a "game option" rather than an "engine option" guarded by a conditional?
  • Should "game options" be exposed as GUI options like this PR attempt to do to "engine options"?

I find the separation of game/engine options inelegant - is there a good reason for it? They could all be "engine options", with the engine deciding which settings to expose based on the game.

@bluegr

This comment has been minimized.

Copy link
Member

commented Apr 1, 2016

Custom engine options are exposed for all the games that an engine supports. Custom game options are exposed for specific detection entries. It's not elegant to check for game-specific options at the envine level (rather than the detection level), as game specific options can be applied to specific games (depending on their platform, language, features and game ID). Putting all of this target separation logic in the engine is ugly, and it's really the work of the detector itself

@salty-horse

This comment has been minimized.

Copy link
Member Author

commented Apr 1, 2016

Deleted the previous comment

Sorry, I missed the code in AdvancedMetaEngine that adds those checkboxes to "Edit Game".

@salty-horse

This comment has been minimized.

Copy link
Member Author

commented Jun 16, 2016

What work needs to be done regarding the engine/game options to get this feature in?

Is there any chance to merge the second SCUMM commit that sets the "object_labels" SCUMM var for COMI without exposing an option for it? The code won't do anything, since the setting isn't changed in-game, but it will serve as documentation for what that SCUMM var does.

@sev-

This comment has been minimized.

Copy link
Member

commented Jun 16, 2016

Well, I was not even looking much into it, since the subject says "do not merge".

Also this one has conflicts now which must be fixed before I could take a closer look.

@salty-horse

This comment has been minimized.

Copy link
Member Author

commented Jun 16, 2016

I can fix the conflicts, but I still have some questions:

  • How should we expose game-specific settings in the in-game GMM Options menu, and what to do if there's no room on the screen to fit all of them? (Specifically in small resolutions).
  • Is the current separation of "custom engine options" and "custom game options" desirable?
@sev-

This comment has been minimized.

Copy link
Member

commented Aug 15, 2016

On the GMM Options. Now we have the Container widget. You may think about using that one. Then the problem with the space goes away.

What separation do you mean? Visually or by the concept?

@sev-

This comment has been minimized.

Copy link
Member

commented Nov 12, 2016

What is the status of this PR? It is not mergeable anymore, and it is still marked as 'do not merge'.

@salty-horse

This comment has been minimized.

Copy link
Member Author

commented Nov 16, 2016

Sorry, I think I missed your previous comment:

I haven't tried using the container object. I can try converting the settings to it.

I will try turning the setting into a "game option" rather than an "engine option".

I was asking about the conceptual and code separation of the two: Seems logical to have the same code handle "engine options" and "game options", and all of them should be exposed in GUI option menus. But let's drop that discussion for now as I want to get this patch in. 😃

@sev-

This comment has been minimized.

Copy link
Member

commented Dec 18, 2016

Exactly, so, could you make this patch mergeable?

@csnover

This comment has been minimized.

Copy link
Member

commented Oct 7, 2017

@salty-horse Is this some thing that you need more time to finish? Should this PR remain open?

@angstsmurf

This comment has been minimized.

Copy link
Contributor

commented Oct 7, 2017

Note that the description is a little misleading now. The actual option was already added to the Edit game dialog in #739. The purpose of the remaining code is to add the checkbox to the global main menu as well.

@bluegr

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

It’s been over 3 years, and this is still labelled as “do not merge”. I’m closing this, as some of the changes have already been merged, and there hasn’t been any progress or update on the rest of the changes for 2 years now.

Once these changes have reached a more mature state, a separate pull request can be opened. Until then, there is no point in keeping this open, as this feature has been partly merged into master anyway.

@bluegr bluegr closed this Aug 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.