Tracks and Control Points should use PhetioGroup

[zepumph]

@samreid, @chrisklus, and I found that phet-io state is currently broken for this sim. To get this working we think that we should get this working with Group.js. Though this is still in development over in https://github.com/phetsims/phet-io/issues/1454, it is to a point where this could be worked on here. Especially since the alternative is a broken state. We were uninterested in trying to devote resources to getting things working with the deprecated pattern.

[samreid]

At today's meeting, we agreed @zepumph and @samreid will work on this, but we should be careful to wait until the sim is in RC, so we don't disrupt in-progress work by @jessegreenberg .

[samreid]

Unassigning until we turn our attention to PhET-iO for this sim.

[zepumph]

Unassigning until we turn our attention to PhET-iO for this sim.

[samreid]

I've made significant progress as part of phetsims/tandem#87. I converted the control points and tracks to use PhetioGroup. PhET brand seems to be working OK. Studio seems to be working OK, with a few TODOs and caveats. The state wrapper is failing for unknown reasons, but I'm still planning to commit since this seems an improvement over master (which also has a failing state wrapper).

UPDATE: I committed 1ca8050

UPDATE: For when we return to PhET-iO instrumentation for this sim, I have a suspicion that the state wrapper issue is related to a failed or improper disposal. The current error in the state wrapper with ?screens=1 is:

``` Assertion failed: Impossible set state from iterate; unset state: { "energySkatePark.introScreen.model.trackSetModel.tracks": { "array": [ { "phetioID": "energySkatePark.introScreen.model.parabolaTrack" }, { "phetioID": "energySkatePark.introScreen.model.slopeTrack" }, { "phetioID": "energySkatePark.introScreen.model.doubleWellTrack" }, { "phetioID": "energySkatePark.introScreen.model.loopTrack" } ] }, "energySkatePark.introScreen.model.trackSetModel.controlPointGroup.controlPoint_2.draggingProperty": { "value": false }, "energySkatePark.introScreen.model.trackSetModel.controlPointGroup.controlPoint_3.draggingProperty": { "value": false }, "energySkatePark.introScreen.model.trackSetModel.controlPointGroup.controlPoint_4.draggingProperty": { "value": false }, "energySkatePark.introScreen.model.trackSetModel.controlPointGroup.controlPoint_5.draggingProperty": { "value": false }, "energySkatePark.introScreen.model.trackSetModel.controlPointGroup.controlPoint_6.draggingProperty": { "value": false }, "energySkatePark.introScreen.model.trackSetModel.controlPointGroup.controlPoint_7.draggingProperty": { "value": false }, "energySkatePark.introScreen.model.trackSetModel.controlPointGroup.controlPoint_8.draggingProperty": { "value": false }, "energySkatePark.introScreen.model.trackSetModel.controlPointGroup.controlPoint_9.draggingProperty": { "value": false }, "energySkatePark.introScreen.model.trackSetModel.controlPointGroup.controlPoint_10.draggingProperty": { "value": false }, "energySkatePark.introScreen.model.trackSetModel.controlPointGroup.controlPoint_11.draggingProperty": { "value": false }, "energySkatePark.introScreen.model.trackSetModel.controlPointGroup.controlPoint_12.draggingProperty": { "value": false }, "energySkatePark.introScreen.model.trackSetModel.controlPointGroup.controlPoint_13.draggingProperty": { "value": false }, "energySkatePark.introScreen.model.trackSetModel.controlPointGroup.controlPoint_14.draggingProperty": { "value": false }, "energySkatePark.introScreen.model.trackSetModel.controlPointGroup.controlPoint_15.draggingProperty": { "value": false }, "energySkatePark.introScreen.model.trackSetModel.controlPointGroup.controlPoint_16.draggingProperty": { "value": false }, "energySkatePark.introScreen.model.trackSetModel.controlPointGroup.controlPoint_17.draggingProperty": { "value": false }, "energySkatePark.introScreen.model.trackSetModel.controlPointGroup.controlPoint_18.draggingProperty": { "value": false }, "energySkatePark.introScreen.model.trackSetModel.controlPointGroup.controlPoint_19.draggingProperty": { "value": false }, "energySkatePark.introScreen.model.trackSetModel.controlPointGroup.controlPoint_20.draggingProperty": { "value": false }, "energySkatePark.introScreen.model.trackSetModel.controlPointGroup.controlPoint_21.draggingProperty": { "value": false }, "energySkatePark.introScreen.model.trackSetModel.controlPointGroup.controlPoint_22.draggingProperty": { "value": false }, "energySkatePark.introScreen.model.trackSetModel.controlPointGroup.controlPoint_23.draggingProperty": { "value": false }, "energySkatePark.introScreen.model.trackSetModel.controlPointGroup.controlPoint_24.draggingProperty": { "value": false }, "energySkatePark.introScreen.model.trackSetModel.controlPointGroup.controlPoint_25.draggingProperty": { "value": false }, "energySkatePark.introScreen.model.trackSetModel.controlPointGroup.controlPoint_26.draggingProperty": { "value": false }, "energySkatePark.introScreen.model.trackSetModel.controlPointGroup.controlPoint_27.draggingProperty": { "value": false }, "energySkatePark.introScreen.model.trackSetModel.controlPointGroup.controlPoint_28.draggingProperty": { "value": false }, "energySkatePark.introScreen.model.trackSetModel.controlPointGroup.controlPoint_29.draggingProperty": { "value": false }, "energySkatePark.introScreen.model.trackSetModel.controlPointGroup.controlPoint_30.draggingProperty": { "value": false }, "energySkatePark.introScreen.model.trackSetModel.controlPointGroup.controlPoint_31.draggingProperty": { "value": false }, "energySkatePark.introScreen.model.trackSetModel.controlPointGroup.controlPoint_32.draggingProperty": { "value": false }, "energySkatePark.introScreen.model.trackSetModel.controlPointGroup.controlPoint_33.draggingProperty": { "value": false }, "energySkatePark.introScreen.model.trackSetModel.controlPointGroup.controlPoint_34.draggingProperty": { "value": false }, "energySkatePark.introScreen.model.trackSetModel.controlPointGroup.controlPoint_35.draggingProperty": { "value": false }, "energySkatePark.introScreen.model.trackSetModel.controlPointGroup.controlPoint_36.draggingProperty": { "value": false }, "energySkatePark.introScreen.model.trackSetModel.controlPointGroup.controlPoint_37.draggingProperty": { "value": false } } ```

