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

[Merged by Bors] - Complete inline documentation for bevy_audio #3510

Closed
wants to merge 4 commits into from

Conversation

NiklasEi
Copy link
Member

@NiklasEi NiklasEi commented Jan 1, 2022

Objective

Part of #3492

  • Complete inline documentation of bevy_audio

Solution

  • Added inline documentation to all public parts of bevy_audio
  • Added a few inline examples at important places
  • Some renaming for clarity (e.g. AudioLoader and generics)
  • added #![warn(missing_docs)] and #![forbid(unsafe_code)] to bevy_audio

I also tried adding support for the other vorbis file endings .oga and .spx to the AudioLoader (see file endings at https://tools.ietf.org/html/rfc5334#section-10.3), but the rodio decoder does not seem to support those.

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Jan 1, 2022
@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 C-Docs An addition or correction to our documentation and removed S-Needs-Triage This issue needs to be labelled labels Jan 1, 2022
@NiklasEi NiklasEi removed the C-Code-Quality A section of code that is hard to understand or change label Jan 1, 2022
@@ -43,9 +50,12 @@ impl AssetLoader for Mp3Loader {
}
}

/// Mark a type as decodable
Copy link
Member

Choose a reason for hiding this comment

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

This could use more information about what exactly it means for something to be "decodable".

Many of the readers will have 0 audio experience.

Copy link
Member Author

Choose a reason for hiding this comment

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

I find it very hard to specify because this trait is so generic. There is no restriction on the Decoder type in the trait.
The restrictions come later in the implementations of AudioOutput and Audio. There we require the Decoder to yield some rodio types.

Copy link
Member Author

@NiklasEi NiklasEi Jan 1, 2022

Choose a reason for hiding this comment

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

I moved the trait bounds from the implementations to the decodable trait. Now it should be a bit clearer what the trait means and we have a lot less duplication in where clauses.

This is not the same API, but I couldn't think of a use case where it would be a problem to have the rodio trait bounds on the Decodable trait directly.

What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

This will lead to a little bit of generic-pollution, but the boilerplate reduction and clarity are worth it here IMO.

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.

Awesome, this is looking good! I particularly like #[forbid(unsafe)]: we should have that pretty much everywhere.

A couple questions and nits for you.

@NiklasEi NiklasEi added the C-Code-Quality A section of code that is hard to understand or change label Jan 1, 2022
pub bytes: Arc<[u8]>,
bytes: Arc<[u8]>,
Copy link
Member

Choose a reason for hiding this comment

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

why removing the pub? doesn't it allow someone to generate an audio source instead of loading it from a file?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. I made it public again.

@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 Jan 5, 2022
@alice-i-cecile
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Jan 5, 2022
# Objective
Part of #3492 

- Complete inline documentation of `bevy_audio`

## Solution

- Added inline documentation to all public parts of `bevy_audio`
- Added a few inline examples at important places
- Some renaming for clarity (e.g. `AudioLoader` and generics)
- added `#![warn(missing_docs)]` and `#![forbid(unsafe_code)]` to `bevy_audio`

I also tried adding support for the other vorbis file endings `.oga` and `.spx` to the `AudioLoader` (see `file endings` at https://tools.ietf.org/html/rfc5334#section-10.3), but the `rodio` decoder does not seem to support those.
@bors bors bot changed the title Complete inline documentation for bevy_audio [Merged by Bors] - Complete inline documentation for bevy_audio Jan 5, 2022
@bors bors bot closed this Jan 5, 2022
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-Code-Quality A section of code that is hard to understand or change C-Docs An addition or correction to our documentation 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