search

phetsims / tandem

Simulation-side code for PhET-iO
MIT License
0 stars 5 forks source link

Several IO types should use ReferenceIO #215

Closed samreid closed 3 years ago

samreid commented 3 years ago

During #211 I noticed several IO types that were simulating ReferenceIO. I'll point their TODOs to this issue. There may be more as well. Also, keep in mind https://github.com/phetsims/phet-io/issues/1709#issuecomment-700805070 has a proposal that PhetioObject.toStateObject would be reference-ish by default.

samreid commented 3 years ago

I converted these and tested briefly. I noted Energy Skate Park was failing before and after this change. @zepumph can you please spot check one or two of these? Close if it seems reasonable.

zepumph commented 3 years ago

Isn't https://github.com/phetsims/energy-skate-park/commit/c0d0a9846f3ec2f650041e113eb772d4ff970980 NullableIO( ReferenceIO( ObjectIO)?

samreid commented 3 years ago

The previous code does seem like it was supposed to be NullableIO, but looking at occurrences, it seems like it shouldn't be. ControlPoint has:

phetioType: ControlPoint.ControlPointIO,

which seems correct, and the snapTargetProperty is:

phetioType: Property.PropertyIO( NullableIO( ControlPoint.ControlPointIO ) ),

So even though I didn't port what was there, perhaps it turned out OK?

zepumph commented 3 years ago

I see. OK sounds good if you feel good. Can you take one more glance for other cases like that, then feel free to close?

samreid commented 3 years ago

The other cases look like they are in the same boat: original version supported null, but didn't need to because it has a Property<Nullable<...>> elsewhere. Closing.