Opened 12 years ago

Closed 12 years ago

Last modified 12 months ago

#8689 closed patch

SWORD1: Possible patch for bug #1730183

Reported by: eriktorbjorn Owned by: sev-
Priority: normal Component: Engine: Sword1
Keywords: Cc:
Game: Broken Sword 1

Description

As described in bug #1730183, when changing the game settings by using the Broken Sword 1 options dialog, they're not saved in the ScummVM config file. This patch tries to fix that.

This is not entirely without complications. Unlike ScummVM's option dialog, the BS1 one has separate volume settings for the left and right channel. I'm not sure if there's any really sensible way of handling this, but here's how it's done in this patch:

For each of the volume settings, e.g. "music_volume", there are two extra settings, e.g. "music_volume_left" and "music_volume_right".

The BS1 options dialog will either store the standard setting or the left/right settings, but never both. The unused volume settings are removed. However, it's possible to get both by using both dialogs.

When loading the settings, if there is a standard setting made explicitly for the active domain, we use that. Otherwise, if there is a left/right setting, we use that. As a last resort, use the standard setting which will then be the one from the [scummvm] section.

It looks like a hack, it feels like a hack... maybe it is a hack. I'm not sure.

Ticket imported from: #1733017. Ticket imported from: patches/794.

Attachments (3)

sword1-options.diff (5.2 KB ) - added by eriktorbjorn 12 years ago.
Patch against current SVN
sword1-balance.diff (3.3 KB ) - added by sev- 12 years ago.
Balance patch
sword1-balance2.diff (3.2 KB ) - added by fingolfin 12 years ago.

Download all attachments as: .zip

Change History (26)

by eriktorbjorn, 12 years ago

Attachment: sword1-options.diff added

Patch against current SVN

comment:1 by eriktorbjorn, 12 years ago

This means that if you have a left/right setting, the volume displayed in the standard options dialog will be completely wrong, of course.

comment:2 by fingolfin, 12 years ago

An alternative "solution" would be to just disallow the user from alterting the channels independently. That is, force the two volume bars to be always in sync.

Or is there any particular reason to allow different left/right speaker volumes this way? It seems to be one of those "let's do it just because we can" features...

comment:3 by fingolfin, 12 years ago

Robert, any oppinion on this one?

comment:4 by fingolfin, 12 years ago

Owner: set to lavosspawn

comment:5 by fingolfin, 12 years ago

What is the status of this item?

comment:6 by eriktorbjorn, 12 years ago

Unchanged, I guess.

comment:7 by fingolfin, 12 years ago

OK, let me rephrase: Robert, are you alive? *Ping* Any oppinion on this item?

comment:8 by SF/richiefs, 12 years ago

I have a proposition:
You could had a setting "music_balance" (editable only in broken sword) the range would be between 0 and 100.
With this setting, we would still use music_volume as before (editable in scummvm and broken sword).

When loading the settings:
the volume of the right channel=round(2*music_volume*(1-music_balance/100))
the volume of the left channel=2*music_volume - volume of the right channel

When saving the settings:
music_volume= round((volume of the right channel + volume of the left channel)/2)
music_balance= round(100*volume of the left channel/(volume of the right channel + volume of the left channel))

I might have done an error in the first formula, I'll check again.
I think it is the best solution, the advantage is that master volume can still be edited in scummvm gui, if you want I can try to code it.

comment:9 by SF/richiefs, 12 years ago

I'll try to do a patch this week if I manage to make scummvm compile under vc2005

comment:10 by SF/richiefs, 12 years ago

I did the patch that manages the balance for music, speech and sfx. Now subtitles setting is also saved in the ini file.
But I can't add any file to this tracker. How can I do it?

comment:11 by SF/richiefs, 12 years ago

Here is a link to the file (available 30 days):

http://dl.free.fr/i6oOPEQsr/sword1-balance.diff

Click on "Télécharger ce fichier"

by sev-, 12 years ago

Attachment: sword1-balance.diff added

Balance patch

comment:12 by sev-, 12 years ago

Attaching file, so it will not be lost.

Torbjorn, may you give some resolution on this patch?
File Added: sword1-balance.diff

comment:13 by eriktorbjorn, 12 years ago

The "balance" patch doesn't always save the volumes correctly, but I don't know if that's worth worrying about. I would like to hear lavosspawn's opinion. I consider him the Broken Sword 1 engine maintaner.

comment:14 by fingolfin, 12 years ago

I don't think we should wait for a reply by lavosspawn any longe, he has had half a year to react but didn't.

Torbjörn, when you say it doesn't save the volumes correctly: Is that an observation (if so: can you explain how to reproduce it?), or do you spot a bug in the code?

comment:15 by eriktorbjorn, 12 years ago

It was observation. For instance, if I increase the music volume to max, and then shift the balance so that the left speaker has full volume and the right one no volume at all. When I restart ScummVM, both speakers have half volume.

comment:16 by fingolfin, 12 years ago

So I took sword1-balance.diff and performed some cleanup. Does it still have the same problems? If so, please tell me how to best reproduce them :)
File Added: sword1-balance2.diff

by fingolfin, 12 years ago

Attachment: sword1-balance2.diff added

comment:17 by fingolfin, 12 years ago

Small modification to the patch: It now also saves the config values if the user does not return to the game from the config dialog, but rather chooses the quit button there. This seems to be the most natural way to handle this.

I think it's safe to apply this to the trunk; the only question is whether we should also apply it to the 0.11 branch. It ought to be safe, but of course I can't guarantee that.
File Added: sword1-balance2.diff

comment:18 by fingolfin, 12 years ago

Owner: changed from lavosspawn to sev-

comment:19 by SF/richiefs, 12 years ago

Nice cleanup

I tested it well, seems to be ok for me.

comment:20 by sev-, 12 years ago

It looks safe enough, but anyway I'd say, commit it just to the trunk, so we will not have any unexpected side effect.

comment:21 by sev-, 12 years ago

OK, I committed it to the trunk. Thanks a lot.

comment:22 by sev-, 12 years ago

Status: newclosed

comment:23 by digitall, 12 months ago

Component: Engine: Sword1
Game: Broken Sword 1
Note: See TracTickets for help on using tickets.