-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Replace SoundGenerator enableControlProperties
with enabledProperty
#189
Comments
There are 33 usages of enableControlProperties. Many (most?) involve 1 boolean Property. The few that involve purely "enabled === true" logic could be replaced with 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 #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: Lines 467 to 479 in bf81ff4
Let's hear from @jbphet. |
I hadn't seen SoundGenerator |
SoundGenerator There's 1 use of The other 3 uses of 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. |
enableControlProperties
with enabledProperty
enableControlProperties
with enabledProperty
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. |
I believe I created |
Thanks @jbphet. Unassigning until addressing sound issues is prioritized for an iteration. |
…ing reset by default, & other sound lib API improvements, see phetsims/tambo#189
…ing reset by default, & other sound lib API improvements, see phetsims/tambo#189
…ing reset by default, & other sound lib API improvements, see phetsims/tambo#189
…ing reset by default, & other sound lib API improvements, see phetsims/tambo#189
…ing reset by default, & other sound lib API improvements, see phetsims/tambo#189
…ing reset by default, & other sound lib API improvements, see phetsims/tambo#189
…ing reset by default, & other sound lib API improvements, see phetsims/tambo#189
…ing reset by default, & other sound lib API improvements, see #189
…ing reset by default, & other sound lib API improvements, see phetsims/tambo#189
…ing reset by default, & other sound lib API improvements, see phetsims/tambo#189
…ing reset by default, & other sound lib API improvements, see phetsims/tambo#189
…ing reset by default, & other sound lib API improvements, see phetsims/tambo#189
…ing reset by default, & other sound lib API improvements, see phetsims/tambo#189
…ing reset by default, & other sound lib API improvements, see phetsims/tambo#189
…ing reset by default, & other sound lib API improvements, see phetsims/tambo#189
…ing reset by default, & other sound lib API improvements, see phetsims/tambo#189
…ing reset by default, & other sound lib API improvements, see phetsims/tambo#189
…ing reset by default, & other sound lib API improvements, see phetsims/tambo#189
…ing reset by default, & other sound lib API improvements, see phetsims/tambo#189
…ing reset by default, & other sound lib API improvements, see phetsims/tambo#189
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 |
I reviewed changes to SoundGenerator in 4f52df7 and soundManager in 4f52df7. 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 #186 (comment) ...
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?
The text was updated successfully, but these errors were encountered: