search

phetsims / my-solar-system

"My Solar System" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
2 stars 2 forks source link

Use NumberControl instead of custom button+slider in SolarSystemCommonSlider #105

Closed zepumph closed 1 year ago

zepumph commented 1 year ago

In https://github.com/phetsims/solar-system-common/commit/74042910dec21e4462ef3268a7df75a7fae15bed buttons were added to this solar system common slider. This should instead be a NumberControl because you get a lot of support for free with that solution. I'll take a look!

samreid commented 1 year ago

In today's meeting, it sounded like this is done or nearly done? I'll label as ready for review in case that's true.

zepumph commented 1 year ago

I do not agree. Let's first take care of the TODOs pointing to this issue:

https://github.com/phetsims/solar-system-common/blob/d0174288e9e758bf6109e80f4c89945f906ed5f8/js/view/SolarSystemCommonSlider.ts#L79-L85

samreid commented 1 year ago

I added different sounds for the min and max, but the implementation is not very elegant and there may be a better way to do it.

AgustinVallejo commented 1 year ago

Thanks! Although found a problem with this implementation: When you move it to the border and back, it doesn't sound. We can pair tomorrow to take a look into it and review :)

zepumph commented 1 year ago

Ok, @samreid and I got to a good point on this issue. We have two more remaining todos, and they are both sound related:

It will be best to proceed with a sound design meeting.

DianaTavares commented 1 year ago

Hi! I see that this issue is assigned to me. I can be in the meeting for support (I am not an expert in sound design, I have been learning for the first time with this simulation) but I am out of the office all Monday and half of Tuesday. @Ashton-Morris is the one that created the sound for this simulation, and also is based on how mass sliders and buttons behave in other sims that Aston knows very well. I think that this issue can be solved with him, @AgustinVallejo and the developers working on this issue.

AgustinVallejo commented 1 year ago

With @samreid, @arouinfar and @zepumph we decided that we should take a look into Slider's common code to try using the same sound for all the interactions.

pixelzoom commented 1 year ago

In https://github.com/phetsims/scenery-phet/commit/574887be6ed90d4308c7ff92d31e43ae7de6f3ce, @zepumph left dead code in NumberControl.ts, at line 591:

        children: [
    /*      new HBox( {
            spacing: options.titleXSpacing,
            children: [ titleNode, numberDisplay ]
          } ),*/
arouinfar commented 1 year ago

To be discussed in conjunction with #131

samreid commented 1 year ago

I'll reassign @zepumph in case he is needed regarding https://github.com/phetsims/my-solar-system/issues/105#issuecomment-1479701619. Self-unassigning, but please re-invite me if needed.

zepumph commented 1 year ago

I am not, thanks for the ping. That was accidental commit that was correctly reverted by @jessegreenberg. Sorry for the trouble.

arouinfar commented 1 year ago

Discussed with @DianaTavares @emily-phet @Ashton-Morris @jbphet. We reviewed the latest Mass control sounds on master. The increment/decrement buttons use the same sound as the slider.

What should the min/max sounds be? Currently committed is the same sound as general steps throughout the slider. Do we want something else?

Not all siders need special min/max sounds. This feels analogous to the Mass slider on Gravity Force Lab (turn on Enhanced Sound) or Solution Volume in Molarity. We think it can stay the way it is.

If we keep it the way it is, Slider doesn't have good support for it, so we should work on the code quality a bit.

@jbphet was surprised by this and will follow up with @zepumph to find out more.

What should the arrow button sounds be? @samreid and I experimented with mapping the value continuously, but found that the playback rate only changes 0.003 per button tweak, so it takes ~6 button presses to hear any noticeable change.

The increment/decrement buttons make small changes, so this behavior is expected and reasonable. The sound design team is happy with the way it currently sounds.

Leaving open for @AgustinVallejo to review, and assigning @jbphet to reach out to @zepumph.

zepumph commented 1 year ago

Happy to do something sync, but likely you will see it to. Basically we want three sound features, and have to duplicate something 3 times to get it:

  1. Slider sound
  2. Arrow Button sounds
  3. Min max sounds

All of which to be the same ValueChangeSoundGenerator, and potentially to be internal to Slider/NumberControl if possible.

https://github.com/phetsims/solar-system-common/blob/1da44066f95e716afd931270bdb80eefe7f7d445/js/view/SolarSystemCommonNumberControl.ts#L32-L44

Some more specifics:

  1. We love the way the the ValueChangeSoundGenerator is working internally to slider. We want to use it everywhere because it sounds so good.
  2. passing in valueChangeSoundGeneratorOptions to slider doesn't work well because that creates a private soundGenerator that we can't use, so we are making our own and passing it through.
  3. We found that numberControl was providing defaults, and couldn't be opted out, so we needed to remove an assertion in Slider about mutual exclusive options (only the options or the sound generator, but never both).
  4. We want to use that valueChangeSoundGenerator on the ArrowButtons, so that they can play the sound associated with each tweaker change.
  5. We want to use the same valueChangeSoundGenerator as the min/max sounds, but there is no support for that, so we have pulled out hard coded sounds to play in those cases, what about an option like defaultSoundForMinMaxToo

I would highly recommend @jbphet puts some eyes on the code linked above. I'm happy to help in the cleanup if desired, but it seems like you all have a good design and can execute from here. I have no skin in the game, but was just running up against a lot of duplication needs for the sound generation.

AgustinVallejo commented 1 year ago

04/07 with @arouinfar and @jonathanolson:

Hey @jbphet, as this is no longer a My Solar System issue, if you think this should be worked on, could you transfer the necessary comments to a Sun issue and close this one?

zepumph commented 1 year ago

as this is no longer a My Solar System issue

I'm not really sure what you mean, there are so many problems with sound in SolarSystemCommonNumberControl. @samreid and I wrote hacky "talking points" as we were working on the problem 2 months ago and now it has been long enough that we can just put those into production?

  1. Arrow button tweakers sound never changes based on mass, it is just the "default" mass sound. This is a bug to the sonification, and likely hasn't been signed off on.

  2. I don't know how we can feel good about this block of code. If it was my sim, I would want to work on ValueChangeSoundPlayer to remove all the duplication in the implementation. If that isn't in the cards can we at least open up a common code code issue to support our min-max- sound needs generally, and write TODOs here so that it can get cleaned up.

    https://github.com/phetsims/solar-system-common/blob/4fbca25621c20a43ae664264923f3669f7286a54/js/view/SolarSystemCommonNumberControl.ts#L33-L58

  3. I stepped in here because, from my understanding, @jbphet has been on the outskirts of this issue, and I didn't want to put parsing my giant comment solely on him.

zepumph commented 1 year ago

Ok. @jbphet @AgustinVallejo and I met and had a productive time. There are three sub issues, but generally we will want an issue to hone in on some NumberControl + sound common fixes (in another issue https://github.com/phetsims/scenery-phet/issues/806).

I believe a good number of those items should block publication of this, and may require cleanup over here once complete, so I'll mark as blocked and blocking :)

AgustinVallejo commented 1 year ago

@jbphet Fixed this in https://github.com/phetsims/scenery-phet/issues/806, thanks! Closing