Opened 15 years ago

Closed 13 years ago

Last modified 10 months ago

#8385 closed patch

ALL, SCUMM: Subtitle speed control

Reported by: salty-horse Owned by: fingolfin
Priority: normal Component: GUI
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)

subtitles-new-fixed.patch (29.4 KB) - added by salty-horse 13 years ago.
Fixed small talkspeed=255 bug
scumm-subtitles-hack-for-review.patch (31.1 KB) - added by salty-horse 13 years ago.
possible SCUMM talkspeed solution for review
subtitles-no-confman.patch (29.0 KB) - added by salty-horse 13 years ago.
Removed talkspeed-scaling code from ConfMan
subtitles-no-floats.cpp (28.5 KB) - added by salty-horse 13 years ago.
Removed floats usage

Download all attachments as: .zip

Change History (33)

comment:1 Changed 15 years ago by salty-horse

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".

comment:2 Changed 15 years ago by sev-

What is the status of this item?

comment:3 Changed 14 years ago by fingolfin

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 Changed 14 years ago by salty-horse

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:5 Changed 14 years ago by sev-

salty-horse, any further progress?

comment:6 Changed 14 years ago by salty-horse

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 Changed 14 years ago by sev-

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 Changed 14 years ago by fingolfin

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 Changed 14 years ago by sev-

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 Changed 14 years ago by salty-horse

* 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:11 Changed 13 years ago by sev-

What is the status of this item?

comment:12 Changed 13 years ago by salty-horse

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 Changed 13 years ago by salty-horse

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.

Changed 13 years ago by salty-horse

Attachment: subtitles-new-fixed.patch added

Fixed small talkspeed=255 bug

comment:14 Changed 13 years ago by fingolfin

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 Changed 13 years ago by salty-horse

So you prefer ConfigManager transparency over engine
transparency? OK. I'll add the get/setTalkSpeed funcs to the
relevant engines.

comment:16 Changed 13 years ago by fingolfin

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 Changed 13 years ago by salty-horse

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?

Changed 13 years ago by salty-horse

possible SCUMM talkspeed solution for review

comment:18 Changed 13 years ago by fingolfin

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 Changed 13 years ago by salty-horse

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 Changed 13 years ago by fingolfin

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 Changed 13 years ago by salty-horse

Attached a new patch, without the ConfMan scaling code. Now
each engine takes care of its own talkspeed values.

Changed 13 years ago by salty-horse

Attachment: subtitles-no-confman.patch added

Removed talkspeed-scaling code from ConfMan

comment:22 Changed 13 years ago by fingolfin

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 Changed 13 years ago by fingolfin

Owner: set to fingolfin

comment:24 Changed 13 years ago by salty-horse

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 Changed 13 years ago by fingolfin

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 Changed 13 years ago by salty-horse

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).

Changed 13 years ago by salty-horse

Attachment: subtitles-no-floats.cpp added

Removed floats usage

comment:27 Changed 13 years ago by fingolfin

Added to Subversion. Thanks! I still need to add you to the credits, any special
wishes?

comment:28 Changed 13 years ago by fingolfin

Status: newclosed

comment:29 Changed 10 months ago by digitall

Component: GUI
Note: See TracTickets for help on using tickets.