zepumph
commented
5 months ago I was going to commit this, but ran into trouble when a multilink in the view updates because of the liquidVolumeProperty changes during model steps and updates in the view eagerly. This triggers a get() on the InterpolationProperty. This seems buggy to me, since we are mid, engine stepping, the interpolated value will be outdated. This intermediate state may be acceptable, but I couldn't be sure immediately.
```diff
Subject: [PATCH] update doc, https://github.com/phetsims/buoyancy/issues/64
---
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 1705c5a5f4b76ec587233cfd62b77e1c1ce5dd53)
+++ b/js/common/model/DensityBuoyancyModel.ts (date 1716322373251)
@@ -35,6 +35,7 @@
import { PhetioObjectOptions } from '../../../../tandem/js/PhetioObject.js';
import PickRequired from '../../../../phet-core/js/types/PickRequired.js';
import Matrix3 from '../../../../dot/js/Matrix3.js';
+import InterpolatedProperty from './InterpolatedProperty.js';
// constants
const BLOCK_SPACING = 0.01;
@@ -563,6 +564,7 @@
* Steps forward in time. This is a "simulation step", not a "physics engine step"
*/
public step( dt: number ): void {
+ InterpolatedProperty.lock();
this.engine.step( dt );
this.masses.forEach( mass => {
@@ -570,6 +572,7 @@
} );
this.pool.liquidYInterpolatedProperty.setRatio( this.engine.interpolationRatio );
+ InterpolatedProperty.unlock();
}
/**
Index: js/common/model/InterpolatedProperty.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/model/InterpolatedProperty.ts b/js/common/model/InterpolatedProperty.ts
--- a/js/common/model/InterpolatedProperty.ts (revision 1705c5a5f4b76ec587233cfd62b77e1c1ce5dd53)
+++ b/js/common/model/InterpolatedProperty.ts (date 1716333246385)
@@ -28,6 +28,10 @@
};
export type InterpolatedPropertyOptions = SelfOptions & PropertyOptions;
+// Lock the ability to read the value of InterpolatedProperties. This is because the interpolated value can be buggy
+// if read by certain functionality. Instead, the interpolated value is just for the rendering portion, not the model
+let readLockCount = 0;
+
export default class InterpolatedProperty extends Property {
// The most recently set value, but changing this will not fire listeners.
@@ -69,10 +73,21 @@
public setRatio( ratio: number ): void {
this.ratio = ratio;
+ const lockCount = readLockCount;
+ InterpolatedProperty.unlock();
+
// Interpolating between two values ago and the most recent value is a common heuristic to do, but it is important
// that no model code relies on this, and instead would just use currentValue directly. This is also duplicated from
// what p2 World does after step.
this.value = this.interpolate( this.previousValue, this.currentValue, this.ratio );
+
+ InterpolatedProperty.lock();
+ assert && assert( lockCount === readLockCount, 'InterpolatedProperty does not support reading other InterpolatedProperties from value set' );
+ }
+
+ public override get(): T {
+ assert && assert( readLockCount === 0, 'Cannot access InterpolatedProperty value when locked. Are you in the model?' );
+ return super.get();
}
/**
@@ -107,6 +122,14 @@
return a.blend( b, ratio );
}
+ public static lock(): void {
+ readLockCount++;
+ }
+
+ public static unlock(): void {
+ readLockCount--;
+ }
+
public static readonly InterpolatedPropertyIO = ( parameterType: IOType ): IOType => {
assert && assert( parameterType, 'InterpolatedPropertyIO needs parameterType' );
AgustinVallejo
samreid
From https://github.com/phetsims/buoyancy/issues/64, @samreid and I viewed that InterpolatedProperty instances were being read from inside the engine model step's postStepListener. This is generally a bug, and should instead use the
currentValue. We think an assertion will help prevent future errors like this.Perhaps by wrapping the DBC model step with an
InterpolatedProperty.lock/unlock()functionality.