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

Added Pitch as an alternative sound source #9225

Merged
merged 10 commits into from
Jul 29, 2023

Conversation

basilefff
Copy link
Contributor

@basilefff basilefff commented Jul 21, 2023

Objective

My attempt at implementing #7515

Solution

Added struct Pitch and implemented on it Source trait.

Changelog

Added

  • File pitch.rs to bevy_audio crate
  • Struct Pitch and type aliases for AudioSourceBundle<Pitch> and SpatialAudioSourceBundle<Pitch>
  • New example showing how to use PitchBundle

Changed

  • AudioPlugin now adds system for Pitch audio

@github-actions
Copy link
Contributor

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@basilefff basilefff changed the title added Note and NoteBundle to bevy_audio; changed breakout to use it Added Note as an alternative sound source Jul 21, 2023
@mockersf
Copy link
Member

mockersf commented Jul 21, 2023

Looks good!

Could you find another name than Note? With Note, I would expect to give something like Dsharp instead of 311.127.

For the example, could you add a dedicated example in the Audio category instead of changing the breakout one?

@mockersf mockersf added C-Feature A new feature, making something new possible A-Audio Sounds playback and modification labels Jul 21, 2023
@basilefff
Copy link
Contributor Author

Could you find another name than Note? With Note, I would expect to give something like Dsharp instead of 311.127.

Sure, I guess the correct name would be pure tone, so maybe Tone?

For the example, could you add a dedicated example in the Audio category instead of changing the breakout one?

Of course, I guess I jumped the gun a little bit because of this comment

@mockersf
Copy link
Member

Sure, I guess the correct name would be pure tone, so maybe Tone?

I think Pitch would be the correct name

@basilefff basilefff changed the title Added Note as an alternative sound source Added Pitch as an alternative sound source Jul 21, 2023
@github-actions
Copy link
Contributor

You added a new example but didn't update the readme. Please run cargo run -p build-templated-pages -- update examples to update it, and commit the file change.

@basilefff
Copy link
Contributor Author

I changed the name and added example, but I don't know how to fix the failing check. Could you help me with that, please?

@mockersf
Copy link
Member

The dependencies/check-bans failure is not because of your PR, and it's not required so it can be ignore.

I would love for the example to be a little more interactive. For example react to key press to play a sound, and displaying a text like "press to play a sound". If you're filling more adventurous you could even display the current frequency, and change it up/down on arrow up/down press.

@basilefff
Copy link
Contributor Author

So, something like this with more comments? I think it overcomplicates the example

@mockersf
Copy link
Member

I think it's great, thanks! if you run cargo fmt it should be good to go

examples/README.md Outdated Show resolved Hide resolved
@alice-i-cecile
Copy link
Member

Doc suggestions are just nits to help users who aren't familiar with the terminology. No objections to merging this as is though.

@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 Jul 25, 2023
basilefff and others added 2 commits July 29, 2023 19:21
Changed description with clarification of what pitch is

Co-authored-by: Alice Cecile <[email protected]>
@mockersf mockersf added this pull request to the merge queue Jul 29, 2023
Merged via the queue into bevyengine:main with commit fb9c5a6 Jul 29, 2023
23 of 24 checks passed
@basilefff basilefff deleted the note-bundle branch July 30, 2023 13:46
@cart cart mentioned this pull request Oct 13, 2023
43 tasks
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-Feature A new feature, making something new possible 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.

3 participants