Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MoleculeIO needs more specific stateSchema for atoms and atomicBonds #40

Open
samreid opened this issue May 31, 2021 · 4 comments
Open

Comments

@samreid
Copy link
Member

samreid commented May 31, 2021

From https://github.com/phetsims/phet-io/issues/1774, MoleculeIO needs more specific stateSchema for atoms and atomicBonds. Specifically, create new IO Types rather than using ObjectLiteralIO.

@pixelzoom
Copy link
Contributor

This issue is > 2 years old. @jbphet is this issue still relevant?

@jbphet
Copy link
Contributor

jbphet commented Jan 29, 2024

Hey @samreid - there has been a lot of phet-io water under the bridge since you created this issue, and the GitHub history shows you as the author of Molecule.MoleculeIO. Could you please take a quick look at Molecule.MoleculeIO in the Molecule.js file, which is now in the greenhouse-effect repo, and let me know if this is still a relevant issue?

@jbphet jbphet assigned samreid and unassigned jbphet Jan 29, 2024
@samreid
Copy link
Member Author

samreid commented Jan 29, 2024

Yes, it still seems relevant. MoleculeIO has a schema listed like so:

Molecule.MoleculeIO = new IOType( 'MoleculeIO', {
  valueType: Molecule,
  toStateObject: molecule => molecule.toStateObject(),
  fromStateObject: Molecule.fromStateObject,
  stateSchema: {
    highElectronicEnergyState: BooleanIO,
    centerOfGravity: Vector2.Vector2IO,

    // TODO: https://github.com/phetsims/greenhouse-effect/issues/40 more specific schema
    atoms: ArrayIO( ObjectLiteralIO ),
    atomicBonds: ArrayIO( ObjectLiteralIO ),
    velocity: Vector2.Vector2IO,
    absorptionHysteresisCountdownTime: NumberIO,
    currentVibrationRadians: NumberIO,
    currentRotationRadians: NumberIO
  }
} );

The problem is that the atoms is just ArrayIO( ObjectLiteralIO) and atomicBonds is listed as ArrayIO( ObjectLiteralIO ). Ideally these would be more like:

    atoms: ArrayIO( Atom.AtomIO ),
    atomicBonds: ArrayIO( AtomicBond.AtomicBondIO ),

Where AtomIO and AtomicBondIO have the appropriate state schema listings. Having the correct type here will help with runtime validation, code maintainability and possible future migration. Either @zepumph or I would be happy to collaborate with you if you wish.

@samreid samreid assigned jbphet and unassigned samreid Jan 29, 2024
@jbphet
Copy link
Contributor

jbphet commented Jan 30, 2024

Thanks for the update @samreid.

I'm going to defer this issue. I just checked the published phet-io version of this sim at https://phet-io.colorado.edu/sims/molecules-and-light/1.3/wrappers/index/, and it looks quite old. The "Studio" wrapper isn't shown, which has been around for a long time now. I expect that there will be a moment in the future when this sim needs a full phet-io update, so I'll defer until that time. It just doesn't seem like it would be worth the time and effort to fix this up now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants