phetsims / projectile-data-lab

"Projectile Data Lab" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
0 stars 0 forks source link

IOType problems #325

Closed pixelzoom closed 1 month ago

pixelzoom commented 2 months ago

I was trying to use this sim as an exemplar for instrumenting particle systems in https://github.com/phetsims/gas-properties/issues/231. I noted the following problems:

(1) In Projectile.ts, ProjectileIO does not identify which type of serialization it uses, as described in https://github.com/phetsims/phet-io/blob/main/doc/phet-io-instrumentation-technical-guide.md#serialization.

(2) In Field.ts, type FieldStateObject is missing, and the result is a lack of type checking:

(3) Also in Field.ts, the documentation for FieldIO does not link to https://github.com/phetsims/phet-io/blob/main/doc/phet-io-instrumentation-technical-guide.md#serialization.

(4) All of the IOTypes are currently public static. They should be public static readonly, e.g.:

-  public static FieldIO = new IOType<Field>( 'FieldIO', {
+  public static readonly FieldIO = new IOType<Field>( 'FieldIO', {
pixelzoom commented 1 month ago

Reopening. This is important to get right because I'm using PDL as an exemplar for instrumenting parts of GP.

ProjectileIO and FieldIO are documented as "value-based serialization". That is not one of the 3 types of serialization at https://github.com/phetsims/phet-io/blob/main/doc/phet-io-instrumentation-technical-guide.md#serialization. Do you mean data-type serialzation?

While ProjectileIO certainly looks like data-type serialization, I'm not convinced that FieldIO is data-type serialization. Data-type serialization does not typically use applyState, and I do not see Field instances being instantiated as the result of deserialization. FieldIO also does not seem like reference-type or dynamic-element serialization. Is it possible that this is a 4th serialization pattern that is not identified in the Technical Guide?

ProjectileIO doc is also missing the link to https://github.com/phetsims/phet-io/blob/main/doc/phet-io-instrumentation-technical-guide.md#serialization.

samreid commented 1 month ago

I updated the documentation to answer the questions above, please review.