Closed pixelzoom closed 1 month ago
There are 33 usages of enableControlProperties. Many (most?) involve 1 boolean Property. The few that involve purely "enabled === true" logic could be replaced with DerivedProperty.or
. All of the others involve creating a DerivedProperty anyway.
enableControlProperties: [
new DerivedProperty( [ model.showChargesProperty ], showCharges => showCharges === 'all' )
]
enableControlProperties: [ DerivedProperty.not( resetAllButton.buttonModel.isFiringProperty ) ]
enableControlProperties: [ surfaceTemperatureIndicatorEnabledProperty, isPlayingProperty ]
enableControlProperties: [ resetNotInProgressProperty, shapeSoundEnabledProperty ],
enableControlProperties: [
playTickMarkBumpSoundProperty,
new DerivedProperty( [ tickMarkViewProperty ], tickMarkView => tickMarkView !== TickMarkView.NONE )
]
enableControlProperties: [
model.isAudioEnabledProperty,
model.isRunningProperty
]
const pitchedPopGenerator = new PitchedPopGenerator( { enableControlProperties: [ resetNotInProgressProperty ] } )
enableControlProperties: [
model.soundScene.isTonePlayingProperty,
model.soundScene.button1PressedProperty,
model.isRunningProperty,
DerivedProperty.not( model.isResettingProperty )
]
There are an additional 4 cases where @zepumph recently applies this odd pattern for https://github.com/phetsims/tambo/issues/186, which I recommended reverting:
enableControlProperties: [ DerivedProperty.not( resetInProgressProperty ) ]
enableControlProperties: [ DerivedProperty.not( model.resetInProgressProperty ) ],
enableControlProperties: [ DerivedProperty.not( model.resetInProgressProperty ) ]
enableControlProperties: [ DerivedProperty.not( resetInProgressProperty ) ],
I think that @jbphet is your lead to discuss this one. My understanding was that sound required that extra level of control for enabled, but if the usages or potential design don't seem to need that, then perhaps it is best to remove this. But I believe it is critical to the implementation of tambo to be able to add in enableControlProperties after construction. I think the primary reason for this is because sound is often not as simple as other tree-like data structures, where each individual instance knows best how to silence itself to avoid clicking/pops and other weirdness. I believe things get problematic when we shutoff from the central gain node, which is why we use enable-control Properties:
Let's hear from @jbphet.
I hadn't seen SoundGenerator addEnableControlProperty
. Diving into the implementation, it makes me even more convinces that this is a horrible API.
SoundGenerator removeEnableControlProperty
is never used. So enableControlProperties
has no need to be dynamic, no need to be an ObservableArray, and addEnableControlProperty
is a convenience method -- at the cost of considerable added complexity in SoundGenerator. I've confirmed that by inspecting uses of addEnableControlProperty
.
There's 1 use of addEnableControlProperty
in InProportionSoundGenerator that could have been avoided by using the enableControlProperties
options.
The other 3 uses of addEnableControlProperty
are in soundManger.ts. Again they are a convenience, and could have been avoided by assembling a Property[] that was passed to soundGenerator
via the enableControlProperties
options. And could therefore be converted to a DerivedProperty passed to as an enabledProperty
option.
Also related to soundManger.ts... 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.
I'm going to unassign myself, since it's not a priority right now. PhET might consider allocating resources for sound common code in a future iteration.
@jbphet thoughts?
I believe I created enableControlProperties
as an array so that new Properties could be added at different times in case everything wasn't available to be bundled into a DerivedProperty
at construction, or conditions within the sim warranted adding or removing things to gate the sounds. From @pixelzoom's investigation above, it sounds like he has found that using a single DerivedProperty
would work fine and would be more consistent with other PhET APIs. I'd be fine with it changing that way, assuming it really does work out in all usages.
Thanks @jbphet. Unassigning until addressing sound issues is prioritized for an iteration.
This change has been implemented. It seems like a good change in that it is both more consistent with other PhET libraries (such as Scenery) and simplifies the tambo code.
Assigning to @pixelzoom for review, as discussed in today's developer meeting. The biggest code risk is probably how fullyEnabledProperty
works in SoundGenerator
, so that's probably a decent place to start the review. The changes to SoundGenerator
and soundManager
should, in general, be the focus of the review, with some spot checking of changes made in the sim code to adapt to the revised API.
I reviewed changes to SoundGenerator in https://github.com/phetsims/tambo/commit/4f52df7850bb503f19ef47bbca3c81c4effd4024 and soundManager in https://github.com/phetsims/tambo/commit/4f52df7850bb503f19ef47bbca3c81c4effd4024. The changes looks nice, better API, and I don't have any suggestions or concerns.
I also spot checked some of the sim-specific commits, and noted that there were no commits needed for FEL.
Back to @jbphet in case there's additional work to be done.
No additional work needed - I think we're done here. Closing.
As noted in https://github.com/phetsims/tambo/issues/186#issuecomment-1952663127 ...
SoundClip
enableControlProperties
is a bad API.enabledProperty: TReadOnlyProperty<boolean>
API used everywhere elseSo I recommend replacing
enableControlProperties
withenabledProperty
. Then usages like the one above would be:... which can be easily instrumented if desired by adding
tandem:
.@jbphet @zepumph thoughts?