phetsims / sun

User-interface components for PhET simulations, built on top of Scenery.
http://scenerystack.org/
MIT License
5 stars 12 forks source link

Default sound for momentary toggle buttons. #896

Closed pixelzoom closed 1 month ago

pixelzoom commented 1 month ago

Related to alt input for momentary toggle buttons in https://github.com/phetsims/sun/issues/796 ...

Should momentary toggle buttons use the same default sound as "sticky" toggle buttons? They currently do not, and momentary toggle buttons use the default button sound. (Also note that sound is currently incorrect for round "sticky" toggle button, see #895.)

Or should momentary toggle buttons have a different default, to distinguish them from sticky toggle buttons?

I'll start by assigning this to @jessegreenberg and @terracoda. Feel free to reassign if sound is in someone else's wheelhouse.

jessegreenberg commented 1 month ago

I suspect it should be the same because "sticky" toggle buttons use a general "pushButton" sound.

https://github.com/phetsims/sun/blob/5cef52b93e60df3d9d2b03311a82d3b09510aba1/js/buttons/RoundStickyToggleButton.ts#L43-L44

But I am not familiar, @jbphet may know.

jbphet commented 1 month ago

I think it was probably set up to use the default because no one wanted to spend the time to design something unique, and I don't think much thought went into making it consistent with other buttons, such the "sticky" toggle ones. In other words, I think you should feel free to do whatever makes the most sense to y'all.

jessegreenberg commented 1 month ago

OK, got it, thanks. I was going to suggest we just close without changes. However, I just noticed that momentary buttons don't produce sound at all right now. I don't see any default sounds playing in ButtonNode or Round/RectangularButton.js. It looks like the pattern is that the button model subclass controls when it is time to play a sound. So here is what Ill do:

1) Add produceSoundEmitter.emit() calls to MomentaryButtonModel. 2) Add the pushButton sound player to RoundMomentaryButton and RectangularMomentaryButton (like the sticky toggle buttons).

pixelzoom commented 1 month ago

Sorry @jessegreenberg, I thought there was some default sound coming from ButtonNode, but apparently not.

Note that sound is also missing from RoundToggleButton https://github.com/phetsims/sun/issues/895. Would you mind handling that while you're doing essentially the same thing for RoundMomentaryButton and RectangularMomentaryButton?

jessegreenberg commented 1 month ago

https://github.com/phetsims/sun/issues/896#issuecomment-2384270404

Sounds good, I asked a question about it in #895.

pixelzoom commented 1 month ago

Nothing to do in #895, not sure what I was looking at when I created that issue. Issue closed, sorry for the bother.

jessegreenberg commented 1 month ago

OK, done in the above commit. I started by copying from "sticky" buttons, but it sounded odd to play the pushButton sound on press and release. So I copied the "toggle" button sounds instead.

@pixelzoom can you please review and try it out?

pixelzoom commented 1 month ago

Looks (and sounds!) great to me in sun demo and in pH Scale. @jessegreenberg feel free to close if this work is done.

jessegreenberg commented 1 month ago

Thanks for reviewing, closing.