phetsims / vibe

Library for handling audio for PhET simulations. Provides cross-platform support and enables usage of base64 audio embedded in an HTML document.
MIT License
3 stars 3 forks source link

move all of the asynchronous loading of sound to the audio.js plugin #29

Closed jbphet closed 4 years ago

jbphet commented 7 years ago

The decoding of audio data into audio buffers is currently handled in Sound.js. It is an asynchronous operation, and should be handled in the audio.js plugin instead. This was originally pointed out by @jonathanolson in https://github.com/phetsims/john-travoltage/issues/163, where some explicit code was needed in order to handle the loading of the sound(s). This should work more similarly to how the image.js plugin works, where the asynchronous operation is performed in the plugin and the sim is prevented from loading until all data is loaded and decoded.

jbphet commented 7 years ago

While investigating this, @jonathanolson and I saw that the code in SimLauncher.js that prevents the sim from launching until all images are loading should be generalized so that it can track loading of any resource, and then be used to track loading of audio resources.

jbphet commented 7 years ago

@jonathanolson and I looked at this together and came up with a very rough plan of what would need to be done on this. We captured it below. We did this because we suspect that this will be made a low priority for now, but will be needed if and when sonification becomes a higher priority, and we will want to refer to these notes at that time.

jbphet commented 7 years ago

Marking for developer meeting so that we can decide on priority and schedule.

pixelzoom commented 7 years ago

My 2 cents on https://github.com/phetsims/vibe/issues/29#issuecomment-273565595:

• Decide if we can get rid of the .ogg files and only handle .mp3 files, ...

Reducing to one audio file type would be great.

• Modify SimLauncher to monitor resource loading generally ...

Rather than putting this in SimLauncher, how about putting it in a new module (e.g. ResourceLoader)? Or maybe even having AudioLoader, ImageLoader, etc. Whatever the approach, let's avoid one monolithic module.

samreid commented 7 years ago

Everything above sounds good, I would add that the current API is like so:

var shockAudio = require( 'audio!JOHN_TRAVOLTAGE/shock' );
var Sound = require( 'VIBE/Sound' );
//...
var shockSound = new Sound( shockAudio );
//...
shockSound.play();

It seems to me that a superior API would be:

var shockAudio = require( 'audio!JOHN_TRAVOLTAGE/shock' );
//...
shockAudio.play();
samreid commented 7 years ago

@ariel-phet suggested @jbphet could work on this after working on the radio button issue. @jbphet mentioned that he has many higher priority issues at the moment.

Consensus: @jbphet will work on higher priority issues before starting on this.

jbphet commented 6 years ago

Undeferring this - a lot of work is being done on the audio portions of simulations, and it would be good to make this part of the effort.

jbphet commented 4 years ago

I'm going to set this back to deferred. There is some work afoot to migrate from RequireJS to standard JavaScript module support, which became available in ES6, so revising our RequireJS sound plugin new doesn't seem like it would be worth the time. I'll put a reminder on my calendar to reconsider this in a few months, since by then we will likely have our module support approach worked out.

jbphet commented 4 years ago

This is a non-issue now that we have migrated to ES6 modules and no longer use RequireJS plugins. Closing.

samreid commented 4 years ago

Please correct me if I misunderstood this issue, but I thought one of the goals was to move the soundInfoDecoder.decode phase closer to the SimLauncher phase, so that we can be sure all audio data is decoded by the time the sim starts up. Like we do with images. Or would that just unnecessarily delay the launch of the sim by changing a happily asynchronous operation to a synchronous one?

jbphet commented 4 years ago

@samreid - You reopened this, undeferred it, and assigned it to me a while back. I believe what you're referring to here was actually covered by https://github.com/phetsims/tambo/issues/100. Vibe now uses the same loading mechanism as tambo due to the work done for https://github.com/phetsims/vibe/issues/33. Are we good to close this now?

samreid commented 4 years ago

Yes, since the decoding is done before SimLauncher is unlocked, that has been accomplished, thanks! Closing.