Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

New mixer mute handling. #12

merged 6 commits into from Apr 18, 2011


None yet
2 participants

lordhoto commented Apr 13, 2011

This is a proof-of-concept implementation for a different muting implementation than we use currently.

Instead of setting the sound volume of all sound types to 0 in case the global mute setting is checked, I added mute flags for the individual sound types, so they can be muted separately and the volume setting is still kept as is.

This allows the engine code to be a bit more agnostic about mute settings, since it can still change the sound volume (like script wise or similary) without unmuting the sound that way. This fixes Kyra's audio dialog, which used the sound settings from config manager and thus always had a volume set and by changing it, it would've umuted the specific sound type and thus confuse the user.

Furthermore it makes the Sword2 options dialog a bit more usable, since it saved the sound volume settings when it was closed via "OK" while the sound was muted, thus even after pressing Ctrl+u again the sound would still be muted. This is especially annoying when one wanted to change the subtitle settings via Sword2's dialog while the sound is muted, since it destroyed the user's sound volume settings.

I am not sure whether we really want to have special mute flags inside the Mixer though, thus I made this pull-request as a proof-of-concept implementation for further discussion on how to improve our mute handling.

lordhoto added some commits Mar 20, 2011

AUDIO: Add per sound type mute flag setting to Mixer(Impl).
This also adapts our default implementation MixerImpl to handle the newly
added flags properly.

Now we do not need to set the sound volume for all types to 0, in case we want
to mute them, but instead just set the mute flag for all types to true. This
allows engines to be a bit more agonstic about mute support, when it comes to
volume options etc. since they can just setup any volume they like, but are
still muted (and thus will not break muting anymore).

MIDI sound is of course not affected by this.
ENGINES: Make Engine::syncSoundSettings use the Mixer's mute flag dir…

This fixes an annoying behavior in the Sword2 option's dialog, which set all
sound type volumes to 0, in case it was opened when the user used the global
mute setting.

fingolfin commented Apr 14, 2011

Well, are there any disadvantages to having that mute flag?


lordhoto commented Apr 14, 2011

Well not much, might be a bit more code currently and of course all possible Mixer implementations would have to implement that, but seeing all backends are using our default one currently that shouldn't matter much I guess.

@fingolfin fingolfin commented on an outdated diff Apr 15, 2011

@@ -97,6 +98,9 @@ public:
virtual bool isSoundHandleActive(SoundHandle handle);
+ virtual void setMuteForSoundType(SoundType type, bool mute);
+ virtual bool getMuteForSoundType(SoundType type) const;

fingolfin Apr 15, 2011


These method names sound very awkward to me. How about muteSoundType and isSoundTypeMuted ? The latter would match with e.g. isSoundHandleActive() a few lines above :)

@fingolfin fingolfin commented on an outdated diff Apr 15, 2011

@@ -64,6 +64,7 @@ private:
bool _mixerReady;
uint32 _handleSeed;
+ bool _mute[4];
int _volumeForSoundType[4];

fingolfin Apr 15, 2011


_volumeForSoundType is a bit verbose, but at least says what it is; while for _mute, it is not immediately clear that this is a per-soundtype mute status.
How about putting these together into a single struct, like this

struct SoundTypeConfig {
    bool _mute;
    int _volume;
SoundTypeConfig  _soundTypes[4];

lordhoto added some commits Apr 16, 2011

AUDIO: Renamed mute related functions in Mixer.
This renames setMuteForSoundType to muteSoundType and getMuteForSoundType to

lordhoto commented Apr 16, 2011

Ok I made the changes you proposed now, I named it "SoundTypeSettings" instead of "SoundTypeConfig" though.


fingolfin commented Apr 18, 2011

That's a better name, yeah!

I like the patch, and would recommend it for merging.

@lordhoto lordhoto merged commit 1d60b26 into scummvm:master Apr 18, 2011


lordhoto commented Apr 18, 2011

Ok I pulled it into master now. Thanks for reviewing it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment