phetsims / tandem

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

Eliminate IOType.fromCoreType #273

Closed samreid closed 1 year ago

samreid commented 1 year ago

From https://github.com/phetsims/tandem/issues/263, I would like to eliminate IOType.fromCoreType. It was introduced as a convenience method, but it is making the type checking very complex and difficult, and making it difficult to make progress in classifying the different state serialization types.

We also now have well-established patterns and examples and type checking, so using the IOType constructor can be used safely. And it will be nice to have one way of doing things instead of two.

samreid commented 1 year ago

To make sure I didn't miss any use cases, I instrumented fromCoreType to output the different parts that are being passed through to the options. Here are the results:

AtomIO stateSchema

FourierComponentIO stateSchema
FourierComponentIO fromStateObject
FourierComponentIO toStateObject

EMEnergyPacketIO stateSchema
EMEnergyPacketIO fromStateObject
EMEnergyPacketIO toStateObject

EMWaveSourceIO stateSchema
EMWaveSourceIO coreTypeHasApplyState
EMWaveSourceIO toStateObject

EnergyInfoQueueItemIO stateSchema
EnergyInfoQueueItemIO fromStateObject

EnergyRateTrackerIO stateSchema

FrictionModelIO stateSchema

LayersModelIO stateSchema
LayersModelIO coreTypeHasApplyState
LayersModelIO toStateObject

PhotonCollectionIO stateSchema
PhotonCollectionIO coreTypeHasApplyState
PhotonCollectionIO toStateObject

PhotonIO stateSchema
PhotonIO fromStateObject
PhotonIO toStateObject

PlankIO stateSchema
PlankIO toStateObject

SpaceEnergySinkIO stateSchema

SunEnergySourceIO stateSchema

Vector2IO stateSchema
Vector2IO fromStateObject
Vector2IO toStateObject

WaveAtmosphereInteractionIO stateSchema
WaveAtmosphereInteractionIO fromStateObject
WaveAtmosphereInteractionIO toStateObject

WavesModelIO stateSchema
WavesModelIO coreTypeHasApplyState
WavesModelIO toStateObject

WaveAttenuatorIO stateSchema
WaveAttenuatorIO fromStateObject
WaveAttenuatorIO toStateObject

WaveCreationSpecIO stateSchema
WaveCreationSpecIO fromStateObject
WaveCreationSpecIO toStateObject

WaveIO stateSchema
WaveIO CoreType.stateToArgsForConstructor
WaveIO coreTypeHasApplyState
WaveIO toStateObject

BunnyIO coreTypeHasApplyState
BunnyIO CoreType.stateToArgsForConstructor
BunnyIO CoreType.STATE_SCHEMA

GenePairIO coreTypeHasApplyState
GenePairIO CoreType.STATE_SCHEMA

GenotypeIO coreTypeHasToStateObject
GenotypeIO coreTypeHasApplyState
GenotypeIO CoreType.STATE_SCHEMA

PhenotypeIO coreTypeHasToStateObject
PhenotypeIO coreTypeHasApplyState
PhenotypeIO CoreType.STATE_SCHEMA

BunnyCountsIO coreTypeHasToStateObject
BunnyCountsIO CoreType.fromStateObject
BunnyCountsIO CoreType.STATE_SCHEMA

ProportionsCountsIO coreTypeHasToStateObject
ProportionsCountsIO CoreType.fromStateObject
ProportionsCountsIO CoreType.STATE_SCHEMA

WolfIO coreTypeHasToStateObject
WolfIO coreTypeHasApplyState
WolfIO CoreType.stateToArgsForConstructor
WolfIO CoreType.STATE_SCHEMA

DataPointIO CoreType.fromStateObject
DataPointIO CoreType.STATE_SCHEMA

ProjectileObjectTypeIO coreTypeHasToStateObject
ProjectileObjectTypeIO coreTypeHasApplyState
ProjectileObjectTypeIO CoreType.STATE_SCHEMA

TrajectoryIO CoreType.stateToArgsForConstructor
TrajectoryIO CoreType.STATE_SCHEMA

RAPRatioTupleIO coreTypeHasToStateObject
RAPRatioTupleIO CoreType.fromStateObject
RAPRatioTupleIO CoreType.STATE_SCHEMA
samreid commented 1 year ago

Some notes from this issue:

TODO: Because serialization involves accessing TODO: Why do some of these things just have stateSchema????

if it has either toStateObject or stateSchema, then it must have both

if it has toStateObject, it must have fromStateObject | applyState | { stateToArgsForConstructor applyState }

pixelzoom commented 1 year ago

Reopening.

Changes like this one in natural-selection violate PhET naming conventions.

- Bunny.BunnyIO = IOType.fromCoreType( 'BunnyIO', Bunny );
+ applyState: ( t, s ) => t.applyState( s ),

Abbreviated names are discouraged, especially names like t and s that convey no information.

I have a hunch that this is what it should be, but I'd have to dig around to be sure:

applyState: ( bunny, stateObject ) => bunny.applyState( stateObject ),

@samreid you did applyState: ( t, s ) 6 times in natural-selection, 1 time in projectile-motion. Please use more descriptive names.

pixelzoom commented 1 year ago

@samreid I see similar naming violations for toStateObject and fromStateObject in the above commits. Please fix those too.

samreid commented 1 year ago

Good idea, I improved the parameter names in the commits. Please let me know if I missed any.

pixelzoom commented 1 year ago

Did you mean to assign this to me for review? I took a peek, looks like you got them all. Feel free to close.