phetsims / sun

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

Dynamic range in Slider #802

Open jonathanolson opened 1 year ago

jonathanolson commented 1 year ago

Most work accomplished in https://github.com/phetsims/sun/issues/792, I'll enable this shortly (and there will be a sound TODO).

jonathanolson commented 1 year ago

Slider allows dynamic ranges above, with the one issue: I didn't see an easy way to move ValueChangeSoundPlayer to a dynamic range (i'm not as familiar).

Right now, it's constructed with the initial range.

@jbphet do you know whether ValueChangeSoundPlayer should adjust to a dynamic range?

jbphet commented 1 year ago

@jbphet do you know whether ValueChangeSoundPlayer should adjust to a dynamic range?

I just looked at the code, and it doesn't look like it would. I think I would be able to add support for this fairly easily, but I'm not 100% certain, since I haven't been in that code for a while. If I should embark on such a mission, I have a few questions:

jonathanolson commented 1 year ago

Hey, I just realized... Slider could create a new ValueChangeSoundPlayer whenever its range changes? This seems like a change I could make. Reassign to me if that seems like it solves it and wouldn't leak memory (I don't see a dispose). Otherwise, answers below:

Are there other use cases that are envisioned?

I've had to work around not being able to change the range on a Slider in the past. Presumably cases where I refactor it to dynamic ranges, or enabled range changing.

Would the sound player need to be able to handle a range change of any arbitrary magnitude?

Possibly?

What is a good test case for my changes?

Probably manual tests, I don't know of a good way in the code.

Should I just make some reasonable decisions and basically just make sure that nothing breaks, since it's not likely to be a very commonly used feature? Can you give me some sense of the urgency and priority of implementing this?

That might be good, I'm not sure about the urgency, it's more "if we change the range on a slider, will it break, or will its sound just be a bit off"? So probably not high priority right now.

jbphet commented 1 year ago

Reassign to me if that seems like it solves it and wouldn't leak memory (I don't see a dispose).

I like that idea. That way the client could control how the sound player should adapt the the change in range instead of building assumptions into the sound player itself. I'd suggest moving forward with this, and let me know if you need any help with it.

As for the lack of a dispose function, it doesn't need one. It doesn't link to any properties.