Rewrite Vibe to use Tambo

[samreid]

We are currently in a hybrid situation where we have some sims using Vibe and some sims using Tambo. This makes it difficult to specify query parameters that will e.g. turn the sound off for all sims. For example, in Joist, we would like to have a variable supportsSound but we are turning that on using ?tambo. So it would be good to unify things. One way to do so would be to rewrite Vibe to use Tambo. @jbphet @chrisklus and I tried this and got Sound.js down from 227 lines of code to 44 lines of code.

To get this working with ?audioVolume, which Vibe used to work nicely with, I also made changes in Tambo to respect that query parameter. (Note we should change that to ?soundVolume to match other query parameters for tambo). I'll create branches for this as well for @jbphet to review and decide how to proceed.

[samreid]

OK changes pushed, the ball is in your court Mr. @jbphet.

[jbphet]

@ariel-phet - there are a lot of positives to doing this, but one downside with be that we will effectively be ending support for sound on Internet Explorer on any sims hereafter published from master. In https://github.com/phetsims/tambo/issues/6, we decided that this was okay for tambo, but we weren't really thinking about converting vibe to use tambo at the time. So, are we okay with this?

[samreid]

One way to do so would be to rewrite Vibe to use Tambo.

The long term solution will be to rewrite the Vibe sims to use Tambo directly.

The advantage of doing this now (at least rewriting Vibe to use Tambo as demonstrated in the branch) is it will allow us to choose the appropriate long term API that we want for sound features, including but not limited to query parameters (that teachers and PhET-iO clients could use). The other advantage of doing this now is that we will have a single audio solution that we need to maintain (tambo) instead of two substantially different ones (tambo + vibe). Finally, the sims that are published with Vibe already have support for IE and will continue to do so for their maintenance releases. They will lose support when republished from master.

[jbphet]

It just occurred to me that we will probably want to remove the sound control button the game screens if and when we do this. We will also end up with the sound icon on the nav bar, which won't have any effect on the first two screens.

Note to self: We will also need to add sound support to the sim options in the main file for the sim.

[ariel-phet]

Currently it does not make sense to go retrofitting existing sims, and am not prepared to drop support for IE 11 just yet.

We can revisit this discussion when we drop IE 11 support.

@samreid feel free to close or mark deferred.

[samreid]

Currently it does not make sense to go retrofitting existing sims

No work will need to be done on the existing sims, we can use the branch that @jbphet and I already developed which is only a change in the vibe repo.

not prepared to drop support for IE 11 just yet.

What is the timeline for publishing our first "sonification" simulation with tambo implementation? What is the timeline for republishing a vibe simulation out of master (not a maintenance release)? How hard would it be to add (even rudimentary, i.e. playing one-shot clips) support to IE11 for tambo?

[jbphet]

No work will need to be done on the existing sims...

There will need to be a bit of work done: Existing sims have sound on/off buttons on the screens where the sound exists. This button will need to be removed, since it will be redundant with the one on the nav bar. One could argue that it doesn't NEED to go, but it seems like a confusing user interface to have the same control in two places.

[samreid]

What is the timeline for publishing our first "sonification" simulation with tambo implementation?

At today's status meeting, @jbphet said he hopes to publish two sonified sims by the end of September.

[ariel-phet]

Marking for dev meeting to discuss pros/cons. I still currently leaning towards not going this route currently, but lets get a better idea of the implications/costs/benefits.

[jonathanolson]

I'm not that familiar with the pros/cons. It seems like a lot of the work might already be in a branch?

[samreid]

Summary of known pros:

• Reduce Sound.js down from 227 lines of code to 44 lines of code.
• We will have a single audio solution that we need to maintain/debug (tambo) instead of two substantially different ones (tambo + vibe)
• This will help us get more experience and testing with our long-term sound implementation.

Summary of known cons:

• New publications from master won't support IE11 (as is the case for tambo)
• We'll need to visit sims and remove the "mute" buttons from them since it will appear in the navigation bar.
• Time to confirm the work in the branch is correct/complete, and any testing/refinement that it requires.

[pixelzoom]

I think it would be good to proceed with this sooner rather than later, but after tambo review.

[samreid]

@jbphet said it may be better to port directly to tambo first (instead of an intermediate phase where vibe is implemented with tambo).

[Denz1994]

@ariel-phet suggests having detection for IE11 and if it is there sound button present in the sim it needs to be removed. Also, an issue should be made in each repo and on next sim run through the sound button should be made compatible with tambo sounds.

@pixelzoom mentioned. At this time there are currently 18 occurrences of SoundToggleButton in different repos.

[samreid]

@jbphet suggested working on this in October, and we agreed that sounds good.

[jbphet]

Here are some checklist items that should be completed before this issue is closed:

• [ ] Search through the codebase for all usages of the audioVolume=0 query parameter, which is used in a number of places to mute sounds during automated testing, and replace them with sound=muted.
• [ ] Remove the audioVolume query parameter support from initialize-globals.js.

[jbphet]

The fractions issue https://github.com/phetsims/fractions-common/issues/80 may be all or partially addressed by this.

[KatieWoe]

For https://github.com/phetsims/QA/issues/294, muting while the "you won" cheering is happening does not stop the sound.

[jbphet]

Due to the changes to sound module loading that were necessary to address https://github.com/phetsims/tambo/issues/100, I have rewritten Sound.js to use tambo. My initial testing shows that this seems to work fine, but there needs to be a bit of followup, and I've listed specific requests below.

