Skip to content
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

No support for sound in the track of Amplitude sliders #179

Closed
pixelzoom opened this issue Sep 25, 2021 · 9 comments
Closed

No support for sound in the track of Amplitude sliders #179

pixelzoom opened this issue Sep 25, 2021 · 9 comments

Comments

@pixelzoom
Copy link
Contributor

From #178 (comment), reported by @kathy-phet:

  • for the amplitude sliders, when you click in the track, keep mouse down, and move up and down, you do not get the clicking slider sounds (I don't naturally do this on the computer version, but this happens a lot on a touch pad device like iPads)
@pixelzoom pixelzoom self-assigned this Sep 25, 2021
@pixelzoom
Copy link
Contributor Author

pixelzoom commented Sep 25, 2021

This is a known issue, symptomatic of (a) lack of a sound API for the common-code Slider, and (b) the Fourier team's decision to go with a partial/adhoc sound implementation for the amplitude sliders.

The current implementation of sound for the amplitude sliders is based on Wave Interference, at the request of the design team. That implementation has no support for interactions with the slider track, as noted in phetsims/wave-interference#519, and there has been no progress on that issue.

The need to support track sounds in common-code Slider.js is also noted in phetsims/sun#697 (comment), and there has been no progress on that issue.

The team's decison on 7/29/21 was to defer any additional work on sound for Fourier sliders, and wait for general support in Slider, see #56 (comment).

So... I recommend doing nothing here. But if you'd like to do additional work on slider sound for Fourier, please reopen #56. @arouinfar @kathy-phet please advise how you'd like to proceed.

@pixelzoom pixelzoom assigned kathy-phet and arouinfar and unassigned pixelzoom Sep 25, 2021
@kathy-phet
Copy link

kathy-phet commented Sep 27, 2021

@pixelzoom - So I understand that there is no sound support for when you "click in the track". The thing that sort of surprised me was after I clicked in the track, and the slider jumped to meet my mouse, I then started to wiggle the slider up and down without releasing and it still did not make any clicking sound even though this is a continuous motion now. Is that also expected and part of the "no sound when clicking in track" behavior?

If this is an associated limitation to the just the single jump not having clicks and isn't easy to fix, then I think we should just publish it as "won't fix". And address it in the slider work to come.

@arouinfar - Please chime in here too.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Sep 27, 2021

When you click in the track and drag, you're not actually dragging the thumb. The track is handling input events, setting the Property value, and the thumb position just stays synchronized with the Property value. This is how Slider.js works, not specific to Fourier's amplitude slider.

I checked Wave Interference, and sound does work when dragging in the track. I haven't made a deep dive. But I'm guessing that the difference is that Fourier provides a custom thumb and track, while Wave Interference uses the default thumb and track. And the "fix" is likely to require changes to Slider.js, and would be more appropriate to be handled as part of phetsims/sun#697 (Slider API for sound).

I can provide no estimate for how long this might take.

Now that you have that information... Do you want me to move forward with this?

@kathy-phet
Copy link

@arouinfar should weigh in, but I am OK with publishing with the current behavior.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Sep 27, 2021

I looked into this further, and the problem is indeed in Slider. If you provide a custom track (as we're doing in Fourier), you are on your own - none of the sound behavior that you've specified for Slider gets propagated to the custom track. I've reported this in phetsims/sun#697 (comment).

Now that I understand the problem, I could work around it. Estimate is 1-2 hours.

@pixelzoom
Copy link
Contributor Author

Now that I understand the problem, I could work around it. Estimate is 1-2 hours.

I'm going to go for it.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Sep 27, 2021

Sound support for the track has been added in the above commits. It took ~30 minutes, and no changes to common-code were required -- it's entirely a workaround in sim code.

Commit 858ed2d was the easy part - getting it working in a hacked-together way.

The harder part was 61fd2c5, factoring it out in away that will make it easy to remove the workaround when Slider eventually supports sound. I had previously factored all of the sound stuff into subclass AudibleSlider, but that approach didn't make it possible to support sound for the track - it had the same problem as Slider, described in phetsims/sun#697 (comment). The new approach is encapsulated in AmplitudeSlider.SoundDragHandler, which handles the drag events, and is shared by both Slider and my custom track.

@arouinfar @kathy-phet please review in master. Please take a good look, because this was a big change.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Sep 27, 2021

I said:

... Please take a good look, because this was a big change.

This was a big enough change that I thought it wise to publish a dev version, for confirming any regressions.

Before the change: 1.0.0-dev.49

After the change: 1.0.0-dev.50

@arouinfar
Copy link
Contributor

I edited the link to dev.49 in @pixelzoom's comment because it was pointing to dev.50.

The behavior in dev.50 sounds good to me @pixelzoom. Thanks for making this change. It definitely improves the experience on mobile.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants