phetsims / sun

User-interface components for PhET simulations, built on top of Scenery.
MIT License
4 stars 12 forks source link

Sound for `NumberSpinner` is being played before value is updated, leading to problematic behavior #880

Closed jbphet closed 3 months ago

jbphet commented 3 months ago

While working to add some sounds in mean-share-and-balance (see https://github.com/phetsims/mean-share-and-balance/issues/172), @marlitas ran into a problem where the sound player was being invoked before the value managed my NumberSpinner had been updated. While this would be fine if the same sound was being played in all cases, the sound design for mean-share-and-balance specifies that the pitch of the played sound be based on the value of the number controlled by the spinner, so using the arrowSoundPlayer option was causing incorrect sounds to be played. This seems wrong, and I'd like to try to fix it.

jbphet commented 3 months ago

I've attempted to address this by overriding the default sound production in the arrow buttons and initiating sound production directly inside the NumberSpinner class.

@AgustinVallejo - I'm assigning this to you for review, since the GitHub history indicates that you initially added sound support here in April of 2023. I regression tested on the sims that I think you were working on at the time, and pasted screenshots of the tested controls below. However, I don't know the sims well, so it would be good if you could test the behavior and review the changes.

MSS:

image

Keplers:

image

AgustinVallejo commented 3 months ago

Looked at the sims and this looks like a good change, thanks!