Skip to content
This repository has been archived by the owner on Jun 23, 2023. It is now read-only.

Adjust music volume slider to match vanilla Doom #531

Merged
merged 1 commit into from
Sep 16, 2022
Merged

Adjust music volume slider to match vanilla Doom #531

merged 1 commit into from
Sep 16, 2022

Conversation

ceski-1
Copy link
Contributor

@ceski-1 ceski-1 commented Sep 16, 2022

This is similar to chocolate-doom/chocolate-doom#1508 but adjustments are made to the master volume instead of individual channels to achieve a similar effect.

@fabiangreffrath fabiangreffrath merged commit db1928a into coelckers:master Sep 16, 2022
@kraflab
Copy link
Collaborator

kraflab commented Sep 16, 2022

I'm a bit confused by this patch. Doesn't it make 13, 14, and 15 all have the same volume? And 12 is closer to 13 than to 11?

@ceski-1
Copy link
Contributor Author

ceski-1 commented Sep 16, 2022

Correct, this is how vanilla Doom behaved for every channel except 10 (drums). But in that instance it's a trade-off between slightly wider dynamic range and slightly louder volume.

@rfomin
Copy link
Contributor

rfomin commented Sep 16, 2022

I'm a bit confused by this patch. Doesn't it make 13, 14, and 15 all have the same volume? And 12 is closer to 13 than to 11?

Volume controls should be non-linear: https://www.dr-lex.be/info-stuff/volumecontrols.html

@kraflab
Copy link
Collaborator

kraflab commented Sep 16, 2022

The volume control is linear though, there's just one notch that isn't, and the rest are 0. How exactly is this a fix?

If we want non-linear scaling, then why aren't we doing that? We can make each value translate to whatever we want, and having multiple numbers that don't mean any difference in the options is an unacceptable user experience.

@rfomin
Copy link
Contributor

rfomin commented Sep 16, 2022

If we want non-linear scaling, then why aren't we doing that?

I tried that, but it changes relative volume of instruments and some songs sounds different.

How exactly is this a fix?

This is how vanilla DOS exe works. I think it fits Choco/Crispy/Woof, not sure about PrBoom+.

@kraflab
Copy link
Collaborator

kraflab commented Sep 16, 2022

Sure, it makes total sense in ports that are sticking closer to the source material.

@ceski-1
Copy link
Contributor Author

ceski-1 commented Sep 16, 2022

When a new user launches a clean install of Pr+ the music volume will match what they would expect from vanilla Doom. It helps reduce questions regarding inconsistent volume levels across different source ports if they closely resemble the original game. I guess those would be the two strongest arguments.

@ceski-1
Copy link
Contributor Author

ceski-1 commented Sep 16, 2022

It wouldn't be too difficult to use a non-linear curve that addresses your concerns though. Since master volume is a 14-bit value, it may have better results than before.

@rfomin
Copy link
Contributor

rfomin commented Sep 16, 2022

Since master volume is a 14-bit value, it may have better results than before.

Yes, I think we should change master volume.

I am changing the channels volume in i_winmusic.c to work around a bug in the Windows mixer. But since DOS exe does the same thing, maybe it's suitable for "conservative" ports.

@kraflab
Copy link
Collaborator

kraflab commented Sep 16, 2022

When a new user launches a clean install of Pr+ the music volume will match what they would expect from vanilla Doom

This is neither as common as you would think nor the target for this port. It doesn't look like vanilla doom either.

@kraflab
Copy link
Collaborator

kraflab commented Sep 16, 2022

It wouldn't be too difficult to use a non-linear curve that addresses your concerns though. Since master volume is a 14-bit value, it may have better results than before.

I'm all for improving the audio experience while still having a good user experience in terms of meeting expectations. All that being said, I'm actually not convinced at all that any change like this should be made in prboom+. While this isn't a "new feature" exactly, it does change the volume of music. It feels weird to me that someone might update a port in maintenance mode and find they need to reconfigure something.

@rfomin
Copy link
Contributor

rfomin commented Sep 16, 2022

