Add Audio support #265

Xaeroxe · Jul 23, 2017

Things done in this PR:

Add rodio dependency
Add amethyst_assets dependency to amethyst
Add modules:
- audio
  - dj
    - pub struct Dj
  - endpoint
    - pub struct Endpoint
    - pub fn default_endpoint()
    - pub fn endpoints()
  - play
    - pub fn once(souce: &Source, endpoint: &Endpoint)
  - source
    - pub struct Source, implements amethyst_assets::Asset
    - pub(crate) struct AudioDataReference
    - pub struct WavFormat, implements amethyst_assets::Format
    - pub struct OggFormat, implements amethyst_assets::Format
    - pub struct FlacFormat, implements amethyst_assets::Format
Add AudioSystem which manages position updating for 3D sounds.
Add AudioEmitter component for playing sounds
Add AudioListener component for listening to those sounds.
Make pong example have sfx.

Linux developers: the libasound2-dev package will be required to compile amethyst once this package is merged, rodio interfaces with alsa which makes this necessary.

Xaeroxe · Jul 23, 2017

Linux developers: the libasound2-dev package will be required to compile amethyst once this package is merged, rodio interfaces with alsa which makes this necessary.

@Xaeroxe

Linux developers: the libasound2-dev package will be required to compile amethyst once this package is merged, rodio interfaces with alsa which makes this necessary.

Add information about it in readme, please

vitvakatu · Jul 23, 2017

@Xaeroxe

Linux developers: the libasound2-dev package will be required to compile amethyst once this package is merged, rodio interfaces with alsa which makes this necessary.

Add information about it in readme, please

torkleyy · Jul 24, 2017

torkleyy reviewed Jul 24, 2017

View changes

torkleyy · Jul 24, 2017

Cargo.toml

 ticketed_lock = "0.1"
 wavefront_obj = "5.0"
 
 thread_profiler = { version = "0.1", optional = true }
 
+[dev_dependencies]


I thought it was dev-dependencies?

I guess it is. Woops. For some reason it still worked when I added this but I'll fix it.

torkleyy · Jul 24, 2017

src/audio/endpoint.rs

+use rodio::*;
+
+/// A speaker(s) through which audio can be played.
+pub struct Endpoint {


Is there a reason for this wrapper?

Mostly just making it so users of the API don't also have to depend on rodio.

Could we give it another name? I think something like Output or OutputDevice is probably clearer to people who didn't deal with cpal stuff before.

I agree, either Output or Sink are good name choices IMO.

torkleyy · Jul 24, 2017

src/audio/mod.rs

+/// Provides structures used to load audio files.
+pub mod source;
+
+/// Provides functions used to play audio.


Would prefer to have module docs in the actual module with //!.

@Xaeroxe Will this pull request eventually include an AudioSystem and components for audio listeners and emitters?

ebkalderon · Jul 27, 2017

@Xaeroxe Will this pull request eventually include an AudioSystem and components for audio listeners and emitters?

@ebkalderon Yes it will, I'm not going to be able to work on this for a few days as my life is a little busy recently but I'll get back to this PR as soon as I can

Xaeroxe · Jul 27, 2017

@ebkalderon Yes it will, I'm not going to be able to work on this for a few days as my life is a little busy recently but I'll get back to this PR as soon as I can

torkleyy · Jul 27, 2017

torkleyy reviewed Jul 27, 2017

View changes

torkleyy · Jul 27, 2017

src/audio/source.rs

+}
+
+/// Loads audio from wav files.
+pub struct WavFormat;


Could we have a module assets which reexports * from amethyst_assets and add the amethyst-specific functionality in there similar to how we do it with amethyst::ecs?

So like

// src/assets/mod.rs use amethyst_assets::*; pub mod formats;

and then keep the formats in the formats module.

But I'm also not sure whether or not formats should belong to amethyst_assets.

@torkleyy I'd prefer to keep the formats in Amethyst. I also like your first proposal, @ebkalderon Does this sound good to you? Bunching up all asset formats into assets::formats would help with asset type discoverability.

