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

Unwanted buoyant force due to a thin layer of liquid inside of the boat #135

Closed DianaTavares closed 1 month ago

DianaTavares commented 1 month ago

I hope this issue is not twice because we commented in a previous design meeting that the Buoyancy force that the blocks have when they are inside the boats, and it doesn't have fluid, should not be there:

image

That 0.14N in the block should not be there. It affects, for example, the calculation of the max mass that the boat can carry because that force increases with the size, like an "imaginary" fluid is inside the boat:

image

and change when I change the fluid density: image

samreid commented 1 month ago

@AgustinVallejo asked me to take a look at this. Solo assigning.

samreid commented 1 month ago

This patch demonstrates that the boat thinks it has liquid in the BoatBasin when it shouldn't:

```diff Subject: [PATCH] Improve HorizontalCylinder.intersect, see https://github.com/phetsims/buoyancy/issues/103 --- Index: js/buoyancy/model/Boat.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/buoyancy/model/Boat.ts b/js/buoyancy/model/Boat.ts --- a/js/buoyancy/model/Boat.ts (revision 41e469c829790ed41a47bfee46d42d6351b89881) +++ b/js/buoyancy/model/Boat.ts (date 1713287916297) @@ -143,6 +143,11 @@ this.basin.stepTop = this.stepTop; this.basin.stepBottom = yOffset + this.stepMultiplier * BoatDesign.ONE_LITER_INTERIOR_BOTTOM; + + console.log( this.basin.stepBottom ); + if ( this.basin.stepBottom !== 0 ) { + debugger; + } } /** ```

That value for this.basin.stepBottom is leading liquidYInterpolatedProperty to become nonzero in the interpolation step even though the boat should have no liquid in it.

I experimented with skipping mutating stepBottom and stepTop but it led to other buggy behavior.

I do not know how to proceed and recommend collaboration.

zepumph commented 1 month ago

@samreid and I were able to find a fix for this bug. In addition, we found 2 other potential issues with how the model was treating and setting Mass.containingBasin. Here is a patch for @samreid to look over before committing. I believe we are close, but I want to regression test more generally before committing.

```diff Subject: [PATCH] doc, https://github.com/phetsims/buoyancy/issues/135 --- Index: js/common/model/Pool.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/common/model/Pool.ts b/js/common/model/Pool.ts --- a/js/common/model/Pool.ts (revision 79b4dd49f166d1879335b073e4c451ae40edeb3b) +++ b/js/common/model/Pool.ts (date 1713300418170) @@ -40,6 +40,7 @@ * liquid). */ public isMassInside( mass: Mass ): boolean { + // TODO: Why not care about horizontal positioning? https://github.com/phetsims/buoyancy/issues/135 return mass.stepBottom < this.stepTop; } Index: js/buoyancy/model/BoatBasin.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/buoyancy/model/BoatBasin.ts b/js/buoyancy/model/BoatBasin.ts --- a/js/buoyancy/model/BoatBasin.ts (revision 79b4dd49f166d1879335b073e4c451ae40edeb3b) +++ b/js/buoyancy/model/BoatBasin.ts (date 1713300327252) @@ -39,16 +39,27 @@ * liquid). */ public isMassInside( mass: Mass ): boolean { - const slip = 1e-2; + const slip = 0.01; // 1 cm of potential overlap due to physics stiffness variables. if ( mass === this.boat || mass.stepBottom >= this.stepTop || mass.stepTop <= this.stepBottom - slip ) { return false; } - const oneLiterBottomPoint = new Vector2( mass.stepX, mass.stepBottom ).minus( this.boat.matrix.translation ).timesScalar( 1 / this.boat.stepMultiplier ); + const stepMiddle = ( mass.stepTop + mass.stepBottom ) / 2; + return this.oneLiterShapeContainsPoint( new Vector2( mass.stepX, mass.stepBottom ) ) || this.oneLiterShapeContainsPoint( new Vector2( mass.stepX, stepMiddle ) ); + } + + /** + * Factored out way to take a point in absolute model coordinates, and determine if it is contained in the boat. This + * accounts for "slip", which occurs when two objects overlap a bit due to physics stiffness modeling. + */ + private oneLiterShapeContainsPoint( point: Vector2 ): boolean { + const slip = 0.01; // 1 cm of potential overlap due to physics stiffness variables. + + const oneLiterPoint = point.minus( this.boat.matrix.translation ).timesScalar( 1 / this.boat.stepMultiplier ); // Check both a point slightly below AND the actual point. - const slippedPoint = oneLiterBottomPoint.plusXY( 0, slip ); - return ( this.oneLiterShape.bounds.containsPoint( oneLiterBottomPoint ) || this.oneLiterShape.bounds.containsPoint( slippedPoint ) ) && - ( this.oneLiterShape.containsPoint( oneLiterBottomPoint ) || this.oneLiterShape.containsPoint( slippedPoint ) ); + const slippedPoint = oneLiterPoint.plusXY( 0, slip ); + return ( this.oneLiterShape.bounds.containsPoint( oneLiterPoint ) || this.oneLiterShape.bounds.containsPoint( slippedPoint ) ) && + ( this.oneLiterShape.containsPoint( oneLiterPoint ) || this.oneLiterShape.containsPoint( slippedPoint ) ); } /** 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 79b4dd49f166d1879335b073e4c451ae40edeb3b) +++ b/js/common/model/DensityBuoyancyModel.ts (date 1713300368494) @@ -330,8 +330,16 @@ } const basin = mass.containingBasin; - const submergedVolume = basin ? mass.getDisplacedVolume( basin.liquidYInterpolatedProperty.currentValue ) : 0; - if ( submergedVolume ) { + + let submergedVolume = 0; + if ( basin ) { + const displacedVolume = mass.getDisplacedVolume( basin.liquidYInterpolatedProperty.currentValue ); + + // The submergedVolume of the mass cannot be more than the liquid volume in the basin. Bug fix for https://github.com/phetsims/buoyancy/issues/135 + submergedVolume = displacedVolume > basin.liquidVolumeProperty.value ? basin.liquidVolumeProperty.value : displacedVolume; + } + + if ( submergedVolume !== 0 ) { const displacedMass = submergedVolume * this.liquidDensityProperty.value; // Vertical acceleration of the boat will change the buoyant force. const acceleration = gravity + ( ( boat && basin === boat.basin ) ? boatVerticalAcceleration : 0 );
zepumph commented 1 month ago

We fixed this bug, found two more, and fixed one of those. The last one is in a side issue. Closing.