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

Ability to set a Global Volume #7706

Merged
merged 19 commits into from
Apr 10, 2023
Merged

Ability to set a Global Volume #7706

merged 19 commits into from
Apr 10, 2023

Conversation

LiamGallagher737
Copy link
Member

@LiamGallagher737 LiamGallagher737 commented Feb 16, 2023

Objective

Adds a new resource to control a global volume.
Fixes #7690


Solution

Added a new resource to control global volume, this is then multiplied with an audio sources volume to get the output volume, individual audio sources can opt out of this my enabling the absolute_volume field in PlaybackSettings.


Changelog

Added

  • GlobalVolume a resource to control global volume (in prelude).
  • global_volume field to AudioPlugin or setting the initial value of GlobalVolume.
  • Volume enum that can be Relative or Absolute.
  • VolumeLevel struct for defining a volume level.

@LiamGallagher737
Copy link
Member Author

This also opens the opportunity to fix the issue in the decodable audio example being very loud by adding .insert_resource(GlobalVolume::new(0.2)) to it. Tested it and it works.

crates/bevy_audio/src/audio_output.rs Outdated Show resolved Hide resolved
crates/bevy_audio/src/audio.rs Outdated Show resolved Hide resolved
crates/bevy_audio/src/audio.rs Outdated Show resolved Hide resolved
@alice-i-cecile alice-i-cecile added A-Audio Sounds playback and modification C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Feb 16, 2023
@alice-i-cecile alice-i-cecile added this to the 0.11 milestone Feb 16, 2023
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Clarity nit in the docs, but I won't block on it.

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Copy link
Member

@mockersf mockersf left a comment

Choose a reason for hiding this comment

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

Changing the global volume should change the sounds already playing that have been set relative to it.

Or as a start it should be mentioned that setting the global volume will only impact new sounds played after but not sounds already playing.

I think I would prefer an enum for the volume instead of having two fields, but I'm not completely sure on that.

@alice-i-cecile
Copy link
Member

Changing the global volume should change the sounds already playing that have been set relative to it.

Agreed, but this may not be trivial. Docs would be fine for me as a start.

I think I would prefer an enum for the volume instead of having two fields, but I'm not completely sure on that.

This is nice :)

@LiamGallagher737
Copy link
Member Author

LiamGallagher737 commented Feb 16, 2023

Enum for volume makes a lot of sense, I'll have a go at changing the volume of already playing sounds.
Would something like this work

pub enum Volume {
    Absolute(f32),
    Relative(f32),
}

@alice-i-cecile
Copy link
Member

That looks perfect. Ideally we could document what the f32 means, or use a more sensible numeric type too. Negative volumes don't exactly make sense to me.

@LiamGallagher737
Copy link
Member Author

Is there a way in rust to have positive only floats?

@alice-i-cecile
Copy link
Member

Is there a way in rust to have positive only floats?

I would newtype here and use a constructor method.

@LiamGallagher737
Copy link
Member Author

LiamGallagher737 commented Feb 17, 2023

I've added the volume enum but using the newtype for the volume level feels too verbose imo, some thoughts on this would be great. I also added the global volume as a field in the audio struct so you can set it using the .set() method on the plugin group.

@LiamGallagher737
Copy link
Member Author

Changing the global volume should change the sounds already playing that have been set relative to it.

Updating the volume for AudioSinks that have not been detached is easy, but I am unsure of how to update ones which have been detached (have no strong handle) so for now I will add message mentioning that only new sounds will be impacted.

Copy link
Contributor

@esensar esensar left a comment

Choose a reason for hiding this comment

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

Nice!
It is a bit more verbose, but it prevent missing the absolute option completely.

crates/bevy_audio/src/lib.rs Outdated Show resolved Hide resolved
crates/bevy_audio/src/audio.rs Show resolved Hide resolved
crates/bevy_audio/src/audio.rs Outdated Show resolved Hide resolved
@alice-i-cecile alice-i-cecile 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 Mar 2, 2023
@alice-i-cecile
Copy link
Member

I'm glad to see this added. Merging now, we can refine later as needed.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Mar 13, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 13, 2023
@alice-i-cecile alice-i-cecile requested a review from mockersf March 14, 2023 03:47
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Apr 10, 2023
Merged via the queue into bevyengine:main with commit dff071c Apr 10, 2023
@LiamGallagher737 LiamGallagher737 deleted the global-audio-volume branch April 11, 2023 00:19
github-merge-queue bot pushed a commit that referenced this pull request Jan 15, 2024
# 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.
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-Usability A targeted 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.

Set audio volume globally
5 participants