SCI: KQ7: Odd behavior for disabling subtitles

[antoniou79]

This was tested on ScummVM 2.8.1 and recent dev build of 2.9.0git on Windows 10.

Under some conditions subtitles for the game (and the early disclaimer warning about them when launching the game) are not disabled despite the Audio setting for the game (from the ScummVM launcher, Game Options) is set to "Speech".

This was brought up on the ScummVM forum here:
​https://forums.scummvm.org/viewtopic.php?p=100028#p100028

My steps to reproduce this (some steps may be redundant):

1. Have ScummVM's "Global Options" for "Audio" set to "Both"
2. Add the KQ7 game to ScummVM
3. Launch the game. It will start with the disclaimer and subtitles enabled, as expected.
4. Exit the game, go to "Game Options" specifically for the game, override the global settings, set Audio to "Speech", click ok.
5. Re-launch the game. It continues to show the warning and subtitles.

At this point it's expected that the game would *not* show the disclaimer warning nor the subtitles. Yet, even the entry in the scummvm.ini file is not updated to have a key-value for "subtitles" as it should.

The issue can be resolved, if the user goes (again) to the Game Options for the game, and sets the Audio to "Text", clicks Apply (at this point the scummvm.ini entry for the game gets a key-value for "subtitles" as "true"). Then, go to Game Options -> Audio again and set the Audio to "Speech", which now does update scummvm.ini and sets "subtitles" to "false".

[eriktorbjorn]

There seems to be something wrong with this part of OptionsDialog::apply():

if (subtitles != ConfMan.getBool("subtitles", _domain)) {
	ConfMan.setBool("subtitles", subtitles, _domain);
	_subToggleDesc->setFontColor(ThemeEngine::FontColor::kFontColorNormal);
}

You've changed the "subtitles" setting to false, but it's already false by default for this game so it doesn't see fit to write the setting to the file? Something like that, anyway?

[m-kiewitz]

Our settings within ScummVM are just trying to set subtitles + audio for games, but there are some games that reset these in game variables to the default on startup, and thus ignore what we set.

In these cases the only solution would be a script patch.

Would have to look into this specific game to be sure that it's this.

Which KQ7 do you use? 1.51 or 2.0?

[eriktorbjorn]

The version file of my copy of King's Quest VII says "2.00b". But I'm still leaning towards an error int he ScummVM options dialog. Let's try from a completely blank slate.

I started ScummVM and changed the global audio settings to "Both". This is now my scummvm.ini:

[scummvm]
gui_browser_native=true
mute=false
speech_volume=192
native_mt32=false
mt32_device=null
confirm_exit=false
gui_use_game_language=false
gui_scale=100
gui_return_to_launcher_at_exit=false
gui_disable_fixed_font_scaling=false
subtitles=true
gm_device=auto
sfx_volume=192
music_volume=192
autosave_period=300
music_driver=auto
opl_driver=auto
aspect_ratio=true
versioninfo=2.9.0git10472-gec1c846eaff
speech_mute=false

I add the game (selecting the "DOS" version), but change the Audio settings to override the global settings so that I get "Speech" instead without subtitles. This is the entry I get for the game:

[kq7]
description=King's Quest VII: The Princeless Bride (DOS/English)
gmm_save_enabled=false
extrapath=dists/engine-data/
path=/path-to/kq7
enable_video_upscale=true
engineid=sci
gameid=kq7
language=en
music_driver=auto
platform=pc
enable_hq_video=true
opl_driver=auto
speech_mute=false
guioptions=sndLinkSpeechToSfx sndLinkMusicToSfx noAspect gameOptionA gameOptionD gameOptionI lang_English

Note that there is no "subtitles" setting in the entry.

I _think_ what happens is that when the options dialog changes the settings, it checks if there is a setting specifically for the game and gets the default value (false). This is the same as what you're trying to save, so it doesn't think it necessary to explicitly write the setting to the file. When the engine checks the setting, it sees that there is no game-specific setting, so you get the global setting (true).

If you change the game-specific setting to one where "subtitles" is true, the dialog _does_ see that it has changed and writes it to the file. Then when you change it back to voices only, the setting has once again changed, so it writes it to the file. Now everything works as expected.

[m-kiewitz]

Can you please try an earlier ScummVM version?
Maybe this is a regression in the settings, but it's weird because I would expect such a problem to get reported basically immediately elsewhere.

I think back then I programmed it in a way, that the game itself changes the game-specific settings too, so if the game sets defaults or changes subtitles on/off, that change is saved to the game specific settings, at least that's what it was originally. And yes, when the global setting is the same as the game specific setting, the game specific setting is not saved individually.

[eriktorbjorn]

I went back to a much earlier version, and there it worked. I then tried bisecting for a bit. Here's one point where it broke:

25401cdf922ca629d8e6c8d8664a07b0c755b897 is the first bad commit
commit 25401cdf922ca629d8e6c8d8664a07b0c755b897
Author: Filippos Karapetis <bluegr@gmail.com>
Date:   Thu Jun 30 17:14:35 2022 +0300

    GUI: Add missing domain when checking for subtitle config - bug #13629

    This disallowed selecting "both" for speech and subtitles, as the
    subtitles setting was never saved. A regression from d6dbf721b62e773f.

 gui/options.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

I don't know if this is the only point. I deliberately bisected a very small interval, because I had a hunch that it would break here.

[eriktorbjorn]

Originally, it would always write both "subtitles" and "speech_mute" to the config manager, like so:

ConfMan.setBool("subtitles", subtitles, _domain);
ConfMan.setBool("speech_mute", speech_mute, _domain);

This was changed in ​https://github.com/scummvm/scummvm/commit/d6dbf721b62e773f529276f5b1c0c3ce59df6f47 ("ALL: add support for --subtitles") to this:

if (subtitles != ConfMan.getBool("subtitles")) {
	ConfMan.setBool("subtitles", subtitles, _domain);
	_subToggleDesc->setFontColor(ThemeEngine::FontColor::kFontColorNormal); 
}
ConfMan.setBool("speech_mute", speech_mute, _domain);

And then, finally, ​https://github.com/scummvm/scummvm/commit/25401cdf922ca629d8e6c8d8664a07b0c755b897 ("GUI: Add missing domain when checking for subtitle config - bug #13629") changed it to:

if (subtitles != ConfMan.getBool("subtitles", _domain)) {
	ConfMan.setBool("subtitles", subtitles, _domain);
	_subToggleDesc->setFontColor(ThemeEngine::FontColor::kFontColorNormal); 
}
ConfMan.setBool("speech_mute", speech_mute, _domain);

I'm not sure I understand even the first change here. Maybe @sev knows?

[eriktorbjorn]

This is a tempting change to make, but I don't know if I'm on the right track:

diff --git a/gui/options.cpp b/gui/options.cpp
index d000413fae8..b586d17cae9 100644
--- a/gui/options.cpp
+++ b/gui/options.cpp
@@ -1086,7 +1086,7 @@ void OptionsDialog::apply() {
                                        break;
                                }
 
-                               if (subtitles != ConfMan.getBool("subtitles", _domain)) {
+                               if (!ConfMan.hasKey("subtitles", _domain) || subtitles != ConfMan.getBool("subtitles", _domain)) {
                                        ConfMan.setBool("subtitles", subtitles, _domain);
                                        _subToggleDesc->setFontColor(ThemeEngine::FontColor::kFontColorNormal);
                                }

[sev-]

Would be nice to get fixed before the release.