phetsims / buoyancy

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

Force display is buggy when lifting a single block #140

Open samreid opened 1 month ago

samreid commented 1 month ago

Before starting on https://github.com/phetsims/density-buoyancy-common/issues/111, I observed the force display on the block is haywire. Here is a video:

https://github.com/phetsims/buoyancy/assets/679486/0a0cb19c-ef0c-46b5-bd2d-305c7a99bd11

This is a new problem and looks serious so I will prioritize this issue on the project board.

samreid commented 1 month ago

Has this always been broken? I've bisected back to March 18 and it is still buggy.

zepumph commented 1 month ago

@AgustinVallejo @samreid and I discussed this, and we can see that the contact force vector is very eratic while dragging with a mouse. We think this is because we are incomplete about how we are writing our force diagram. We don't show the pointer contraint force arrow. But p2 is still including the pointer force's contact force as part of the main contact force. This makes the diagram unbalanced, AND quite flickery.

To proceed, here are some ideas:

zepumph commented 1 month ago

Let's see if we can use some sort of average/median of the contact force interpolated property. That will get rid of some of the noise from rapid acceleration/deceleration from the mouse/keyboard input.

zepumph commented 3 weeks ago

Here is a patch of some logging I was doing to try to understand how InterpolatedProperty works.

I don't really understand how we have 10 or so model runs in between steps, but then all the previous values are ignored when calculated the actual value based on a ratio. The ratio is only from the most recent section, so basically, all other items since last draw are ignored. Perhaps most of the time that is fine, but for the force arrows, I think it would be good to figure out some sort of average, and the ratio only applies to the very last value.

```diff Subject: [PATCH] reset poolScaleHeightProperty, https://github.com/phetsims/density-buoyancy-common/issues/107 --- Index: js/buoyancy/model/BuoyancyLabModel.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/buoyancy/model/BuoyancyLabModel.ts b/js/buoyancy/model/BuoyancyLabModel.ts --- a/js/buoyancy/model/BuoyancyLabModel.ts (revision c25b8ba0a25f8ff7bd4b9f87d5cba2065a70da7c) +++ b/js/buoyancy/model/BuoyancyLabModel.ts (date 1713826219983) @@ -46,6 +46,8 @@ } ); this.availableMasses.push( this.primaryMass ); + this.primaryMass.hi = true; + this.primaryMass.contactForceInterpolatedProperty.hi = true; // Left scale this.availableMasses.push( new Scale( this.engine, this.gravityProperty, { matrix: Matrix3.translation( -0.65, -Scale.SCALE_BASE_BOUNDS.minY ), 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 c25b8ba0a25f8ff7bd4b9f87d5cba2065a70da7c) +++ b/js/common/model/Mass.ts (date 1713825270420) @@ -462,6 +462,10 @@ phetioHighFrequency: true } ); + + if ( this.hi ) { + this.contactForceInterpolatedProperty.debug( 'contact' ); + } this.forceOffsetProperty = new Property( Vector3.ZERO, { valueType: Vector3, valueComparisonStrategy: 'equalsFunction', @@ -658,6 +662,7 @@ this.transformedEmitter.emit(); + this.contactForceInterpolatedProperty.value.x !== 0 && this.hi && console.log( 'ratio', interpolationRatio ); this.contactForceInterpolatedProperty.setRatio( interpolationRatio ); this.buoyancyForceInterpolatedProperty.setRatio( interpolationRatio ); this.gravityForceInterpolatedProperty.setRatio( interpolationRatio ); 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 c25b8ba0a25f8ff7bd4b9f87d5cba2065a70da7c) +++ b/js/common/model/DensityBuoyancyModel.ts (date 1713825288933) @@ -304,6 +304,9 @@ if ( !contactForce.isFinite() ) { contactForce = Vector2.ZERO; } + if ( mass.hi && contactForce.x !== 0 ) { + console.log( 'model step', contactForce ); + } this.engine.resetContactForces( mass.body ); mass.contactForceInterpolatedProperty.setNextValue( contactForce ); 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 c25b8ba0a25f8ff7bd4b9f87d5cba2065a70da7c) +++ b/js/common/model/InterpolatedProperty.ts (date 1713826523810) @@ -17,7 +17,7 @@ import densityBuoyancyCommon from '../../densityBuoyancyCommon.js'; import IOTypeCache from '../../../../tandem/js/IOTypeCache.js'; -type Interpolate = ( a: T, b: T, ratio: number ) => T; +type Interpolate = ( a: T, b: T, ratio: number, allPrevious: T[] ) => T; type SelfOptions = { interpolate: Interpolate; }; @@ -27,6 +27,7 @@ public currentValue: T; public previousValue: T; + private allPreviousValues: T[] = []; public ratio: number; private readonly interpolate: Interpolate; @@ -52,6 +53,7 @@ */ public setNextValue( value: T ): void { this.previousValue = this.currentValue; + this.allPreviousValues.push( this.currentValue ); this.currentValue = value; } @@ -61,7 +63,11 @@ public setRatio( ratio: number ): void { this.ratio = ratio; - this.value = this.interpolate( this.previousValue, this.currentValue, this.ratio ); + const interpolated = this.interpolate( this.previousValue, this.currentValue, this.ratio, this.allPreviousValues, this.hi ); + this.value = interpolated; + this.hi && interpolated.x !== 0 && console.log( interpolated ); + this.allPreviousValues.length = 0; + this.hi && interpolated.x !== 0 && console.log( '\n\n\n' ); } /** @@ -72,27 +78,36 @@ this.currentValue = this.value; this.previousValue = this.value; + this.allPreviousValues.length = 0; this.ratio = 0; } /** * Interpolation for numbers. */ - public static interpolateNumber( a: number, b: number, ratio: number ): number { + public static interpolateNumber( a: number, b: number, ratio: number, allPrevious: number[] ): number { return a + ( b - a ) * ratio; } /** * Interpolation for Vector2. */ - public static interpolateVector2( a: Vector2, b: Vector2, ratio: number ): Vector2 { + public static interpolateVector2( a: Vector2, b: Vector2, ratio: number, allPrevious: Vector2[], special: boolean ): Vector2 { + + if ( special && a.x !== 0 ) { + const vector2 = _.reduce( allPrevious, ( sum, current ) => { + return sum.plus( current ); + }, new Vector2( 0, 0 ) ); + console.log( 'average', vector2.dividedScalar( allPrevious.length ) ); + return vector2; + } return a.blend( b, ratio ); } /** * Interpolation for Vector3. */ - public static interpolateVector3( a: Vector3, b: Vector3, ratio: number ): Vector3 { + public static interpolateVector3( a: Vector3, b: Vector3, ratio: number, allPrevious: Vector3[] ): Vector3 { return a.blend( b, ratio ); }
AgustinVallejo commented 23 hours ago

I don't really understand how we have 10 or so model runs in between steps

Not sure if this is related, but when working on https://github.com/phetsims/buoyancy/issues/64, I logged the flickering values, corresponding to this vertical forces, and they seem to be periodical. I'm wondering if having multiple model runs between steps could be causing small inconsistencies between forces.

The following is a chatgpt made chart with a subset of the logging:

image

Period is exactly 11, so this theory is growing on me.