Assertion Failed: 'empty volume should be non-negative'

[zepumph]

@samreid and I were both able to reproduce this bug while fuzzing, but CT doesn't show it too much so I don't have a current stack trace.

[zepumph]

@samreid and I were able to trace it down to the ability for the boat to detect the block as a contained object incorrectly when you "hit" the top left corner of the block into the curved portion of the bottom right of the boat. This patch is on its way to helping us debug it. It isn't really clear why the logic in oneLiterShapeContainsPoint is so buggy sometimes. My best guess is that it is about the "stiffness" of objects in the p2 engine, and that for one or two p2 model steps, the objects are wildly overlapping in the physcis. That hasn't been confirmed yet.

``` Subject: [PATCH] Add table of contents, see https://github.com/phetsims/density-buoyancy-common/issues/167 --- Index: js/buoyancy/model/applications/BoatBasin.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/buoyancy/model/applications/BoatBasin.ts b/js/buoyancy/model/applications/BoatBasin.ts --- a/js/buoyancy/model/applications/BoatBasin.ts (revision 71fcc6e22f4485770b54b6da812d1cc6866d8725) +++ b/js/buoyancy/model/applications/BoatBasin.ts (date 1724272324095) @@ -43,9 +43,8 @@ if ( mass === this.boat || mass.stepBottom >= this.stepTop || mass.stepTop <= this.stepBottom - DensityBuoyancyCommonConstants.SLIP ) { return false; } - const stepMiddle = ( mass.stepTop + mass.stepBottom ) / 2; - return this.oneLiterShapeContainsPoint( new Vector2( mass.stepX, mass.stepBottom ) ) || - this.oneLiterShapeContainsPoint( new Vector2( mass.stepX, stepMiddle ) ); + // const stepMiddle = ( mass.stepTop + mass.stepBottom ) / 2; + return this.oneLiterShapeContainsPoint( new Vector2( mass.stepX, mass.stepBottom ) ); } /** @@ -53,12 +52,14 @@ * accounts for "slip", which occurs when two objects overlap a bit due to physics stiffness modeling. */ private oneLiterShapeContainsPoint( point: Vector2 ): boolean { - 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 = oneLiterPoint.plusXY( 0, DensityBuoyancyCommonConstants.SLIP ); - return ( this.oneLiterShape.bounds.containsPoint( oneLiterPoint ) || this.oneLiterShape.bounds.containsPoint( slippedPoint ) ) && - ( this.oneLiterShape.containsPoint( oneLiterPoint ) || this.oneLiterShape.containsPoint( slippedPoint ) ); + const testPoint = point.minus( this.boat.matrix.translation ); + return this.boat.shapeProperty.value.containsPoint( testPoint ); + // 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 = oneLiterPoint.plusXY( 0, DensityBuoyancyCommonConstants.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 71fcc6e22f4485770b54b6da812d1cc6866d8725) +++ b/js/common/model/DensityBuoyancyModel.ts (date 1724271348083) @@ -359,6 +359,10 @@ this.visibleMasses.forEach( mass => mass.updateStepInformation() ); basins.forEach( basin => { basin.stepMasses = this.visibleMasses.filter( mass => basin.isMassInside( mass ) ); + + if ( basin.constructor.name === 'BoatBasin' ) { + console.log( basin.stepMasses.length ); + } } ); // Check to see if fluid "spilled" out of the pool, and set the finalized fluid volume

[samreid]

Initial report with some more diagnostic info is in #350. Here is an updated patch:

```diff Subject: [PATCH] test --- Index: js/buoyancy/model/applications/BoatBasin.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/buoyancy/model/applications/BoatBasin.ts b/js/buoyancy/model/applications/BoatBasin.ts --- a/js/buoyancy/model/applications/BoatBasin.ts (revision 71fcc6e22f4485770b54b6da812d1cc6866d8725) +++ b/js/buoyancy/model/applications/BoatBasin.ts (date 1724277812249) @@ -43,22 +43,17 @@ if ( mass === this.boat || mass.stepBottom >= this.stepTop || mass.stepTop <= this.stepBottom - DensityBuoyancyCommonConstants.SLIP ) { return false; } - const stepMiddle = ( mass.stepTop + mass.stepBottom ) / 2; - return this.oneLiterShapeContainsPoint( new Vector2( mass.stepX, mass.stepBottom ) ) || - this.oneLiterShapeContainsPoint( new Vector2( mass.stepX, stepMiddle ) ); - } + const point = new Vector2( mass.stepX, mass.stepBottom + DensityBuoyancyCommonConstants.SLIP ); - /** - * 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 oneLiterPoint = point.minus( this.boat.matrix.translation ).timesScalar( 1 / this.boat.stepMultiplier ); - - // Check both a point slightly below AND the actual point. - const slippedPoint = oneLiterPoint.plusXY( 0, DensityBuoyancyCommonConstants.SLIP ); - return ( this.oneLiterShape.bounds.containsPoint( oneLiterPoint ) || this.oneLiterShape.bounds.containsPoint( slippedPoint ) ) && - ( this.oneLiterShape.containsPoint( oneLiterPoint ) || this.oneLiterShape.containsPoint( slippedPoint ) ); + // const testPoint = point.minus( this.boat.matrix.translation ); + // console.log( testPoint ); + const boatBounds = this.boat.shapeProperty.value.bounds.transformed( this.boat.matrix ); + // console.log( point, boatyShape.bounds ); + // const b = boatyShape.containsPoint( point ); + // console.log( 'contains: ' + b ); + const xContains = boatBounds.containsPoint( point ); + // console.log( 'bounds contains: ' + xContains ); + return xContains; } /** 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 71fcc6e22f4485770b54b6da812d1cc6866d8725) +++ b/js/common/model/DensityBuoyancyModel.ts (date 1724277584937) @@ -356,9 +356,15 @@ * @param assignableBasins - The basins that masses can be assigned to, note these must be specified in order of precedence */ protected updateFluidForBasins( basins: Basin[], assignableBasins: Basin[] ): void { + this.visibleMasses.forEach( mass => mass.updateStepInformation() ); + basins.forEach( basin => { basin.stepMasses = this.visibleMasses.filter( mass => basin.isMassInside( mass ) ); + + if ( basin.constructor.name === 'BoatBasin' ) { + console.log( basin.stepMasses.length ); + } } ); // Check to see if fluid "spilled" out of the pool, and set the finalized fluid volume ```

Oddly, in this patch, we often get boatShape.containsPoint evaluating to false, while the boatShape.bounds.containsPoint evaluating to true, even for a point where both should be true.

[samreid]

I thought it would be better to check one test point for the block, but we still have false positives when rapidly colliding the block with the boat:

```diff Subject: [PATCH] test --- Index: js/buoyancy/model/applications/BoatBasin.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/buoyancy/model/applications/BoatBasin.ts b/js/buoyancy/model/applications/BoatBasin.ts --- a/js/buoyancy/model/applications/BoatBasin.ts (revision 71fcc6e22f4485770b54b6da812d1cc6866d8725) +++ b/js/buoyancy/model/applications/BoatBasin.ts (date 1724278245705) @@ -43,22 +43,11 @@ if ( mass === this.boat || mass.stepBottom >= this.stepTop || mass.stepTop <= this.stepBottom - DensityBuoyancyCommonConstants.SLIP ) { return false; } - 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 oneLiterPoint = point.minus( this.boat.matrix.translation ).timesScalar( 1 / this.boat.stepMultiplier ); - - // Check both a point slightly below AND the actual point. - const slippedPoint = oneLiterPoint.plusXY( 0, DensityBuoyancyCommonConstants.SLIP ); - return ( this.oneLiterShape.bounds.containsPoint( oneLiterPoint ) || this.oneLiterShape.bounds.containsPoint( slippedPoint ) ) && - ( this.oneLiterShape.containsPoint( oneLiterPoint ) || this.oneLiterShape.containsPoint( slippedPoint ) ); + // The only way for a mass to be inside the basin is if its bottom center is in the boat bounds. + const point = new Vector2( mass.stepX, mass.stepBottom + DensityBuoyancyCommonConstants.SLIP ); + const boatBounds = this.boat.shapeProperty.value.bounds.transformed( this.boat.matrix ); + return boatBounds.containsPoint( point ); } /** @@ -74,6 +63,10 @@ public getMaximumVolume( y: number ): number { return this.boat.getBasinVolume( y ); } + + public override stepMassesUpdated(): void { + phet.chipper.queryParameters.dev && console.log( 'boat masses: ' + this.stepMasses.length ); + } } densityBuoyancyCommon.register( 'BoatBasin', BoatBasin ); \ No newline at end of file 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 71fcc6e22f4485770b54b6da812d1cc6866d8725) +++ b/js/common/model/DensityBuoyancyModel.ts (date 1724278106259) @@ -356,9 +356,12 @@ * @param assignableBasins - The basins that masses can be assigned to, note these must be specified in order of precedence */ protected updateFluidForBasins( basins: Basin[], assignableBasins: Basin[] ): void { + this.visibleMasses.forEach( mass => mass.updateStepInformation() ); + basins.forEach( basin => { basin.stepMasses = this.visibleMasses.filter( mass => basin.isMassInside( mass ) ); + basin.stepMassesUpdated(); } ); // Check to see if fluid "spilled" out of the pool, and set the finalized fluid volume Index: js/common/model/Basin.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/common/model/Basin.ts b/js/common/model/Basin.ts --- a/js/common/model/Basin.ts (revision 71fcc6e22f4485770b54b6da812d1cc6866d8725) +++ b/js/common/model/Basin.ts (date 1724278191473) @@ -181,6 +181,11 @@ ) ); } + public stepMassesUpdated(): void { + + // no op + } + /** * Resets to an initial state. */ ```

In other places, we have accepted that the p2 model has slip and uncontrollable error circumstances. Perhaps the solution is to make getEmptyVolume more tolerant.

[samreid]

@zepumph and I discussed the commit above and decided to go with it, closing.