#8385 closed patch
ALL, SCUMM: Subtitle speed control
| Reported by: | salty-horse | Owned by: | fingolfin |
|---|---|---|---|
| Priority: | normal | Component: | GUI |
| Version: | Keywords: | ||
| Cc: | Game: |
Description
Related feature request: https://sourceforge.net/tracker/?func=detail&aid=664823&group_id=37116&atid=418823
The subtitle options were moved to a different tab due to space constraints. Notice the "Subs"
Before trying the patch, set talkspeed to a value between 0 and 9 under [scummvm] in your settings file.
Standing problems:
* Different engines have different min/max ranges for talkspeed. This creates problems with the slider, and warrents the fix given above.
What should be done about them?
1) Scale the global [scummvm] value according to the max values of each? There is no linear relation between the values. Without the scaling, the key should not exist on that domain.
2) Add different talkspeed keys for each engine?
3) Default talkspeed values for each engine should be added.
Notice the duality here: queen/queen.cpp:112 base/gameDetector.cpp:150 - defined under #ifndef DISABLE_SCUMM. Should be fixed!
4) Configuration file: Currently, the user can state one global value, and different values for each target. Since the talkspeed signifies the same speed for each engine, should an engine domain be added? ([scumm], [simon])
TODO: * resize scumm dialog for WINCE (scumm/dialogs.cpp:398)
Ticket imported from: #1091170. Ticket imported from: patches/490.
Attachments (4)
Change History (33)
comment:1 by , 19 years ago
comment:3 by , 19 years ago
It would be intersting to see if we can somehow re-define the talkspeeds so that the different engines use the same scale... hrm
comment:4 by , 19 years ago
I know it's been open for a long time.. sorry for the delay. I've been having less and less free time (when my brain functions normally) to work on it. If it's important to have in the next release, I have no problem passing it on to someone else next week.
comment:6 by , 19 years ago
Attached patch. Here's the summary of the changes. TODO's require feedback, if you may.
The new subtitle settings are: * Enable speech checkbox * Show subtitles checkbox * Subtitle speed slider (values 0-255)
When the show subtitles checkbox is off: * the slider is disabled * "Enable speech" is toggled on and disabled
TODO in this area: * non-talkie games should have the subtitle checkbox locked as "on" and the speech checkbox locked as "off"
The subtitle settings appear in the launcher under options > audio tab, along with the volume settings Also, in the "edit game" dialog, in their own "Subtitles" tab, with a regular "override global settings"
TODO in this area: * The "edit game" gui doesn't look well in the old low-res gui mode. There's not enough room to cramp the settings in the other tabs * I think speech volume belongs under volume, and not near the new "enable speech". Any objections?
talkspeed scaling: The talkspeed value in the config file and launcher is always in the range of 0-255 Each engine sees its own internal value. When users of the scumm engine see a speed of 9, it's actually 255
How it works? Each engine "registers" it's min-max speed range in the engine constructor using ConfMan.setTalkspeedScale(minValue, maxValue). Then, whenever a request to the subtitle speed is made to ConfMan, the scaled value is returned transparently. If such a func isn't called, which means we're probably in the launcher, the scale used is 1 (which means the range is 0-255).
Engine specific information: * BASS and SWORD don't have talkspeed support. * Queen scales correctly * *** TODO TODO *** I don't hava SAGA, so I'll ask someone else to add that small scaling func to saga/saga.cpp
TODO
* There is a small loss of accuracy with the scaling funcs in common/config-manager.cpp When using the scale value of 0.035294 and going from the scumm value 7 to the global 198, the backwards trip yields 6.
In the scumm engine: The options dialog uses the same volume controls drawing funcs as the launcher. I used the same trick for the subtitle drawing, but since the launcher and scumm have different talkspeed values, I added a small default parameter to distinguish the ranges 0-255 and 0-9
the talkspeed value is saved to the ini file on the same occasions as the volume: * When changed via the +- hotkeys * When changed via the scumm options menu, and "OK" is pressed * It is loaded when the engine is initialized
I added "DEBUG PRINT"s to common/config-manager.cpp to test the scaling. They should be removed before check-in (if one occurs).
final TODO: I don't think a special effort should be made to convert old configuration files to new values. Or Should It?
comment:7 by , 19 years ago
Best to name your patches subtitle.v1.patch, subtitle.v2.patch or something like that.
Also in your patch you mix tabs with 4 spaces in many places. That should be fixed.
> The new subtitle settings are: > * Enable speech checkbox > * Show subtitles checkbox There is an inconsistency here. You put these controls to Audio tab in global options but have separate tab in game options. Subtitles belong to audio
Though we already have such inconsistency with volume there. I'd put it on one tab and if I'd want to override them independently I'd add 'override' checkbox next to each group of controls.
> * non-talkie games should have the subtitle checkbox locked > as "on" and the speech checkbox locked as > "off" What about semi-talking games like floppy versions of DOTT, S&M and ITE where voices presented in intro?
> * *** TODO TODO *** I don't hava SAGA, so I'll ask > someone > else to add that small scaling func to saga/saga.cpp Demo is free and playable. Though it is full talkie and so doesn't have subtitle speed. In floppy version subtitle speed varies in range 0-3 (_readingSpeed) Fast, Mid, Slow, Click. 'Click' means infinite, when you should click to proceed. For CD version there is no control of subtitle speed.
> the talkspeed value is saved to the ini file on the same > occasions as the volume: > * When changed via the +- hotkeys [your patch]: > input.cpp: ConfMan.set("talkspeed", _defaultTalkDelay); It is not saved but stored. ConfMan doesn't flush file to disk without request. Which happens in scumm module only either on obsolete target update, or in special HE-related opcodes or when you save a game. I.e. in this case you will lose it when exit ScummVM, but that's intentional.
> I don't think a special effort should be made to convert old > configuration files to new values. Or Should It? You definitely should. If users someday will get text changed 25 times per second, that will at least get them puzzled.
In GUI I would put Show subtitles next to Enable speech, or at least would make more space between subtitles speed gauge and OK button. Looks too squashed now.
comment:8 by , 19 years ago
For the speech/subtitle settings, we really have three possible settings to choose from (Speech+Subtitles; Subtitles only; Speech only). This is a bit awkward using checkboxes. So I guess we'd really like to have either a new radio button control, or use a popup button for those three...
As for the value ranges: 25 times per second shouldn't be possibel anyway. An engine should always perform sanity checks on such external values and cap them appropriately. That's obviously not an argument *against* auto-updating config files, mind you. It's just an argument against your justification why it's necessary :-).
comment:9 by , 19 years ago
Well, I didn't want to be mathematically precise with that number (better choose more straightforward examples next time :) ). Let's say user is a slow readed and when one day after upgrading ScummVM he gets all his dialog in fastest mode, that will at least annoy him. So config file definitely should be upgraded.
But this arises need of config file versioning (not a bad thing after all, we may upgrade something more in the future). Our current 'versioninfo' doesn't serve the purppose as it reflects major version of ScummVM which changes pretty rarely. Probably additional configversion with incremental numbers like we do with saves now could be a solution.
comment:10 by , 19 years ago
* SAGA's slowest subtitle mode, which requires clicking to move the text, may not be set correctly with the scaling functions inaccuricies. I'll change the funcs so that the global 255 value will be automatically translated to the maximum engine value without all the floating point arithmetic in the middle. * About the talk/speech radio button, SKY's gui works that way. I think a pop-up is not the best visual choice for that, and since there's no radio button ATM, how about this: a text label specifying "text and speech settings:" and a button which changes its text every time it is clicked (like SAGA) * _sev, are you suggesting to put both the volume and subtitle tabs into the audio tab of the "edit game" dialog? It's a good idea, consisting with the general options dialog. I'll try it and post the results. I hope it will all fit in the low-res gui.
comment:12 by , 18 years ago
Here's a new version of the patch. * No more scaling inaccuricies: ** Lowest and maximum values stay that way no matter the scaling. ** Scaling stays accurate when converting from/to engine/global - tested with all of SCUMM's slider notches. * Moved subtitle controls to the Audio tab. It now resides below the volume settings in the "options" menu, and immidiately below the music driver in the "edit game" menu. * All GUI variations are supported. (New theme, old theme) X (320x, 640x). I even made small adjustments to the GUI here and there: The "aspect ratio correction" text wasn't displaying correctly in one of the 320x themes, volume description texts weren't greyed out along with the sliders. * Subtitle/speech toggling is now a shiny button instead of two checkboxes. I favored that widget against the drop-down list - same as BASS's widget. * In SCUMM, the options slider is now correctly oriented as Slow |-----| Fast, like the -/+ hotkey OSD.
* I didn't address the SCUMM hotkey vs. dialog screen directly, BUT what happens now is this: When changing the subtitle speed via hotkeys and exiting, nothing is saved. When changing via hotkeys and entering the options screen, the hotkey setting is displayed, but still not saved. When pressing OK, the setting that was displayed in the option screen is saved. The hot-key starting speed is taken from the wrong domain, though. I will fix this. * The scaler init function still needs to be called for SAGA and tested for its 4 speeds. * The wording of the "subtitles and speech" label needs rework - I took the different mode descriptions from SCUMM's CTRL-T hotkey.
comment:13 by , 18 years ago
I'm uploading a new version with a small fix: The max engine value wasn't initialized for the launcher, so it didn't handle the value 255 correctly.
Also, I found the problem with the SCUMM hotkey. It's unrelated to the patch. When the hotkeys are first pressed, I compared the values of _defualtTalkDelay and VAR_CHARINC. _defaultTalkDelay was 3, while VAR_CHARINC was 9 - the correct config value. Both variables are initialized to the config value in the engine's initialization. I'll open a different bug for it.
comment:14 by , 18 years ago
Hacking the ConfigManager to deal with the talkspeed range conversion etc. is not acceptable. It is supposed to be completely independent from specific engines and shouldn't have to know anything about specific config keys.
An alternative solution which would be acceptable: Add getTalkSpeed/setTalkSpeed methods to the affected engines, which are layered between the ConfigManager value and the rest of the engine.
comment:15 by , 18 years ago
So you prefer ConfigManager transparency over engine transparency? OK. I'll add the get/setTalkSpeed funcs to the relevant engines.
comment:16 by , 18 years ago
Yes, I do. The general support framework (of which the ConfigManager is part of) should have as little as possible knowledge about Engine specifics... the behaviour of one specific config key certainly *is* Engine specific.
comment:17 by , 18 years ago
Due to the scumm engine sharing the talkspeed slider widget with the launcher dialogs, I had to use a little trick. I am attaching a patch with the changes for review - (note that it still has the old changes to ConfigManager - I'm just asking for a review of the problem described) Tell me if it's not too wrong: Already in the last submitted patch, addSubtitleOptions() recieves a maximum talkspeed value, in order to set the max value of the slider accordingly.
Since talkspeed scaling is now outside of ConfigManager, GUI_OptionsDialog::open/close needs to know by itself if it's interacting with the value via the launcher or SCUMM. I added logic in gui/options.cpp that examines the slider's max value and scales the get/set values according to it. I admit it's not so pretty.
At first I thought that I could somehow inject the correct values in scumm's ConfigDialog::open/close, which wrap the gui dialog, but the actual set/get is done inside the gui's classes.
Another approach can be taken, which seperates the launcher/scumm widgets completely: remove the talkspeed widget from addSubtitleControls, and create different "addTalkspeedControls" for SCUMM and the launcher.
I think the last approach is the cleanest. What do you think?
by , 18 years ago
| Attachment: | scumm-subtitles-hack-for-review.patch added |
|---|
possible SCUMM talkspeed solution for review
comment:18 by , 18 years ago
I haven't yet looked at the patch, but why does the ConfigManager need to know about the value ranges at all? In particular, since we also want to use it from the launcher, this would require the launcher "Edit game" dialog to somehow query the plugin responsible for a given target for the acceptable min/max range. I.e. unless I misunderstand you (which is very much possible, sorry in that case :-/), what you describe wouldn't handle that and hence wouldn't work correctly in the "Edit Game from launcher" scenario.
The only clean way I see would be as follow (and as was suggested by me in my very first comment on this tracker item *g*):
Always use a range of 0 to 255 in the GUI code, and always store values from 0 to 255 in the "talkspeed" setting. Uniform across all engines. Document it as such, too, in the README, usage string, etc.. Then modify all engines. Whenever they do ConfMan.get/set("talkspeed"), let them convert between their internal range and the 0..255 range.
That way, the generic framework code doesn't have to know anything about the engine(s) it's dealing with.
Is there anything which I am missing that is preventing this or making it a bad solution? Otherwise, it would seem to me to be the most natural way to go about this, wouldn't it?
comment:19 by , 18 years ago
Those are exactly the guidelines I followed. I agree the configmanager doesn't need to know the ranges, and I applied some of those changes in the new, yet uncomplete, patch. I submitted it only to show the specific SCUMM engine problem - I did not yet remove the ConfigManager "hack" from the other engines.
comment:20 by , 18 years ago
I understood your previous comment (at least I thought so), and my reply was directed precisely at that:
The GUI code should always use the 0-255 range (or whatever other range we want to use, that's not the point :-). No matter whether it is used in the launcher or from within the GUI engine.
Hence, no need for any hack. And I don't see a SCUMM engine specific problem either.... It just does the usual talkspeed conversion, back and from...
comment:21 by , 18 years ago
Attached a new patch, without the ConfMan scaling code. Now each engine takes care of its own talkspeed values.
by , 18 years ago
| Attachment: | subtitles-no-confman.patch added |
|---|
Removed talkspeed-scaling code from ConfMan
comment:22 by , 18 years ago
Looks mostly good, great! A few trifles:
* The talkspeed conversion in Queen looks wrong to me. It should convert between the range MIN_TEXT_SPEED..MAX_TEXT_SPEED and the range 0..255. But instead of the former, it looks as if it was using the range 0..(MAX_TEXT_SPEED-MIN_TEXT_SPEED) (unless I am missing something, which is quite possible).
* Those _talkspeedScale private vars are essentially constants, and could be removed. In fact, it shouldn't be necessary to use floats for this either. Code like this:
void ScummEngine::setTalkspeed(int talkspeed) { if (talkspeed != 0) if (talkspeed == 9) talkspeed = 255; else talkspeed = (int)(talkspeed / _talkspeedScale + 0.5); ConfMan.setInt("talkspeed", talkspeed); }
could simply be replaced by this
void ScummEngine::setTalkspeed(int talkspeed) { ConfMan.setInt("talkspeed", talkspeed * 255 / 9); }
I can do all these changes, but wanted to hear your thoughts before commiting anything either way.
comment:23 by , 18 years ago
| Owner: | set to |
|---|
comment:24 by , 18 years ago
1) I certainly forgot about Queen's minimal value. Since it's such a small value (4), it's easy to miss it when testing. What do you think about tuning that scale 4 points down (from 4-100 to 0-96)? It's cleaner, and won't mess with people's configs since there are already checks for overflows (if someone has a value of 100, it will automatically be moved down to 96).
2) It's wrong not to use floats for those calculations. when using integers, scaling a value and un-scaling it yields incorrect results: 5 * 255 / 9 = 141 but 141 * 9 / 255 = 4 (should be 5)
Funny thing is, you can't see it happening in SCUMM because it never uses setTalkspeed. The only time the SCUMM engine saves values is when using the Options dialog, and then, the code that's responsible for the value setting is GUI:OptionsDialog (which uses floats).
3) When you *do* use floats, the extreme values of 0,255 aren't converted correctly to 0,9. That's why the specific checks are there. For Queen, I forgot those checks as well.
4) I put the constants are there for readability, and to prevent extra float calculations, (they aren't very effective at that). I'll remove them.
comment:25 by , 18 years ago
re 1) I don't mind which scale is used as long as now functionality is lost
re 2) First off, let me state that I really don't
care strongly whether use float or int here. But
while it's true that it's wrong to use int scaling
the way I did in my example (i.e. w/o properly
rounding), you still can do all this int-only.
Simple do rounding:
(5*255 + 4)/9
i.e.
(val*255 + maxVal/2) / maxVal
and the other way:
(142 * 9 + 128) / 255
i.e.
(val*maxVal + 255/2) / 255
work well enough. Note that you also have to do
these "rounding tricks" (by adding 0.5) in the
float case. Anyway, it might really be a good idea
to add a simple CONVERT_RANGE macro, which could
hide these details, so we could switch between
float and int whenever we like. Alas, this is
minor, and if you prefer to use floats, that's
fine by me, too. :-)
re 3) Note that the above int code works correctly in the corner cases, too.
re 4) Fine!
comment:26 by , 18 years ago
I removed the floats usage, and changed the queen scaling code to take the minimal value into account. (I didn't change the queen scale to start from 0).
comment:27 by , 18 years ago
Added to Subversion. Thanks! I still need to add you to the credits, any special wishes?
comment:28 by , 18 years ago
| Status: | new → closed |
|---|
comment:29 by , 5 years ago
| Component: | → GUI |
|---|
In the game configuration dialog, the Volume tab is written as "Vo...me". What can be done about it? Currently, the tab width of all tabs is even, with room for tab-scrolling arrows. If this isn't going to change, maybe "Volume" should become "Vol".