According to the PhET Project Overview, time for PhET-iO development has not yet been scheduled.

[samreid]

I noted that ?testTrackIndex=2 is failing both before and after this change.

[samreid]

I fixed some errors CT discovered in #318, here is a comment from that issue:

The problem seems to be that the archetype only had 2 control points, but the actual track had 3, so it failed to see that last control point. For now I've solved this by increasing the number of control points in the archetype. I'm not sure if the long term solution is to use a PhetioGroup for the archetype. It's working on my local side, and we may need to defer the long term solution until we are focusing more on next phet-io publication for these sims.

[zepumph]

This seems like a clue that we should be using nested PhetioGroups. I prototyped doing that in Projectile Motion and it went pretty well, though I didn't end up keeping that strategy and went with a different pattern. Are you interested in trying that out?

Or perhaps this isn't possible, because controlPoints are dependency injected?

[samreid]

Currently the model is structured like so:

• Model contains controlPoints and tracks. The tracks have references to the model's control points.

If it was changed to

• Model contains tracks, and tracks contain control points.

Then the nested group would be a good fit. But this would be a non-trivial refactoring--may be best to wait until we are focusing on this for a quarterly goal.

[zepumph]

Understood. I also feel like my PM prototyping was before we really leaned in on the PhetioGroup as a set of Objects separate from the data structures needed by the sim. We started (in CAF/JT) with a 1 to 1 swap from array to PhetioGroup, but I think in a lot of cases it makes more sense to just have lists of the controlPoints you need, but a single central group that holds all for a screen. I wouldn't recommend refactoring unless you saw a large benefit from it.