Why is "Reset All" not handled globally in soundManager.ts?

[pixelzoom]

Related to the proposal to add isResettingAllProperty.ts in #186, and discovered in #189 (comment) ...

If we don't want sound when doing a "Reset All", why are we making every generator/clip deal with it? Why are we not handling it globally in soundManager.ts.

If the only issue (?) is that ResetAllButton needs to make sound, we could probably make that work.

@jbphet thoughts? @zepumph FYI.

[jbphet]

If I recall correctly, we didn't handle Reset All globally in the early days of adding sounds to sims because the sound designers originally thought they might want to have sims produce sounds that indicated changes to model and view components when a Reset All occurred. However, at this point in time, I'm pretty sure we never do that - we've realized that playing a bunch of sounds at the same time is generally incomprehensible to our users, so we just play the Reset All earcon and leave it at that.

I'm fine with the idea of handling it globally, and agree that we can likely figure out a way to allow the Reset All button its sound during a reset.

Hopefully that answers the question in the title. Assigning back to @pixelzoom. It seems like we're gearing up to spend some time improving certain aspects of the sound library, so perhaps the next steps are to give this some sort of relative priority and put it on a list of requested changes.

[pixelzoom]

Thanks @jbphet. Unassigning until addressing sound issues is prioritized for an iteration.

[jbphet]

Handling reset all in soundManager would actually be pretty tricky, since the only way we would could prevent sounds from being played there would be to mute a gain node in the signal path, which could lead to a number of problems, not the least of which would be knowing when to turn the gain back up (because there would be no easy way to know how long a played-but-muted sound is). I think it's a better idea to stick with the original approach, which is to prevent sounds from being played at all during a Reset All, but to make it more global. After reading through several related issues, I thought @pixelzoom's suggesting in a recently closed issue sounded quite good. In this approach, we make a single isResettingAllProperty available from the ResetAllButton class.

I implemented this approach and reviewed the changes to ResetAllButton with @pixelzoom, and he approved, so this has now been committed. I've already tested it out in mean-share-and-balance, and will commit that code shortly. I'll then look through the code base for other places where this can replace existing, sim-specific isResettingAllProperty instances.

[jbphet]

Hey @pixelzoom - I know we originally decided that I didn't need to assign this to you for review, but after we met I found what I thought might be a better way to implement this. PushButtonModel has a field isFiringProperty that looks like it may have been added to support things related to a11y. Using this seems less intrusive, since it is now unnecessary to wrap the provided listener in this case. So please have a look at the latest version.

Also, you may want to give this a test drive by replacing FEL's isResettingAllProperty.ts.

[pixelzoom]

Using PushButtonModel isFiringProperty looks good, and I switched to using ResetAllButton isResettingAllProperty in FEL (see above commit).

Back to @jbphet, feel free to close if this is done.