Address problems related to `ContinuousPropertySoundGenerator.resetInProgressProperty`

[zepumph]

Recommended from a conversation with @pixelzoom over in https://github.com/phetsims/faradays-electromagnetic-lab/issues/77.

This could be internal to ContinuousPropertySoundGenerator, and doesn't need to cascade to individual sim usages (since those are reset specific). I just think it would make the API for the sound generator more usable and general [@pixelzoom: if resetInProgressProperty were renamed to updateEnabledProperty].

@jbphet does this sound reasonable? I'll implement if you'd like. Is that the best new name?

[pixelzoom]

I agree with @zepumph. There are may be other reasons for disabling updates, and resetInProgressProperty should be renamed to updateEnabledProperty (and the logic therefore inverted).

But there's more to the problem - so I'll retitle this issue and describe.

The other problem with option resetInProgressProperty is the inconvenience of having to create resetInProgressProperty somewhere (model? view? both?) then pass it through constructors to get it to where ContinuousPropertySoundGenerator is instantiated. That's done inconsistently (one might even say incorrectly) in all of the sims that I looked at, and creates the opportunity for mistakes like https://github.com/phetsims/center-and-variability/issues/612.

To address the need for a Property that tells us if "reset all" is in progress, let's add something like this added to common-code (joist?). This was done in CAV (isResettingPropery), I intend to do it in FEL (more appropriately named isResettingAllPropery), and it would be better not to keep duplicating this pattern.

// isResettingAllProperty.ts

import joistNamespace from './joistNamespace.js';
import TinyProperty from '../../axon/js/TinyProperty.js';

const isResettingAllProperty = new TinyProperty( false );

joist.register( 'isResettingAllProperty', isResettingAllProperty );
export default isResettingAllProperty;

BakingisResettingAllProperty into ResetAllButton would probably be best, maybe even defining public static isResettingAllProperty: TReadOnlyPropery<boolean> therein, readonly so that only ResetAllButton can set it. But if we don't want to do that for some reason...

Each ScreenView would need wireisResettingAllProperty into ResetAllButton like this:

  this.resetAllButton = new ResetAllButton( {
      listener: () => {
       isResettingAllProperty.value = true;
       ...
       isResettingAllProperty.value = false;
      },
      tandem: options.tandem.createTandem( 'resetAllButton' )
    } );

Then use isResettingAllProperty where necessary, instead of having to pass it around through multiple layers of constructors. E.g. in GFLBScreenView:

import isResettingAllProperty from '../../../joist/js/isResettingAllProperty.js';
...
    this.forceSoundGenerator = new ContinuousPropertySoundGenerator( ..., {
      resetInProgressProperty: isResettingAllProperty,
      ...
    } );

Note that is similar to PhET-iO's isSettingPhetioStateProperty, which was created for similar reasons.

[zepumph]

Maybe scenery-phet is best, since it is a reset pattern used by PhET and could potentially be tied directly into the resetAllButton.

Yeah I like this quite a bit. If @jbphet likes it, then I'm happy to implement.

[pixelzoom]

Summary of more discussion that @zepumph and I had in Slack#DM:

• Move ResetAllButton to joist so that it's clear that it's specific to joist and how it's used in ScreenView, and not as generally reusable as other components in scenery-phet.
• Also bake screenView.interruptSubtreeInput() into ResetAllButton, which is chronically duplicated (and often omitted) in sim code.

[jbphet]

The idea of having a centralized isResettingAllProperty seems like a win to me for consistency and avoidance of code duplication. In that case, I am assuming we would change the current resetInProgressProperty option for the above-mentioned sound generator to something like updateDuringReset, and have it default to false, and then use the globally available Property.

Back to @zepumph, since he said he was up for doing the implementation.

[pixelzoom]

... In that case, I am assuming we would change the current resetInProgressProperty option for the above-mentioned sound generator to something like updateDuringReset,

No, not updateDuringReset. We are trying to decouple this from being specific to "reset". @zepumph suggested updateEnabledProperty, and I'm +1.

[zepumph]

Hmm, I'm also curious why we can't use the soundGenerator 'enabledControlProperties' for this case, I'll look into it more.

[zepumph]

I had good success with the idea of using enableControlProperties.

I got particularly excited about it when I saw this precedent: https://github.com/phetsims/tambo/blob/f9fc303fa5433276c76b6041158f77b6ccffb517/js/demo/testing/view/TestingScreenView.ts#L292

As for the pattern more generally, @pixelzoom, I wonder if we want to actually have a singleton that says "resetNOTInProgressProperty" so we can pass it directly into SoundGenerators. Can you please review the above while I go work on https://github.com/phetsims/tambo/issues/187.

[pixelzoom]

I had good success with the idea of using enableControlProperties.

I really do not like this pattern. It differs from the enabledProperty pattern used everywhere else. (Can you imagine Node with enableControlProperties?) If the logic always needs to be inverted for 1 or more Properties (in addition to the resetNOTInProgressProperty) then you need to create one or more DerivedProperty anyway. And it complicates the API if we need the option of PhET-iO instrumentation.

I got particularly excited about it when I saw this precedent:

I wouldn't call that a precedent. I would call that a bad API that should be replaced ASAP, before it's used further -- see https://github.com/phetsims/tambo/issues/189.

So... I think we should stick with the original plan -- updateEnabledProperty: TReadOnlyProperty<boolean>. And if the client needs a derivation that involves > 1 Property (possibly including isResettingAllProperty.ts) then the client should create a DerivedProperty.

[zepumph]

If this issue has turned into changing the entire way that tambo controls its enabled, I'm not sure that I agree. Perhaps lets close this issue and conduct a mass refactor over in https://github.com/phetsims/tambo/issues/189, if that is what is deemed appropriate. If we proceed with updateEnabledProperty, then we are recognizing that ContinuousPropertySoundGenerator requires a unique and custom solution, which I do not believe it does.

[pixelzoom]

Re isResettingAllProperty, please see https://github.com/phetsims/tambo/issues/190:

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.

[pixelzoom]

I'm going to unassign myself. If I'm going to provided feedback while trying to move FEL sound forward, then PhET needs to allocate resources for common code in some (the next?) iteration.

In the meantime... I'm concerned that we may have some partial work in main which is liable to remain there for a long time before it's completed and vetted. Since @zepumph commited that work, so I'm going to assign this issue back to him. @zepumph If you feel like this issue is in a safe stopping place, were we can leave it unattended for awhile, then feel free to unassign yourself. If not, please put main in a "safe" state before unassigning.

[zepumph]

Yes thanks. I believe that the next step here is https://github.com/phetsims/tambo/issues/189, which will be done generally. We may also choose to do more work generally for reset-specific sound cases in https://github.com/phetsims/tambo/issues/190. I believe that closing this issue is best. We did good work by removing a custom option and moving it into a general feature of SoundGenerator. Now we just need to improve those general patterns. Please reopen if you'd like anything more done to ContinuousPropertySoundGenerator's reset handling.