search

phetsims / scenery-phet

Reusable components based on Scenery that are specific to PhET simulations.
MIT License
8 stars 6 forks source link

PhET-iO implementation for GroupSortInteraction #835

Closed zepumph closed 5 months ago

zepumph commented 5 months ago

From https://github.com/phetsims/scenery-phet/issues/815, let's handle PhET-iO in its own issue. I'll move TODOs over

Also noting here that CAV is failing state/migration tests on CT because of this needed work.

zepumph commented 5 months ago

Let's see if that cleans up CT.

zepumph commented 5 months ago

Fixing CT was quite straight forward with migration rules that mapped the dragIndicatorModel.valueProperty to groupSortInteractionModel.selectedGroupItemProperty (with a value of null). This required me to implement https://github.com/phetsims/phet-io-wrappers/issues/600 though. I'm ready to finish discussion during design meeting tomorrow.

zepumph commented 5 months ago

Alright. We had a PhET-iO meeting today where we discussed what was needed for the group sort interaction + PhET-iO. It was determined that it is valuable to have PhET-iO have control over the visual cue. There were a few good reasons for this: If the sim is used as a diagram in a text book, if the client doesn't yet want to convey that the component is moveable. Thus, the first pass that I landed on was to instrument a single Property called showMouseCueProperty. When set the false, the cue will never be shown, when true, it uses default logic from the sim.

I am ready for a PhET-iO tree review with the following notes for the reviewer:

  1. When showMouseCueProperty is true, a new instance of the simulation will always have cues shown (none of the underlying interaction logic is instrumented). 2 Please note that you can pass an enabledProperty into GroupSortInteractionModel, but it doesn't show as a linkedElement (that would be hard to do in this case), and an enabledProperty is not auto instrumented. I don't recommend adding this capability since disabled isn't too important for just the keyboard interaction (if it is needed, likely it will come from outside this model like soccerBallsEnabledProperty does).
  2. I did end up keeping the selectedGroupItemProperty as a phetioReadOnly:true Property. I think it makes sense to allow this to be stateful. It makes the migration from CAV 1.1 into main easier, and it does set us up better for stateful playbacks in the future.

I believe that @arouinfar is a good candidate for this review since she was in the meeting this morning, but please reassign if you think someone else would be a better fit. Or better yet, co-review! Anywho, let me know if you want to chat.

zepumph commented 5 months ago

@arouinfar and I discussed, and we have this list of stuff to do

zepumph commented 5 months ago

Done, and confirmed that it has the behavior that @arouinfar and I were hoping for during the meeting. Closing

phet-dev commented 5 months ago

Reopening because there is a TODO marked for this issue.

zepumph commented 5 months ago

Thanks @phet-dev. I appreciate you.