search

phetsims / states-of-matter

"States of Matter" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
7 stars 8 forks source link

Should MoleculeDataSet serialize `nextMoleculeTorques` too? #366

Open zepumph opened 11 months ago

zepumph commented 11 months ago

While working on https://github.com/phetsims/states-of-matter/issues/365, I saw that this property wasn't being set by PhET-iO state. Is this by design or is it a bug? Not a high priority thing or anything. I can't see problems in this area.

```diff Subject: [PATCH] recurse into ArrayIOs and MapIOs bug fix, https://github.com/phetsims/phet-io-wrappers/issues/630 --- Index: js/common/model/MoleculeForceAndMotionDataSet.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/common/model/MoleculeForceAndMotionDataSet.js b/js/common/model/MoleculeForceAndMotionDataSet.js --- a/js/common/model/MoleculeForceAndMotionDataSet.js (revision 352b4c5f639c9c0dd2cf3c427ed84b76661d44ee) +++ b/js/common/model/MoleculeForceAndMotionDataSet.js (date 1702062627566) @@ -424,6 +424,7 @@ const moleculeRotationAngles = Float64ArrayIO.fromStateObject( stateObject.moleculeRotationAngles ); const moleculeRotationRates = Float64ArrayIO.fromStateObject( stateObject.moleculeRotationRates ); const moleculeTorques = Float64ArrayIO.fromStateObject( stateObject.moleculeTorques ); + const nextMoleculeTorques = Float64ArrayIO.fromStateObject( stateObject.nextMoleculeTorques ); const insideContainer = ArrayIOBooleanIO.fromStateObject( stateObject.insideContainer ); for ( let i = 0; i < this.numberOfMolecules; i++ ) { this.moleculeCenterOfMassPositions[ i ] = moleculeCenterOfMassPositions[ i ]; @@ -433,6 +434,7 @@ this.moleculeRotationAngles[ i ] = moleculeRotationAngles[ i ]; this.moleculeRotationRates[ i ] = moleculeRotationRates[ i ]; this.moleculeTorques[ i ] = moleculeTorques[ i ]; + this.nextMoleculeTorques[ i ] = nextMoleculeTorques[ i ]; this.insideContainer[ i ] = insideContainer[ i ]; } }
jbphet commented 11 months ago

You raise a good question. This code was written a long time ago, so I can't really trust my memories about it, but since I don't recall any reasons to have omitted nextMoleculeTorques and there is no documentation about it, the most reasonable conclusion is that it was simply an oversight. I tested the patch above with some basic setting of state for the diatomic and water cases (this data would be irrelevant to the single-atom cases), and everything seemed fine, so I have committed it.

I'm going to mark this issue as resolved, but I think we should give it special scrutiny during the next release of this sim from the main branch, so I'll leave it open for that purpose.