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

Remove the ability to ignore global volume #11092

Merged
merged 2 commits into from
Jan 15, 2024

Conversation

tim-blackbird
Copy link
Contributor

@tim-blackbird tim-blackbird commented Dec 26, 2023

Objective

The ability to ignore the global volume doesn't seem desirable and complicates the API.

#7706 added global volume and the ability to ignore it, but there was no further discussion about whether that's useful. Feel free to discuss here :)

Solution

Replace the Volume type's functionality with the VolumeLevel. Remove VolumeLevel.

I also removed DerefMut derive that effectively made the volume pub and actually ensured that the volume isn't set below 0 even in release builds.

Migration Guide

The option to ignore the global volume using Volume::Absolute has been removed and Volume now stores the volume level directly, removing the need for the VolumeLevel struct.

@Nilirad Nilirad added A-Audio Sounds playback and modification C-Usability A simple quality-of-life change that makes Bevy easier to use labels Dec 26, 2023
@Bcompartment
Copy link

IMO, it might be worth leaving the option to play sound without a volume filter (direct sound output).

  1. How removing the ability to ignore global volume improves the user experience or adds functionality ?
  2. What are the alternatives to your solution ?

@matiqo15 matiqo15 added the C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide label Dec 27, 2023
@tim-blackbird
Copy link
Contributor Author

IMO, it might be worth leaving the option to play sound without a volume filter (direct sound output).

Opting out of other kinds of filters makes sense to me, but not volume control.

How removing the ability to ignore global volume improves the user experience or adds functionality ?

It doesn't, but I don't see any reason you would want to ignore the volume setting. Not even a niche one.
The goal here is improving the API and removing cruft.

What are the alternatives to your solution ?

Keep the option but as a separate absolute_volume bool on PlaybackSettings.

Copy link
Member

@matiqo15 matiqo15 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also can't find any case that would need to use absolute volume.

But it could be useful to implement (in another PR) an option for more global volumes (probably would need another name) so that we could have "volume categories" for things like sound effects, background music, etc. If someone had any good reason for absolute volume, this would make it possible to achieve the same effect in a different way.

@Ixentus
Copy link
Contributor

Ixentus commented Dec 27, 2023

Removing this makes sense to me. I've never seen audio software where you can ignore the global volume (usually called Master Volume).

If you need functionality like this, it's simple to make. Just set a group volume and multiply it by the entities volume.

Copy link
Contributor

@atlv24 atlv24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

full agreement with this

@matiqo15 matiqo15 added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jan 15, 2024
@alice-i-cecile
Copy link
Member

I'm convinced too. This isn't useful enough to complicate a core bit of the API, even if I do love enums.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jan 15, 2024
Merged via the queue into bevyengine:main with commit c29a972 Jan 15, 2024
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Audio Sounds playback and modification C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide C-Usability A simple quality-of-life change that makes Bevy easier to use S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

7 participants