Yes, keeping the assets in a formats module sounds good to me. 👍

It may be worth dropping the Format suffix if all the format types are put into one module.

I think this PR is worth a line in the CHANGELOG.md.

torkleyy · Aug 6, 2017

I think this PR is worth a line in the CHANGELOG.md.

torkleyy · Aug 15, 2017

torkleyy reviewed Aug 15, 2017

View changes

Really nice PR!

torkleyy · Aug 15, 2017

CHANGELOG.md

@@ -10,12 +10,14 @@ The format is based on [Keep a Changelog][kc], and this project adheres to
 ## [Unreleased]
 ### Changed
 * Asset management rewrite (pull request [#244]).
+* Add audio support ([#265])


Should be in ## Added section

torkleyy · Aug 15, 2017

examples/06_audio/assets/config.yml

@@ -0,0 +1,10 @@
+# ---


Should be a RON file.

Woops, yah sorry copy paste error.

torkleyy · Aug 15, 2017

src/assets/mod.rs

+
+pub use amethyst_assets::*;
+
+pub mod formats;


Nice, exactly the structure I would have used.

torkleyy · Aug 15, 2017

src/audio/audio_context.rs

+
+/// A context for loading audio files
+pub struct AudioContext {
+    inner: SimpleContext<Source, Vec<u8>, NoError, fn(Vec<u8>) -> Result<Source, NoError>>,


Not sure about that dynamic dispatch here..I think we should replace that. Pretty sure a change in amethyst_assets is required.

@torkleyy Just to clarify: Dynamic dispatch usually refers to a vtable lookup occurring, this doesn't involve a vtable lookup though, it just uses a hard coded function pointer so this isn't really that expensive.

Oh yeah sorry, I'm pretty inaccurate about that. The problem with a runtime function pointer is that inlining isn't possible. And it is a bit ugly in a type signature IMO.

torkleyy · Aug 15, 2017

src/audio/mod.rs

+mod audio_context;
+mod source;
+
+pub use self::audio_context::AudioContext;


I guess we don't need such a deep hierarchy. I would move source and context into this file, actually.

I like to keep my mod files as clean as possible, make them only describe the hierarchy and exports. Makes the code a little easier to read in my opinion. What do you think of that?

torkleyy · Aug 15, 2017

src/audio/output.rs

+
+/// Get a list of outputs available to the system.
+pub fn outputs() -> Vec<Output> {
+    get_endpoints_list().map(|re| Output {endpoint: re}).collect::<Vec<Output>>()


Is that allocation necessary?

I guess it isn't. (I'm really looking forward to impl Trait so that I can have efficient interfaces that aren't a pain to write)

torkleyy · Aug 15, 2017

src/audio/play.rs

+        /// See documentation for DecoderError here:
+        /// https://docs.rs/rodio/0.5.1/rodio/decoder/enum.DecoderError.html
+        Err(_) => {
+            Err(())


Could we still return a proper error here?

@torkleyy What would you consider proper?

Not () 😄 Just think about the case where a user unwraps it or returns Box<Error> or similar cases. A unit type DecoderError is much more descriptive here.

torkleyy · Aug 16, 2017

torkleyy reviewed Aug 16, 2017

View changes

torkleyy · Aug 16, 2017

src/ecs/components/audio/audio_listener.rs

+/// An audio listener, add this component to the local player character.
+pub struct AudioListener {
+    /// Position of the left_ear in the world.
+    pub left_ear: [f32; 3],


Not sure if this should be a global position.

Yeah I had the same thought after I pushed the commit. I'll fix it.

torkleyy · Aug 16, 2017

src/ecs/components/audio/audio_listener.rs

+}
+
+impl Component for AudioListener {
+    type Storage = VecStorage<Self>;


As far as I see it, there is only one AudioListener, so VecStorage is not very efficient here.

Alright, I'll check out the other storage types and see if I can find a better one.

torkleyy · Aug 16, 2017

src/ecs/components/audio/audio_emitter.rs

+    }
+
+    /// Adds an audio source to the queue of sounds to be played from this emitter.
+    pub fn append(&self, source: &Source) {


Shouldn't a source also allow playing multiple sounds at once? If that's already the case, I would ask you to improve naming or docs.

Current architecture doesn't permit multiple sounds at once, but it probably should. I'll think about ways to accomplish this.

Xaeroxe · Aug 17, 2017

Xaeroxe reviewed Aug 17, 2017

View changes

Xaeroxe · Aug 17, 2017

src/ecs/systems/audio.rs

+                    let z = transform.0[3][2];
+                    let emitter_position = [x, y, z];
+                    // Remove all sinks whose sounds have ended.
+                    for i in 0..audio_emitter.sinks.len() {


This is broken and will cause panics. I was trying to emulate the retain functionality here since that doesn't exist on SmallVec. I've opened a PR to add retain to SmallVec here

torkleyy · Aug 17, 2017

torkleyy reviewed Aug 17, 2017

View changes

torkleyy · Aug 17, 2017

src/audio/output.rs

@@ -1,27 +1,35 @@
 //! Provides structures and functions used to get audio outputs.
 
+// We have to use types from this to provide an output iterator type.
+extern crate cpal;


Could we have crate declarations in src/lib.rs?

torkleyy · Aug 17, 2017

torkleyy reviewed Aug 17, 2017

View changes

torkleyy · Aug 17, 2017

src/audio/output.rs

+//! Provides structures and functions used to get audio outputs.
+
+// We have to use types from this to provide an output iterator type.
+extern crate cpal;


Somehow my comment was marked as outdated... Could you please move this declaration to src/lib.rs?

I was reluctant to add a dependency on this crate at all, but it was necessary to write the OutputIterator type, as soon as impl Trait is stabilized I'd like to remove this dependency, so I chose to limit the scope of it. Do you feel my decision making process here was sound?

Yes, definitely.

torkleyy · Aug 17, 2017

torkleyy reviewed Aug 17, 2017

View changes

torkleyy · Aug 17, 2017

src/audio/output.rs

+}
+
+/// An iterator over outputs
+pub type OutputIterator = Map<EndpointsIterator, fn(Endpoint) -> Output>;


Could you just create a struct which owns EndpointIterator and map it in the next method? Would probably be more efficient.

torkleyy · Aug 19, 2017

torkleyy reviewed Aug 19, 2017

View changes

torkleyy · Aug 19, 2017

src/audio/play.rs

+/// Play a sound once.
+///
+/// This will return an Error if the loaded audio file in source could not be decoded.
+pub fn once(source: &Source, endpoint: &Output) -> Result<(), DecoderError> {


Calling this function from ECS is a bit ugly, since you got two unwraps above and the default ouptut has to be retrieved every time. Not sure, but I don't think a game should really crash / exit even if such a thing failed. Especially during development, it's enough to see an error in the console IMO.

I fixed how the pong example was handling this, can you take a look at it and tell me what you think?

torkleyy · Aug 19, 2017

torkleyy reviewed Aug 19, 2017

View changes

Wow, looks very nice ;)

torkleyy · Aug 19, 2017

src/audio/output.rs

+
+impl Output {
+    /// Gets the name of the output
+    pub fn get_name(&self) -> String {


Sorry about me nitpicking, but getters shouldn't have a get_ unless necessary.

torkleyy · Aug 19, 2017

examples/04_pong/main.rs

             }
 
             // Check if the ball is above the bottom boundary, if it is not deflect it
             if ball.position[1] - ball.size / 2. < bottom_bound {
                 ball.position[1] = bottom_bound + ball.size / 2.;
                 ball.velocity[1] = -ball.velocity[1];
+                play::once(&self.bounce_sfx, &output::default_output().unwrap()).unwrap();


Do we really need this 6 times? Probably we can improve that once we got proper collision detection added.

Yeah I was just working in the existing framework of the pong example :P I'm happy to refactor this to use a physics system as soon as we have it.

torkleyy · Aug 19, 2017

src/ecs/components/audio/audio_emitter.rs

+}
+
+impl Component for AudioEmitter {
+    type Storage = VecStorage<Self>;


I would use something like BTreeMap for this component.

torkleyy · Aug 19, 2017

src/ecs/components/audio/audio_listener.rs

+use ecs::{Component, HashMapStorage};
+
+/// An audio listener, add this component to the local player character.
+pub struct AudioListener {


Debug implementation would probably be nice here.

torkleyy · Aug 19, 2017

src/ecs/systems/audio.rs

+     ReadStorage<'a, AudioListener>,
+     WriteStorage<'a, AudioEmitter>);
+
+    fn run(&mut self, (transform, listener, mut audio_emitter): Self::SystemData) {


Looks amazing, especially because it uses Transform now! Have to look at this code later.

torkleyy · Aug 19, 2017

torkleyy reviewed Aug 19, 2017

View changes

torkleyy · Aug 19, 2017

src/audio/mod.rs

+
+/// An error occurred while decoding the source.
+#[derive(Debug)]
+pub struct DecoderError;


This should probably implement Error.

It does now!

I think it is better than before, but the function should probably part of the library and I'm not sure if we should get the default output every frame. Do you know how expensive that is?

torkleyy · Aug 19, 2017

I think it is better than before, but the function should probably part of the library and I'm not sure if we should get the default output every frame. Do you know how expensive that is?

Documentation generated on this branch: https://xaeroxe.github.io/amethyst/doc/amethyst/index.html

Xaeroxe · Aug 26, 2017

Documentation generated on this branch: https://xaeroxe.github.io/amethyst/doc/amethyst/index.html

It's really hard to review this given you merged all the commits of renderer into this branch.

torkleyy · Aug 27, 2017

It's really hard to review this given you merged all the commits of renderer into this branch.

I know :/ I wasn't expecting anyone to review this until renderer was merged. Which by the way while I have your attention, is there any reason for us to not merge renderer yet? Everything seems to be up to speed on that branch after @omni-viral 's PR is merged.

Xaeroxe · Aug 27, 2017

I know :/ I wasn't expecting anyone to review this until renderer was merged. Which by the way while I have your attention, is there any reason for us to not merge renderer yet? Everything seems to be up to speed on that branch after @omni-viral 's PR is merged.

Well, let's do this :)

torkleyy · Aug 27, 2017

Well, let's do this :)

torkleyy · Aug 27, 2017

torkleyy reviewed Aug 27, 2017

View changes

torkleyy · Aug 27, 2017

src/ecs/systems/mod.rs

 pub use self::rendering::RenderSystem;
 pub use self::transform::TransformSystem;
 
 // use config::Config;
 use error::Result;
 use ecs::{System, World};
 
-mod rendering;


Actually, the standard seems to be to put use before mod, although I did it the other way around before I read about it.

Oh and could you please try to not create any merge commits in PRs? It is pretty confusing when you're looking at the commit log.

torkleyy · Aug 27, 2017

Oh and could you please try to not create any merge commits in PRs? It is pretty confusing when you're looking at the commit log.

The two commit histories are so intertwined at this point it's basically impossible to rebase, hence the merge commits.

Xaeroxe · Aug 27, 2017

The two commit histories are so intertwined at this point it's basically impossible to rebase, hence the merge commits.

Guess we could also just sqash this into three commits: low level, ecs stuff and example.

torkleyy · Aug 27, 2017

Guess we could also just sqash this into three commits: low level, ecs stuff and example.

I am trying and failing. These two could probably be made to work together without merges but it would involve a lot of manual labor during which I'd probably make a few mistakes and break something.

Xaeroxe · Aug 27, 2017

I am trying and failing. These two could probably be made to work together without merges but it would involve a lot of manual labor during which I'd probably make a few mistakes and break something.

I can do it if you want. Shouldn't be hard thanks to git.

torkleyy · Aug 27, 2017

I can do it if you want. Shouldn't be hard thanks to git.

By all means go ahead

Xaeroxe · Aug 27, 2017

By all means go ahead

Ready to merge as far as I'm concerned. Thanks @torkleyy for your help straightening out the commit history.

Xaeroxe · Aug 27, 2017

Ready to merge as far as I'm concerned. Thanks @torkleyy for your help straightening out the commit history.

Xaeroxe requested a review from amethyst/engine-devs Jul 23, 2017

This was referenced Jul 24, 2017

Closed

Nitro vs Amethyst comparison #245

Closed

Implementing Audio #243

torkleyy added the project: audio label Aug 6, 2017

torkleyy added status: working type: feature labels Aug 7, 2017

Xaeroxe self-assigned this Aug 25, 2017

Xaeroxe changed the base branch from develop to renderer Aug 25, 2017

Xaeroxe changed the base branch from renderer to develop Aug 25, 2017

Xaeroxe changed the title from [WIP] Add Audio support to Add Audio support Aug 27, 2017

Xaeroxe removed the status: working label Aug 27, 2017

Add music to pong example

Loading status checks…

e9c21e2

Xaeroxe requested a review from amethyst/engine-devs Aug 27, 2017

nchashch referenced this pull request Aug 27, 2017
Closed
Release 0.5.0 #241
13 of 13 tasks complete

Xaeroxe changed the base branch from develop to release-0.5.0 Aug 27, 2017

Xaeroxe merged commit c2733e6 into amethyst:release-0.5.0 Aug 27, 2017
1 of 2 checks passed

1 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

Xaeroxe deleted the Xaeroxe:audio branch Aug 27, 2017

amethyst/amethyst

Join GitHub today

Add Audio support #265

Conversation

Xaeroxe commented Jul 23, 2017 • edited Edited 1 time Xaeroxe edited Aug 27, 2017 (most recent)

Things done in this PR:

Xaeroxe requested a review from amethyst/engine-devs Jul 23, 2017

This comment has been minimized.

Xaeroxe commented Jul 23, 2017

This comment has been minimized.

vitvakatu commented Jul 23, 2017

torkleyy reviewed Jul 24, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This was referenced Jul 24, 2017

This comment has been minimized.

ebkalderon commented Jul 27, 2017 • edited Edited 1 time ebkalderon edited Jul 27, 2017 (most recent)

This comment has been minimized.

Xaeroxe commented Jul 27, 2017

torkleyy reviewed Jul 27, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

torkleyy added the project: audio label Aug 6, 2017

This comment has been minimized.

torkleyy commented Aug 6, 2017

torkleyy added status: working type: feature labels Aug 7, 2017

torkleyy reviewed Aug 15, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

torkleyy reviewed Aug 16, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Xaeroxe reviewed Aug 17, 2017 View changes

This comment has been minimized.

torkleyy reviewed Aug 17, 2017 View changes

This comment has been minimized.

torkleyy reviewed Aug 17, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

torkleyy reviewed Aug 17, 2017 View changes

This comment has been minimized.

Xaeroxe commented Jul 23, 2017 •

edited

Edited 1 time

Xaeroxe edited Aug 27, 2017 (most recent)

torkleyy reviewed Jul 24, 2017

View changes

ebkalderon commented Jul 27, 2017 •

edited

Edited 1 time

ebkalderon edited Jul 27, 2017 (most recent)

torkleyy reviewed Jul 27, 2017

View changes

torkleyy reviewed Aug 15, 2017

View changes

torkleyy reviewed Aug 16, 2017

View changes

Xaeroxe reviewed Aug 17, 2017

View changes

torkleyy reviewed Aug 17, 2017

View changes

torkleyy reviewed Aug 17, 2017

View changes

torkleyy reviewed Aug 17, 2017

View changes