In case anyone is wondering why we don't just convert all sims to use tambo instead of vibe, it's because using tambo comes with a lot of other things, such as the sound on/off button on the nav bar and common UI sounds. Porting vibe to be based on tambo allows sims that are using vibe for basic sound generation to continue doing so without having to implement a full sound design for them. Examples of such sims are plinko-probability and forces-and-motion-basics.

Requested followup:

• [x] @KatieWoe - Can you please test sound on several sound-enabled sims on all supported platforms and make sure that we don't have any issues? Please include at least one tambo-based sim, such as friction, and one vibe-based sim, such as plinko-probability.
• [x] @samreid - Please review the code change. It essentially consists of gutting the contents of Sound.js and replacing it with tambo classes.
• [x] @ariel-phet - This is to let you know that this change ends all support for any sound on IE. Prior to this change, some sims, such as Plinko, would still produce sound on IE. This changes that, so now if you're on IE you get no sound at all. It's a little weird, because the sims that use vibe generally have sound on/off buttons in their play areas, and those buttons won't do anything on IE. Given our usage on IE, I'm assuming that this is okay, but I thought I should check that with you. If necessary, we could hide the sound buttons on IE. If you feel that we should, please log a global issue in vibe and I'll create individual ones for the sims that need it.

[KatieWoe]

@jbphet can you make versions of Molarity, John Travoltage, Plinko Probability, and FAMB? Some devices don't have a VPN, making it hard to check master.

[KatieWoe]

Vibe sound isn't working in IE, even though it should. @jbphet do you want me to continue testing?

[jbphet]

@KatieWoe - Yes. As noted in https://github.com/phetsims/vibe/issues/33#issuecomment-628185583 (in the part that is marked for @ariel-phet):

[T]his change ends all support for any sound on IE.

He has to weigh in about whether that's okay or not, but it's a known consequence of the work done here.

[ariel-phet]

@jbphet I think considering our IE usage and that we don't really even have good metrics as to how much sound is actually used in the classroom (although we have good anecdotal evidence that students/teachers appreciate the correct "ding" in games) I am fine with this feature just not working in IE.

If we start to get reports emailed to us of sound not working, we can consider hiding the sound buttons, but at this point I think IE is becoming such a obsolete browser that anyone using it realizes some things are going to be broken. And I am pretty sure that since Windows 7 support has been officially dropped by MS it is possible we could drop IE support altogether, seems like a good impetus to check into that.

[jbphet]

@KatieWoe - I've published the following dev versions in support of this testing (as requested above):

• https://phet-dev.colorado.edu/html/molarity/1.6.0-dev.3/phet/molarity_en_phet.html
• https://phet-dev.colorado.edu/html/john-travoltage/1.6.0-dev.17/phet/john-travoltage_en_phet.html
• https://phet-dev.colorado.edu/html/plinko-probability/1.2.0-dev.5/phet/plinko-probability_en_phet.html
• https://phet-dev.colorado.edu/html/forces-and-motion-basics/2.4.0-dev.4/phet/forces-and-motion-basics_en_phet.html

[KatieWoe]

QA is done

[jbphet]

Thanks @KatieWoe. The only issue that came out of this is closed, so I'm going to go ahead and close this one.

[samreid]

Reopening based on this TODO:

* TODO: This should be removed once all usages of Vibe have been converted to Tambo, see https://github.com/phetsims/vibe/issues/33.
* 

@jbphet can you please advise?

[jbphet]

@jbphet can you please advise?

I think it would be safe to remove this query parameter, but we need to check that aqua isn't using it anymore, then verify that things work as intended, which is to say that continuous testing doesn't generate a bunch of sounds. Plinko Probability would be a good one to test, since it using the Sound type from vibe, which now uses tambo.

[samreid]

I got partway through this, but it wasn't clear what to do about this line in Sound.js:

sharedGainNode.gain.setValueAtTime( phet.chipper.queryParameters.audioVolume, phetAudioContext.currentTime );

[jbphet]

Thanks @samreid. Before going any further, we should remove all usages of the query parameter and make sure that it doesn't break anything and also doesn't produce horrendous nasty sounds when we don't want them.

I just searched through the code base for usages of audioVolume, and found three that should be removed. The are in take-snapshot.js in the aqua repo, in phetmarks.js in the phetmarks repo, and index.html in the phettest repo. All three of these are, I believe, managed by @jonathanolson. I'm going to assign this issue to him. Once the usages of this parameter have been removed from those repos, please assign this back to me, and I'll remove it from vibe.

[jonathanolson]

@jbphet what should I change the references to, in order to turn off audio (e.g. during fuzzing)?

[jbphet]

I think you may already be using this in most of the places where it is needed, but the query parameter to turn off sound that comes from tambo is supportsSound=false.

[jbphet]

Due to the work that has been done for https://github.com/phetsims/joist/issues/734, we have a new query parameter that can be used to disable sounds. It is audio=disabled, and I'm going to use it in Sound.js, and then we should have a universal way of disabling sound whether using sounds from the vibe or tambo repos. While doing this, I also intend to get rid of the audioVolume query parameter.

[jbphet]

@jonathanolson - I have removed support for the audioVolume query parameter and attempted to replace all usages of it in aqua with audio=disabled. Please review the commits above and, if all looks good and you think I got everything, this can be closed.

[jonathanolson]

Looks good to me!

[jbphet]

Cool. Closing.