While this isn't a "new feature" exactly, it does change the volume of music. It feels weird to me that someone might update a port in maintenance mode and find they need to reconfigure something.

I think only MIDI enthusiasts will notice this feature. MIDI SysEx messages PR already merged, it seems odd not to use master volume SysEx messages.

@ceski-1
Copy link
Contributor Author

ceski-1 commented Sep 16, 2022

Here's my thought process: Most users are on a Windows OS. They launch PrBoom+ and then FluidSynth can't find TimGM6mb.sf2 so OPL2 is automatically selected as the fallback music system. Some users don't care, others edit the .cfg to point to a valid soundfont or switch to PortMidi and are happy with the default MS GS Wavetable Synth. Users looking for an authentic experience are using a different source port. Now we're left with a tiny subset of users: those who prefer a VSTi (e.g. Sound Canvas VA), real hardware (e.g. SC-55), or VirtualMIDISynth (for soundfonts optimized for it or for better implemented reverb/chorus over FluidSynth). They all benefit from these volume changes.

With that in mind, it's important to consider the biggest group of users who would notice a difference: those who selected PortMidi, left it at MS GS Wavetable Synth, but then didn't change the volume slider or only bumped it up a couple steps. But if they had the volume at max, there would be no difference at all.

Here it is visualized:
chart

So the volume of the music does change, but not the overall max volume. A side effect is the lower steps of the volume slider are a little more useful now.

If the concern is any change in volume at all, I guess I don't understand the objection. For example, adding SysEx support enabled the use of master volume. This change means it's no longer necessary to scale the individual channels using large steps (0-127) which had altered the channel volume balance, especially at lower volumes.

My impression is that PrBoom+ is a good compromise between feeling like vanilla Doom in many ways but having quality of life changes plus additional features like Boom support and great demo compatibility. The suggested volume line in the image above tries to stick to this philosophy without stepping outside the boundaries of a source port in maintenance mode. If it seems like this leans too far in the out of scope direction, I'm fine with rolling back the changes. Thanks for reviewing, either way.

@kraflab
Copy link
Collaborator

kraflab commented Sep 16, 2022

A side effect is the lower steps of the volume slider are a little more useful now

It looks like the opposite to me - the jumps are larger, which means less precision, which means you have less control over the volume than you did before. So if you do want low volume, you have fewer options now. I don't use portmidi though, maybe there is just a fundamental misunderstanding here. Anyway, this is all just my opinion - I certainly don't own the port.

@ceski-1
Copy link
Contributor Author

ceski-1 commented Sep 16, 2022

It looks like the opposite to me - the jumps are larger, which means less precision, which means you have less control over the volume than you did before. So if you do want low volume, you have fewer options now.

If you try adjusting the slider before and after you'll notice that the lower steps are essentially silent before the changes. After the changes there's about one or two extra steps where the music is audible. So the volume slider effectively has more range.

@ceski-1
Copy link
Contributor Author

ceski-1 commented Sep 16, 2022

Anyway, this is all just my opinion - I certainly don't own the port.

I value your opinion because it's hard to get good constructive feedback sometimes. I make many of these PRs with the hope that you'll add them to dsda-doom but also with the understanding that some changes don't make sense. For example, the reverb/chorus setting isn't really necessary and I'm glad you ripped out mus_extend_volume.

@kraflab
Copy link
Collaborator

kraflab commented Sep 16, 2022

the lower steps are essentially silent before the changes

Alright, then I understand this a bit better. The "suggested" curve looks the best to me but if it's not an important change then I'll leave it up to the wider consensus.

the reverb/chorus setting

I haven't looked at that one yet (last check was aug 21). I adopted the sysex stuff early to avoid conflicts 😸

@rfomin
Copy link
Contributor

rfomin commented Sep 17, 2022

@ceski-1 Thanks for the detailed posts.

if it's not an important change then I'll leave it up to the wider consensus.

Linear volume controls seem broken (lower values are not audible, higher values are perceived as equal).
By the way, I think we should use Fluidsynth's "gain" setting to control the volume in flplayer.c, this will give us a non-linear volume control for Fluidsynth (as in SDL_Mixer and Woof).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants