Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add volume_linear property and getter/setter methods for audio-related classes #99268

Merged
merged 1 commit into from
Dec 20, 2024

Conversation

Meorge
Copy link
Contributor

@Meorge Meorge commented Nov 15, 2024

This PR implements the backend side for godotengine/godot-proposals#1884 (but does not fully close it).

  • It adds the property volume_linear to AudioStreamPlayer, AudioStreamPlayer2D, AudioStreamPlayer3D, and AudioEffectAmplify (including the setter set_volume_linear and getter get_volume_linear).
  • It adds the property volume_linear for every bus within an AudioBusLayout.
  • It adds the getter get_bus_volume_linear and setter set_bus_volume_linear to AudioServer. (Because it requires an argument for the bus index, it can't be made into a property.)

Internally, it simply wraps the volume_db properties with linear_to_db and db_to_linear calls. However, having these as properties with built-in getters and setters has some nice benefits:

  • A volume slider can be constructed easily and without having to write any code, by hooking the Range.value_changed signal up to set_volume_linear.
  • My personal favorite: a Tween can be used to smoothly fade music or sounds in and out with an AudioStreamPlayer:
func _fade_with_volume_linear():
    var tween = create_tween()

    # Fade music out over 2 seconds
    tween.tween_property(%MusicPlayer, "volume_linear", 0.0, 2.0)
    tween.tween_interval(3.0)
    # Fade music in over 2 seconds
    tween.tween_property(%MusicPlayer, "volume_linear", 1.0, 2.0)


func _fade_without_volume_linear():
    var tween = create_tween()

    # Fade music out over 2 seconds
    tween.tween_method(func(v): %MusicPlayer.volume_db = linear_to_db(v), db_to_linear(%MusicPlayer.volume_db), 0.0, 2.0)
    tween.tween_interval(3.0)
    # Fade music in over 2 seconds
    # (Note that the `from` argument is likely to catch people off-guard!)
    tween.tween_method(func(v): %MusicPlayer.volume_db = linear_to_db(v), 0.0, 1.0, 2.0)

Note

This PR does not implement any frontend/inspector behavior. Users will still only be able to modify the volume in the inspector in dBs. If desired, it would be trivial to expose "Volume Linear" underneath "Volume dB" in the inspector. However this would take up twice as much space in the inspector for what is effectively the same value.
In the long run, I think it'd be best to include a custom input field that can switch between dB and the linear scale, as suggested in the proposal. Because this PR doesn't implement this (currently), I don't feel it would close that proposal.

@Mickeon
Copy link
Contributor

Mickeon commented Nov 15, 2024

I want to note, volume_linear as a concept has been talked about on-and-off, but no one really made a PR about it until now.
One simple argument in favour of adding this new property is the fact that rotation_degrees exists. That's pretty much it, but it's a very strong one because users are expecting this kind of property to exist.
In fact, back in 4.0's beta, rotation_degrees was removed in favour of rotation itself displaying as degrees in the Inspector. The property was later restored due to backlash.
However, it's worth keeping in mind that was a whole different case. The code workaround was simple, but script readability and intuitiveness suffered drastically for being such a common operation. A property that already existed was removed and re-added for being too useful.

So my question is, do users need this commonly enough?

@AdriaandeJongh
Copy link
Contributor

I have a lot of silly code in my commercial game project that wraps volume_db so I can control it in linear volume space. So yes, this would be a fantastic addition!

@Meorge
Copy link
Contributor Author

Meorge commented Nov 15, 2024

The two big use cases I see for volume_linear personally are fading music in or out with tweens (a pretty standard operation when you have music) and volume sliders (a standard setting in games these days).

I really love the power of the tweening engine, and I think it could work very well for fading music in and out, but as it is right now with only volume_db as a property to access, the code is a mess as described in the original PR post. While it's not as extreme as the tweening case, the user-code overhead for a volume slider feels a bit unnecessary.

Personally, I don't think I can recall a single time where I was writing code to control volume, and actually wanted to work in decibels; the tasks I wanted to accomplish always necessitated converting to and/or from a linear scale anyways. My sentiment on this is very close to what aXu-AP said here - behavior like setting and adjusting a volume percentage feels like something common enough that we shouldn't have to add in a snippet of code to get it on each individual project.

I've asked around a little bit for others' experiences on this, and have gotten varied responses: some have said they always use a linear scale and don't know why a decibel property would even be exposed, others said that having both would be the best option, and others yet said they hated linear scale. Most of the pushback I saw to the linear scale seemed to be more in the context of sound effects and dialogue, rather than music or overall audio, which is where I think the linear property would shine the most.

As for the general sentiment towards a feature like this: the original proposal post has (at the time of writing this) 18 👍 reactions and only one 👎 reaction. Replies to the proposal supporting it have consistently more positive reactions (22ish) than replies that were dismissive of it (3ish). I'd generally point to the proposal thread for peoples' discussions and opinions on adding such a feature.

@Meorge
Copy link
Contributor Author

Meorge commented Nov 18, 2024

It did occur to me that position and global_position are another pair of properties that ultimately modify the same underlying information (where the node is in the world) but within different contexts, so that the appropriate one for a given task can be used.


Oops, looks like the volume_linear property is being saved to scene files (I opened this project with the feature branch, then again with the master branch, and just noticed it removed this line).

CleanShot 2024-11-17 at 18 00 33@2x

That definitely doesn't make sense 😅 Will have to take a look at that and how to fix it in the future before a potential merge.

@KoBeWi
Copy link
Member

KoBeWi commented Nov 18, 2024

It's because of PROPERTY_USAGE_NO_EDIT.
If you want the property to be non-serializable and not editable in the inspector, you should use PROPERTY_USAGE_NONE.

@Meorge
Copy link
Contributor Author

Meorge commented Nov 19, 2024

PROPERTY_USAGE_NO_EDIT has been changed to PROPERTY_USAGE_NONE to avoid the serialization :)

Remove default value from `volume_linear` property documentation

Remove `volume_linear` internal property from `AudioBusLayout`

Update doc/classes/AudioEffectAmplify.xml [no ci]

Co-authored-by: A Thousand Ships <96648715+AThousandShips@users.noreply.github.com>

Make documentation more concise [no ci]
@Repiteo Repiteo modified the milestones: 4.x, 4.4 Dec 20, 2024
@Repiteo Repiteo merged commit 37d1e7f into godotengine:master Dec 20, 2024
@Repiteo
Copy link
Contributor

Repiteo commented Dec 20, 2024

Thanks!

@Meorge Meorge deleted the add-volume-linear branch February 3, 2025 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants