Skip to content

Move Volume and GlobalVolume to own file#16838

Merged
alice-i-cecile merged 1 commit intobevyengine:mainfrom
mgi388:volume-prework
Dec 16, 2024
Merged

Move Volume and GlobalVolume to own file#16838
alice-i-cecile merged 1 commit intobevyengine:mainfrom
mgi388:volume-prework

Conversation

@mgi388
Copy link
Copy Markdown
Contributor

@mgi388 mgi388 commented Dec 16, 2024 •
edited
Loading

Objective

Solution

  • Move the two types to volume.rs and made it compile.
  • Also #[reflect(Debug)] on Volume while I'm here.

Testing

  • Ran example locally.
  • Rely on CI.

@mgi388 mgi388 marked this pull request as ready for review December 16, 2024 06:26
@alice-i-cecile alice-i-cecile added A-Audio Sounds playback and modification C-Code-Quality A section of code that is hard to understand or change S-Needs-Review Needs reviewer attention (from anyone!) to move forward D-Trivial Nice and easy! A great choice to get started with Bevy labels Dec 16, 2024
alice-i-cecile
alice-i-cecile approved these changes Dec 16, 2024
View reviewed changes
Copy link
Copy Markdown
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.

Yes please :)

@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Dec 16, 2024
SolarLiner
SolarLiner approved these changes Dec 16, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@SolarLiner SolarLiner left a comment

Choose a reason for hiding this comment

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

We should put a pin on improving the documentation of the Volume newtype, because it's unclear from the doc comments which scale we are using (linear can be inferred from the debug_assert!, but it would be nice to make it explicit).

Additionally, we should add a second constructor for setting the volume in decibels.

@mgi388
Copy link
Copy Markdown
Contributor Author

mgi388 commented Dec 16, 2024

We should put a pin on improving the documentation of the Volume newtype, because it's unclear from the doc comments which scale we are using (linear can be inferred from the debug_assert!, but it would be nice to make it explicit).

Additionally, we should add a second constructor for setting the volume in decibels.

Absolutely. We can look at these in a follow up PR since this PR is just a simple move around. FWIW, I also noticed this from a user perspective. The constructor just says Create a new volume level. which leaves the user wondering :)

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Dec 16, 2024
Merged via the queue into bevyengine:main with commit 74e793d Dec 16, 2024
@mgi388 mgi388 deleted the volume-prework branch December 17, 2024 01:42
ecoskey pushed a commit to ecoskey/bevy that referenced this pull request Jan 6, 2025
@mgi388 @ecoskey
Move Volume and GlobalVolume to own file (bevyengine#16838)
7cef71a 
# Objective

- Prework for reviving bevyengine#9582.

## Solution

- Move the two types to volume.rs and made it compile.
- Also `#[reflect(Debug)]` on `Volume` while I'm here. 

## Testing

- Ran example locally.
- Rely on CI.
mrchantey pushed a commit to mrchantey/bevy that referenced this pull request Feb 4, 2025
@mgi388 @mrchantey
Move Volume and GlobalVolume to own file (bevyengine#16838)
5458265 
# Objective

- Prework for reviving bevyengine#9582.

## Solution

- Move the two types to volume.rs and made it compile.
- Also `#[reflect(Debug)]` on `Volume` while I'm here. 

## Testing

- Ran example locally.
- Rely on CI.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

+2 more reviewers

@SolarLiner SolarLiner SolarLiner approved these changes

@mnmaita mnmaita mnmaita approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A-Audio Sounds playback and modification C-Code-Quality A section of code that is hard to understand or change D-Trivial Nice and easy! A great choice to get started with Bevy S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it

Projects

No open projects
Audio (Old)
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@mgi388 @alice-i-cecile @SolarLiner @mnmaita