phetsims / density-buoyancy-common

Common code for the Density and Buoyancy simulations
GNU General Public License v3.0
0 stars 2 forks source link

Mass.writeData is sometimes called without calling `transformedEmitter` #231

Closed zepumph closed 1 month ago

zepumph commented 3 months ago

While reviewing https://github.com/phetsims/density-buoyancy-common/issues/201, it seems that it could be buggy to not be consistent about calling these together. I noted a couple of spots that seemed buggy with TODOs, mostly in updateSize() calls. Cuboid does call transformedEmitter, but not Duck or Boat.

zepumph commented 3 months ago

Furthermore, I'm tempted to create something like Mass.save() that calls writeData and transformedEmitter() together.

samreid commented 1 month ago

How about putting transformedEmitter.emit at the end of writeData? This patch seems pretty safe but I also noted one TODO:

```diff Subject: [PATCH] Use gravityValueProperty where possible, see https://github.com/phetsims/density-buoyancy-common/issues/312 --- Index: js/common/view/PoolScaleHeightControl.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/common/view/PoolScaleHeightControl.ts b/js/common/view/PoolScaleHeightControl.ts --- a/js/common/view/PoolScaleHeightControl.ts (revision b46ede89c8c2020f85eb4701c41b2add5f182494) +++ b/js/common/view/PoolScaleHeightControl.ts (date 1722718563061) @@ -103,7 +103,6 @@ poolScale.matrix.set02( SCALE_X_POSITION ); poolScale.matrix.set12( currentHeight + Scale.SCALE_HEIGHT / 2 ); poolScale.writeData(); - poolScale.transformedEmitter.emit(); } ); thumbNode.touchArea = thumbInteractionArea.copy().rect( -10, -10, 20, 50 ); Index: js/common/model/DensityBuoyancyModel.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/common/model/DensityBuoyancyModel.ts b/js/common/model/DensityBuoyancyModel.ts --- a/js/common/model/DensityBuoyancyModel.ts (revision b46ede89c8c2020f85eb4701c41b2add5f182494) +++ b/js/common/model/DensityBuoyancyModel.ts (date 1722718563057) @@ -248,7 +248,6 @@ otherMass.matrix.set02( mass.matrix.m02() - delta ); otherMass.writeData(); - otherMass.transformedEmitter.emit(); this.engine.bodySynchronizePrevious( otherMass.body ); } @@ -410,7 +409,6 @@ ); position -= BLOCK_SPACING + mass.sizeProperty.value.width; mass.writeData(); - mass.transformedEmitter.emit(); } ); } @@ -427,7 +425,6 @@ ); position += BLOCK_SPACING + mass.sizeProperty.value.width; mass.writeData(); - mass.transformedEmitter.emit(); } ); } @@ -462,7 +459,6 @@ position += mass.sizeProperty.value.height; mass.writeData(); this.engine.bodySynchronizePrevious( mass.body ); - mass.transformedEmitter.emit(); } ); } Index: js/common/model/Mass.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/common/model/Mass.ts b/js/common/model/Mass.ts --- a/js/common/model/Mass.ts (revision b46ede89c8c2020f85eb4701c41b2add5f182494) +++ b/js/common/model/Mass.ts (date 1722718563065) @@ -474,6 +474,8 @@ public writeData(): void { this.engine.bodySetPosition( this.body, this.matrix.translation.minus( this.bodyOffsetProperty.value ) ); this.engine.bodySetRotation( this.body, this.matrix.rotation ); + + this.transformedEmitter.emit(); } /** @@ -525,7 +527,6 @@ public setPosition( x: number, y: number ): void { this.matrix.setToTranslation( x, y ); this.writeData(); - this.transformedEmitter.emit(); } /** @@ -534,8 +535,6 @@ public step( dt: number, interpolationRatio: number ): void { this.readData(); - this.transformedEmitter.emit(); - this.contactForceInterpolatedProperty.setRatio( interpolationRatio ); this.buoyancyForceInterpolatedProperty.setRatio( interpolationRatio ); this.gravityForceInterpolatedProperty.setRatio( interpolationRatio ); @@ -550,7 +549,6 @@ this.matrix.set( this.originalMatrix ); this.writeData(); this.engine.bodySynchronizePrevious( this.body ); - this.transformedEmitter.emit(); } /** Index: js/buoyancy/model/shapes/BuoyancyShapesModel.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/buoyancy/model/shapes/BuoyancyShapesModel.ts b/js/buoyancy/model/shapes/BuoyancyShapesModel.ts --- a/js/buoyancy/model/shapes/BuoyancyShapesModel.ts (revision b46ede89c8c2020f85eb4701c41b2add5f182494) +++ b/js/buoyancy/model/shapes/BuoyancyShapesModel.ts (date 1722718563053) @@ -100,7 +100,6 @@ newMass.matrix.set( oldMass.matrix ); } newMass.writeData(); - newMass.transformedEmitter.emit(); if ( this.availableMasses.includes( oldMass ) ) { this.availableMasses.remove( oldMass ); Index: js/common/model/Cuboid.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/common/model/Cuboid.ts b/js/common/model/Cuboid.ts --- a/js/common/model/Cuboid.ts (revision b46ede89c8c2020f85eb4701c41b2add5f182494) +++ b/js/common/model/Cuboid.ts (date 1722718626288) @@ -82,6 +82,7 @@ this.forceOffsetProperty.value = new Vector3( 0, 0, size.maxZ ); this.massLabelOffsetProperty.value = new Vector3( size.minX, size.minY, size.maxZ ); + // TODO: This is pretty far from the automatic transformedEmitter that happened in writeData above. Is it necessary? this.transformedEmitter.emit(); } } Index: js/density/model/DensityMysteryModel.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/density/model/DensityMysteryModel.ts b/js/density/model/DensityMysteryModel.ts --- a/js/density/model/DensityMysteryModel.ts (revision b46ede89c8c2020f85eb4701c41b2add5f182494) +++ b/js/density/model/DensityMysteryModel.ts (date 1722718626293) @@ -496,8 +496,6 @@ // Adjust its previous position also this.engine.bodySynchronizePrevious( this.scale.body ); - - this.scale.transformedEmitter.emit(); } ); } Index: js/buoyancy/model/shapes/BuoyancyShapeModel.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/buoyancy/model/shapes/BuoyancyShapeModel.ts b/js/buoyancy/model/shapes/BuoyancyShapeModel.ts --- a/js/buoyancy/model/shapes/BuoyancyShapeModel.ts (revision b46ede89c8c2020f85eb4701c41b2add5f182494) +++ b/js/buoyancy/model/shapes/BuoyancyShapeModel.ts (date 1722718563049) @@ -84,7 +84,6 @@ const minYAfter = this.shapeProperty.value.getBounds().minY; this.shapeProperty.value.matrix.multiplyMatrix( Matrix3.translation( 0, minYBefore - minYAfter ) ); this.shapeProperty.value.writeData(); - this.shapeProperty.value.transformedEmitter.emit(); } public reset(): void {
samreid commented 1 month ago

Fixed, closing.