Opened 12 years ago

Closed 11 years ago

Last modified 8 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


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 11 years ago.

Download all attachments as: .zip

Change History (26)

Changed 12 years ago by eriktorbjorn

Attachment: sword1-options.diff added

Patch against current SVN

comment:1 Changed 12 years ago by eriktorbjorn

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

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

Robert, any oppinion on this one?

comment:4 Changed 12 years ago by fingolfin

Owner: set to lavosspawn

comment:5 Changed 12 years ago by fingolfin

What is the status of this item?

comment:6 Changed 12 years ago by eriktorbjorn

Unchanged, I guess.

comment:7 Changed 12 years ago by fingolfin

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

comment:8 Changed 12 years ago by SF/richiefs

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 Changed 12 years ago by SF/richiefs

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

comment:10 Changed 12 years ago by SF/richiefs

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 Changed 12 years ago by SF/richiefs

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

Click on "Télécharger ce fichier"

Changed 12 years ago by sev-

Attachment: sword1-balance.diff added

Balance patch

comment:12 Changed 12 years ago by sev-

Attaching file, so it will not be lost.

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

comment:13 Changed 12 years ago by eriktorbjorn

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

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 Changed 11 years ago by eriktorbjorn

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

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

Changed 11 years ago by fingolfin

Attachment: sword1-balance2.diff added

comment:17 Changed 11 years ago by fingolfin

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

Owner: changed from lavosspawn to sev-

comment:19 Changed 11 years ago by SF/richiefs

Nice cleanup

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

comment:20 Changed 11 years ago by sev-

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

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

comment:22 Changed 11 years ago by sev-

Status: newclosed

comment:23 Changed 8 months ago by digitall

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