Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move boat to BuoyancyApplicationsModel #234

Closed
samreid opened this issue Jun 24, 2024 · 6 comments
Closed

Move boat to BuoyancyApplicationsModel #234

samreid opened this issue Jun 24, 2024 · 6 comments

Comments

@samreid
Copy link
Member

samreid commented Jun 24, 2024

On Friday @zepumph and I discussed that there are modularity problems, including but not limited to cases where base classes know too much about unrelated subclasses. This is partly what is causing #172 and making the simulation more difficult to understand and maintain. One step we can take to improve this would be to move boat from DensityBuoyancyModel to BuoyancyApplicationsModel. I'll take a look.

@samreid samreid self-assigned this Jun 24, 2024
@samreid
Copy link
Member Author

samreid commented Jun 24, 2024

Working copy:

Subject: [PATCH] Do not show the pool scale when resetting the boat scene, see https://github.com/phetsims/buoyancy/issues/179
---
Index: js/buoyancy/model/applications/BuoyancyApplicationsModel.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/buoyancy/model/applications/BuoyancyApplicationsModel.ts b/js/buoyancy/model/applications/BuoyancyApplicationsModel.ts
--- a/js/buoyancy/model/applications/BuoyancyApplicationsModel.ts	(revision 2a2232fae9909676ede8b4116680d9b3e89bac4f)
+++ b/js/buoyancy/model/applications/BuoyancyApplicationsModel.ts	(date 1719241175312)
@@ -19,6 +19,18 @@
 import { BottleOrBoat, BottleOrBoatValues } from './BottleOrBoat.js';
 import StringUnionProperty from '../../../../../axon/js/StringUnionProperty.js';
 import MassTag from '../../../common/model/MassTag.js';
+import Basin from '../../../common/model/Basin.js';
+import Mass from '../../../common/model/Mass.js';
+
+// Faster than normal stepping to fill the boat (kind of like animation speed)
+const FILL_EMPTY_MULTIPLIER = 0.3;
+
+// 90% of the boat is out of the water before spilling out the full boat
+const BOAT_READY_TO_SPILL_OUT_THRESHOLD = 0.9;
+
+// Y model distance of tolerance between the boat basin fluidY level and the boat basin stepTop. This was needed to
+// prevent filling thrashing as a containing mass floats around. See updateLiquidLevel();
+const BOAT_FULL_THRESHOLD = 0.01;
 
 export type BuoyancyApplicationsModelOptions = DensityBuoyancyModelOptions;
 
@@ -31,6 +43,9 @@
   public readonly boat: Boat;
   private readonly scale: Scale; // Scale sitting on the ground next to the pool
 
+  // Flag that sets an animation to empty the boat of any water inside of it
+  private spillingWaterOutOfBoat = false;
+
   public constructor( options: BuoyancyApplicationsModelOptions ) {
 
     const tandem = options.tandem;
@@ -103,6 +118,20 @@
       assert && assert( !this.boat.visibleProperty.value || !this.bottle.visibleProperty.value,
         'Boat and bottle should not be visible at the same time' );
     } );
+
+    let boatVerticalVelocity = 0;
+    let boatVerticalAcceleration = 0;
+
+    this.postStepPhase1Emitter.addListener( dt => {
+      const boat = this.boat;
+
+      if ( dt ) {
+        boat.setUnderwaterState( this.pool.fluidYInterpolatedProperty.currentValue );
+        const nextBoatVerticalVelocity = this.engine.bodyGetVelocity( boat.body ).y;
+        boatVerticalAcceleration = ( nextBoatVerticalVelocity - boatVerticalVelocity ) / dt;
+        boatVerticalVelocity = nextBoatVerticalVelocity;
+      }
+    } );
   }
 
   public override step( dt: number ): void {
@@ -112,10 +141,6 @@
     super.step( dt );
   }
 
-  public override getBoat(): Boat | null {
-    return this.boat;
-  }
-
   /**
    * Moves the boat and block to their initial locations (see https://github.com/phetsims/buoyancy/issues/25)
    */
@@ -143,11 +168,147 @@
 
     super.reset();
 
+    this.spillingWaterOutOfBoat = false;
+
     this.sceneProperty.reset();
 
     assert && assert( !this.boat.visibleProperty.value || !this.bottle.visibleProperty.value,
       'Boat and bottle should not be visible at the same time' );
   }
+
+  private updateFluidsForBoat( poolFluidVolume: number ): number {
+    const boat = this.boat;
+
+    assert && assert( boat, 'boat needed to update liquids for boat' );
+
+    const boatBasin = boat.basin;
+    if ( boat.visibleProperty.value ) {
+      let boatFluidVolume = boatBasin.fluidVolumeProperty.value;
+      const boatBasinMaximumVolume = boatBasin.getMaximumVolume( boatBasin.stepTop );
+
+      const poolEmptyVolumeToBoatTop = this.pool.getEmptyVolume( Math.min( boat.stepTop, this.poolBounds.maxY ) );
+      const boatEmptyVolumeToBoatTop = boatBasin.getEmptyVolume( boat.stepTop );
+
+      // Calculate adjustments to water volumes to match the current space in the basin
+      let poolExcess = poolFluidVolume - poolEmptyVolumeToBoatTop;
+      let boatExcess = boatFluidVolume - boatEmptyVolumeToBoatTop;
+
+      const boatHeight = boat.shapeProperty.value.getBounds().height;
+
+      if ( boatFluidVolume ) {
+
+        // If the top of the boat is out of the water past the height threshold, spill the water back into the pool
+        // (even if not totally full).
+        if ( boat.stepTop > this.pool.fluidYInterpolatedProperty.currentValue + boatHeight * BOAT_READY_TO_SPILL_OUT_THRESHOLD ) {
+          this.spillingWaterOutOfBoat = true;
+        }
+      }
+      else {
+        // If the boat is empty, stop spilling
+        this.spillingWaterOutOfBoat = false;
+      }
+
+      // If the boat is out of the water, spill the water back into the pool
+      if ( this.spillingWaterOutOfBoat ) {
+        boatExcess = Math.min( FILL_EMPTY_MULTIPLIER * boat.volumeProperty.value, boatFluidVolume );
+      }
+      else if ( boatFluidVolume > 0 &&
+                Math.abs( boatBasin.fluidYInterpolatedProperty.currentValue - boatBasin.stepTop ) >= BOAT_FULL_THRESHOLD ) {
+        // If the boat is neither full nor empty, nor spilling, then it is currently filling up. We will up no matter
+        // the current water leve or the boat AND no matter the boats position. This is because the boat can only
+        // ever be full or empty (or animating to one of those states).
+
+        const excess = Math.min( FILL_EMPTY_MULTIPLIER * boat.volumeProperty.value, boatBasinMaximumVolume - boatFluidVolume ); // This animates the boat spilling in
+        poolExcess = excess;
+        boatExcess = -excess;
+      }
+
+      if ( poolExcess > 0 && boatExcess < 0 ) {
+        const transferVolume = Math.min( poolExcess, -boatExcess );
+        poolFluidVolume -= transferVolume;
+        boatFluidVolume += transferVolume;
+      }
+      else if ( boatExcess > 0 ) {
+        // If the boat overflows, just dump the rest in the pool
+        poolFluidVolume += boatExcess;
+        boatFluidVolume -= boatExcess;
+      }
+      boatBasin.fluidVolumeProperty.value = boatFluidVolume;
+    }
+    else {
+      boatBasin.fluidVolumeProperty.value = 0;
+    }
+    return poolFluidVolume;
+  }
+
+  public override overrideSubmergedVolume( mass: Mass, submergedVolume: number ): number {
+    const boat = this.boat;
+    if ( mass === boat && boat.isUnderwater ) {
+
+      // Special consideration for when boat is underwater
+      // Don't count the liquid inside the boat as part of the mass
+      return boat.volumeProperty.value;
+
+    }
+    else {
+      return submergedVolume;
+    }
+  }
+
+  public override overrideMassValue( submergedVolume: number, massValue: number ): number {
+    return submergedVolume * this.boat.materialProperty.value.density;
+  }
+
+
+  /**
+   * Computes the heights of the main pool liquid (and optionally that of the boat)
+   * NOTE: This does not call the super class method, as it needs to manage the sequencing of the actions, hence
+   * there is duplicated code in this implementation.
+   */
+  protected override updateFluid(): void {
+    const boat = this.boat;
+
+    const basins: Basin[] = [ this.pool ];
+    if ( boat && boat.visibleProperty.value ) {
+      basins.push( boat.basin );
+      this.pool.childBasin = boat.basin;
+    }
+    else {
+      this.pool.childBasin = null;
+    }
+
+    this.masses.forEach( mass => mass.updateStepInformation() );
+    basins.forEach( basin => {
+      basin.stepMasses = this.masses.filter( mass => basin.isMassInside( mass ) );
+    } );
+
+    let poolFluidVolume = this.pool.fluidVolumeProperty.value;
+
+    // May need to adjust volumes between the boat/pool if there is a boat
+    if ( boat ) {
+      poolFluidVolume = this.updateFluidsForBoat( poolFluidVolume );
+    }
+
+    // Check to see if water "spilled" out of the pool, and set the finalized liquid volume
+    this.pool.fluidVolumeProperty.value = Math.min( poolFluidVolume, this.pool.getEmptyVolume( this.poolBounds.maxY ) );
+
+    this.pool.computeY();
+    boat && boat.basin.computeY();
+
+    // If we have a boat that is NOT underwater, we'll assign masses into the boat's basin where relevant. Otherwise,
+    // anything will go just into the pool's basin.
+    if ( boat && boat.visibleProperty.value && !boat.isUnderwater ) {
+      this.masses.forEach( mass => {
+        mass.containingBasin = boat.basin.isMassInside( mass ) ? boat.basin : ( this.pool.isMassInside( mass ) ? this.pool : null );
+      } );
+    }
+    else {
+      this.masses.forEach( mass => {
+        mass.containingBasin = this.pool.isMassInside( mass ) ? this.pool : null;
+      } );
+    }
+  }
+
 }
 
 densityBuoyancyCommon.register( 'BuoyancyApplicationsModel', BuoyancyApplicationsModel );
\ 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 2a2232fae9909676ede8b4116680d9b3e89bac4f)
+++ b/js/common/model/DensityBuoyancyModel.ts	(date 1719241175320)
@@ -18,10 +18,8 @@
 import Pool from './Pool.js';
 import Scale, { SCALE_WIDTH } from './Scale.js';
 import optionize from '../../../../phet-core/js/optionize.js';
-import Boat from '../../buoyancy/model/applications/Boat.js';
 import PhysicsEngine, { PhysicsEngineBody } from './PhysicsEngine.js';
 import Mass from './Mass.js';
-import Basin from './Basin.js';
 import Cuboid from './Cuboid.js';
 import TModel from '../../../../joist/js/TModel.js';
 import { PhetioObjectOptions } from '../../../../tandem/js/PhetioObject.js';
@@ -40,16 +38,6 @@
 const GROUND_FRONT_Z = POOL_DEPTH / 2;
 const POOL_BACK_Z = -POOL_DEPTH / 2;
 
-// Faster than normal stepping to fill the boat (kind of like animation speed)
-const FILL_EMPTY_MULTIPLIER = 0.3;
-
-// 90% of the boat is out of the water before spilling out the full boat
-const BOAT_READY_TO_SPILL_OUT_THRESHOLD = 0.9;
-
-// Y model distance of tolerance between the boat basin fluidY level and the boat basin stepTop. This was needed to
-// prevent filling thrashing as a containing mass floats around. See updateLiquidLevel();
-const BOAT_FULL_THRESHOLD = 0.01;
-
 export type DensityBuoyancyModelOptions = {
   usePoolScale?: boolean;
 } & PickRequired<PhetioObjectOptions, 'tandem'>;
@@ -74,8 +62,6 @@
   private barrierBody: PhysicsEngineBody;
   protected readonly availableMasses: ObservableArray<Mass>;
 
-  // Flag that sets an animation to empty the boat of any water inside of it
-  private spillingWaterOutOfBoat = false;
 
   public constructor( providedOptions?: DensityBuoyancyModelOptions ) {
     const options = optionize<DensityBuoyancyModelOptions, DensityBuoyancyModelOptions>()( {
@@ -203,9 +189,6 @@
       mass.interruptedEmitter.emit();
     } );
 
-    let boatVerticalVelocity = 0;
-    let boatVerticalAcceleration = 0;
-
     // The main engine post-step actions, that will determine the net forces applied on each mass. This callback fires
     // once per "physics engine step", and so results in potentially up to "p2MaxSubSteps" calls per simulation frame
     // (30 as of writing). This instance lives for the lifetime of the simulation, so we don't need to remove this
@@ -216,14 +199,7 @@
       // {number}
       const gravity = this.gravityProperty.value.value;
 
-      const boat = this.getBoat();
-
-      if ( boat && dt ) {
-        boat.setUnderwaterState( this.pool.fluidYInterpolatedProperty.currentValue );
-        const nextBoatVerticalVelocity = this.engine.bodyGetVelocity( boat.body ).y;
-        boatVerticalAcceleration = ( nextBoatVerticalVelocity - boatVerticalVelocity ) / dt;
-        boatVerticalVelocity = nextBoatVerticalVelocity;
-      }
+      this.postStepListenerPhase1Emitter.emit( dt );
 
       // Will set the force Properties for all the masses
       this.masses.forEach( mass => {
@@ -291,12 +267,8 @@
 
         let massValue = mass.massProperty.value;
 
-        if ( mass === boat && boat.isUnderwater ) {
-          // Special consideration for when boat is underwater
-          // Don't count the liquid inside the boat as part of the mass
-          submergedVolume = boat.volumeProperty.value;
-          massValue = submergedVolume * boat.materialProperty.value.density;
-        }
+        submergedVolume = this.overrideSubmergedVolume( submergedVolume );
+        massValue = this.overrideMassValue( submergedVolume, massValue );
 
         if ( submergedVolume !== 0 ) {
           const displacedMass = submergedVolume * this.pool.fluidDensityProperty.value;
@@ -352,123 +324,31 @@
     this.pool.scale && this.availableMasses.push( this.pool.scale );
   }
 
-  /**
-   * Returns the boat (if there is one). Overridden in subclasses that have a boat.
-   */
-  public getBoat(): Boat | null {
-    return null;
+  public overrideSubmergedVolume( mass: Mass, submergedVolume: number ): number {
+    return submergedVolume;
+  }
+
+  public overrideMassValue( submergedVolume: number, massValue: number ): number {
+    return massValue;
   }
 
   /**
    * Computes the heights of the main pool liquid (and optionally that of the boat)
    */
-  private updateFluid(): void {
-    const boat = this.getBoat();
+  protected updateFluid(): void {
 
-    const basins: Basin[] = [ this.pool ];
-    if ( boat && boat.visibleProperty.value ) {
-      basins.push( boat.basin );
-      this.pool.childBasin = boat.basin;
-    }
-    else {
-      this.pool.childBasin = null;
-    }
+    this.pool.childBasin = null;
 
     this.masses.forEach( mass => mass.updateStepInformation() );
-    basins.forEach( basin => {
-      basin.stepMasses = this.masses.filter( mass => basin.isMassInside( mass ) );
-    } );
-
-    let poolFluidVolume = this.pool.fluidVolumeProperty.value;
-
-    // May need to adjust volumes between the boat/pool if there is a boat
-    if ( boat ) {
-      poolFluidVolume = this.updateFluidsForBoat( poolFluidVolume );
-    }
+    this.pool.stepMasses = this.masses.filter( mass => this.pool.isMassInside( mass ) );
 
     // Check to see if water "spilled" out of the pool, and set the finalized liquid volume
-    this.pool.fluidVolumeProperty.value = Math.min( poolFluidVolume, this.pool.getEmptyVolume( this.poolBounds.maxY ) );
-
+    this.pool.fluidVolumeProperty.value = Math.min( this.pool.fluidVolumeProperty.value, this.pool.getEmptyVolume( this.poolBounds.maxY ) );
     this.pool.computeY();
-    boat && boat.basin.computeY();
 
-    // If we have a boat that is NOT underwater, we'll assign masses into the boat's basin where relevant. Otherwise,
-    // anything will go just into the pool's basin.
-    if ( boat && boat.visibleProperty.value && !boat.isUnderwater ) {
-      this.masses.forEach( mass => {
-        mass.containingBasin = boat.basin.isMassInside( mass ) ? boat.basin : ( this.pool.isMassInside( mass ) ? this.pool : null );
-      } );
-    }
-    else {
-      this.masses.forEach( mass => {
-        mass.containingBasin = this.pool.isMassInside( mass ) ? this.pool : null;
-      } );
-    }
-  }
-
-  private updateFluidsForBoat( poolFluidVolume: number ): number {
-    const boat = this.getBoat()!;
-
-    assert && assert( boat, 'boat needed to update liquids for boat' );
-
-    const boatBasin = boat.basin;
-    if ( boat.visibleProperty.value ) {
-      let boatFluidVolume = boatBasin.fluidVolumeProperty.value;
-      const boatBasinMaximumVolume = boatBasin.getMaximumVolume( boatBasin.stepTop );
-
-      const poolEmptyVolumeToBoatTop = this.pool.getEmptyVolume( Math.min( boat.stepTop, this.poolBounds.maxY ) );
-      const boatEmptyVolumeToBoatTop = boatBasin.getEmptyVolume( boat.stepTop );
-
-      // Calculate adjustments to water volumes to match the current space in the basin
-      let poolExcess = poolFluidVolume - poolEmptyVolumeToBoatTop;
-      let boatExcess = boatFluidVolume - boatEmptyVolumeToBoatTop;
-
-      const boatHeight = boat.shapeProperty.value.getBounds().height;
-
-      if ( boatFluidVolume ) {
-
-        // If the top of the boat is out of the water past the height threshold, spill the water back into the pool
-        // (even if not totally full).
-        if ( boat.stepTop > this.pool.fluidYInterpolatedProperty.currentValue + boatHeight * BOAT_READY_TO_SPILL_OUT_THRESHOLD ) {
-          this.spillingWaterOutOfBoat = true;
-        }
-      }
-      else {
-        // If the boat is empty, stop spilling
-        this.spillingWaterOutOfBoat = false;
-      }
-
-      // If the boat is out of the water, spill the water back into the pool
-      if ( this.spillingWaterOutOfBoat ) {
-        boatExcess = Math.min( FILL_EMPTY_MULTIPLIER * boat.volumeProperty.value, boatFluidVolume );
-      }
-      else if ( boatFluidVolume > 0 &&
-                Math.abs( boatBasin.fluidYInterpolatedProperty.currentValue - boatBasin.stepTop ) >= BOAT_FULL_THRESHOLD ) {
-        // If the boat is neither full nor empty, nor spilling, then it is currently filling up. We will up no matter
-        // the current water leve or the boat AND no matter the boats position. This is because the boat can only
-        // ever be full or empty (or animating to one of those states).
-
-        const excess = Math.min( FILL_EMPTY_MULTIPLIER * boat.volumeProperty.value, boatBasinMaximumVolume - boatFluidVolume ); // This animates the boat spilling in
-        poolExcess = excess;
-        boatExcess = -excess;
-      }
-
-      if ( poolExcess > 0 && boatExcess < 0 ) {
-        const transferVolume = Math.min( poolExcess, -boatExcess );
-        poolFluidVolume -= transferVolume;
-        boatFluidVolume += transferVolume;
-      }
-      else if ( boatExcess > 0 ) {
-        // If the boat overflows, just dump the rest in the pool
-        poolFluidVolume += boatExcess;
-        boatFluidVolume -= boatExcess;
-      }
-      boatBasin.fluidVolumeProperty.value = boatFluidVolume;
-    }
-    else {
-      boatBasin.fluidVolumeProperty.value = 0;
-    }
-    return poolFluidVolume;
+    this.masses.forEach( mass => {
+      mass.containingBasin = this.pool.isMassInside( mass ) ? this.pool : null;
+    } );
   }
 
   /**
@@ -477,7 +357,6 @@
   public reset(): void {
 
     this.gravityProperty.reset();
-    this.spillingWaterOutOfBoat = false;
 
     this.pool.reset();
     this.masses.forEach( mass => mass.reset() );
Index: js/buoyancy/model/BuoyancyModel.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/buoyancy/model/BuoyancyModel.ts b/js/buoyancy/model/BuoyancyModel.ts
new file mode 100644
--- /dev/null	(date 1719239673376)
+++ b/js/buoyancy/model/BuoyancyModel.ts	(date 1719239673376)
@@ -0,0 +1,8 @@
+// Copyright 2024, University of Colorado Boulder
+
+/**
+ * BuoyancyModel adds information specific to the Buoyancy screens.
+ */
+export default abstract class BuoyancyModel {
+
+}
\ No newline at end of file

@samreid
Copy link
Member Author

samreid commented Jun 24, 2024

Updated patch, maybe? Make sure we don't drop the other boat fluid fix

Subject: [PATCH] Do not show the pool scale when resetting the boat scene, see https://github.com/phetsims/buoyancy/issues/179
---
Index: js/buoyancy/model/applications/BuoyancyApplicationsModel.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/buoyancy/model/applications/BuoyancyApplicationsModel.ts b/js/buoyancy/model/applications/BuoyancyApplicationsModel.ts
--- a/js/buoyancy/model/applications/BuoyancyApplicationsModel.ts	(revision fcac2779775372bb7a48f68dbfc79333c62b4e2e)
+++ b/js/buoyancy/model/applications/BuoyancyApplicationsModel.ts	(date 1719246494870)
@@ -19,6 +19,18 @@
 import { BottleOrBoat, BottleOrBoatValues } from './BottleOrBoat.js';
 import StringUnionProperty from '../../../../../axon/js/StringUnionProperty.js';
 import MassTag from '../../../common/model/MassTag.js';
+import Basin from '../../../common/model/Basin.js';
+import Mass from '../../../common/model/Mass.js';
+
+// Faster than normal stepping to fill the boat (kind of like animation speed)
+const FILL_EMPTY_MULTIPLIER = 0.3;
+
+// 90% of the boat is out of the water before spilling out the full boat
+const BOAT_READY_TO_SPILL_OUT_THRESHOLD = 0.9;
+
+// Y model distance of tolerance between the boat basin fluidY level and the boat basin stepTop. This was needed to
+// prevent filling thrashing as a containing mass floats around. See updateLiquidLevel();
+const BOAT_FULL_THRESHOLD = 0.01;
 
 export type BuoyancyApplicationsModelOptions = DensityBuoyancyModelOptions;
 
@@ -31,6 +43,9 @@
   public readonly boat: Boat;
   private readonly scale: Scale; // Scale sitting on the ground next to the pool
 
+  // Flag that sets an animation to empty the boat of any water inside of it
+  private spillingWaterOutOfBoat = false;
+
   public constructor( options: BuoyancyApplicationsModelOptions ) {
 
     const tandem = options.tandem;
@@ -103,6 +118,20 @@
       assert && assert( !this.boat.visibleProperty.value || !this.bottle.visibleProperty.value,
         'Boat and bottle should not be visible at the same time' );
     } );
+
+    let boatVerticalVelocity = 0;
+    let boatVerticalAcceleration = 0;
+
+    this.postStepPhase1Emitter.addListener( dt => {
+      const boat = this.boat;
+
+      if ( dt ) {
+        boat.setUnderwaterState( this.pool.fluidYInterpolatedProperty.currentValue );
+        const nextBoatVerticalVelocity = this.engine.bodyGetVelocity( boat.body ).y;
+        boatVerticalAcceleration = ( nextBoatVerticalVelocity - boatVerticalVelocity ) / dt;
+        boatVerticalVelocity = nextBoatVerticalVelocity;
+      }
+    } );
   }
 
   public override step( dt: number ): void {
@@ -112,10 +141,6 @@
     super.step( dt );
   }
 
-  public override getBoat(): Boat | null {
-    return this.boat;
-  }
-
   /**
    * Moves the boat and block to their initial locations (see https://github.com/phetsims/buoyancy/issues/25)
    */
@@ -143,11 +168,152 @@
 
     super.reset();
 
+    this.spillingWaterOutOfBoat = false;
+
     this.sceneProperty.reset();
 
     assert && assert( !this.boat.visibleProperty.value || !this.bottle.visibleProperty.value,
       'Boat and bottle should not be visible at the same time' );
   }
+
+  private updateFluidsForBoat( poolFluidVolume: number ): number {
+    const boat = this.boat;
+
+    assert && assert( boat, 'boat needed to update liquids for boat' );
+
+    const boatBasin = boat.basin;
+    if ( boat.visibleProperty.value ) {
+      let boatFluidVolume = boatBasin.fluidVolumeProperty.value;
+      const boatBasinMaximumVolume = boatBasin.getMaximumVolume( boatBasin.stepTop );
+
+      const poolEmptyVolumeToBoatTop = this.pool.getEmptyVolume( Math.min( boat.stepTop, this.poolBounds.maxY ) );
+      const boatEmptyVolumeToBoatTop = boatBasin.getEmptyVolume( boat.stepTop );
+
+      // Calculate adjustments to water volumes to match the current space in the basin
+      let poolExcess = poolFluidVolume - poolEmptyVolumeToBoatTop;
+      let boatExcess = boatFluidVolume - boatEmptyVolumeToBoatTop;
+
+      const boatHeight = boat.shapeProperty.value.getBounds().height;
+
+      if ( boatFluidVolume ) {
+
+        // If the top of the boat is out of the water past the height threshold, spill the water back into the pool
+        // (even if not totally full).
+        if ( boat.stepTop > this.pool.fluidYInterpolatedProperty.currentValue + boatHeight * BOAT_READY_TO_SPILL_OUT_THRESHOLD ) {
+          this.spillingWaterOutOfBoat = true;
+        }
+      }
+      else {
+        // If the boat is empty, stop spilling
+        this.spillingWaterOutOfBoat = false;
+      }
+
+      // If the boat is out of the water, spill the water back into the pool
+      if ( this.spillingWaterOutOfBoat ) {
+        boatExcess = Math.min( FILL_EMPTY_MULTIPLIER * boat.volumeProperty.value, boatFluidVolume );
+      }
+      else if ( boatFluidVolume > 0 &&
+                Math.abs( boatBasin.fluidYInterpolatedProperty.currentValue - boatBasin.stepTop ) >= BOAT_FULL_THRESHOLD ) {
+        // If the boat is neither full nor empty, nor spilling, then it is currently filling up. We will up no matter
+        // the current water leve or the boat AND no matter the boats position. This is because the boat can only
+        // ever be full or empty (or animating to one of those states).
+
+        const excess = Math.min( FILL_EMPTY_MULTIPLIER * boat.volumeProperty.value, boatBasinMaximumVolume - boatFluidVolume ); // This animates the boat spilling in
+        poolExcess = excess;
+        boatExcess = -excess;
+      }
+
+      if ( poolExcess > 0 && boatExcess < 0 ) {
+        const transferVolume = Math.min( poolExcess, -boatExcess );
+        poolFluidVolume -= transferVolume;
+        boatFluidVolume += transferVolume;
+      }
+      else if ( boatExcess > 0 ) {
+        // If the boat overflows, just dump the rest in the pool
+        poolFluidVolume += boatExcess;
+        boatFluidVolume -= boatExcess;
+      }
+      boatBasin.fluidVolumeProperty.value = boatFluidVolume;
+    }
+    else {
+
+      // When the boat is hidden (whether via changing scene or by phet-io), move the fluid from the boat basin to the pool.
+      poolFluidVolume += boatBasin.fluidVolumeProperty.value;
+      boatBasin.fluidVolumeProperty.value = 0;
+    }
+    return poolFluidVolume;
+  }
+
+  public override overrideSubmergedVolume( mass: Mass, submergedVolume: number ): number {
+    const boat = this.boat;
+    if ( mass === boat && boat.isUnderwater ) {
+
+      // Special consideration for when boat is underwater
+      // Don't count the liquid inside the boat as part of the mass
+      return boat.volumeProperty.value;
+
+    }
+    else {
+      return submergedVolume;
+    }
+  }
+
+  public override overrideMassValue( submergedVolume: number, massValue: number ): number {
+    return submergedVolume * this.boat.materialProperty.value.density;
+  }
+
+
+  /**
+   * Computes the heights of the main pool liquid (and optionally that of the boat)
+   * NOTE: This does not call the super class method, as it needs to manage the sequencing of the actions, hence
+   * there is duplicated code in this implementation.
+   *
+   * TODO: Call super.updateFluid and remove the redundancies from this override, see https://github.com/phetsims/density-buoyancy-common/issues/234
+   */
+  protected override updateFluid(): void {
+    const boat = this.boat;
+
+    const basins: Basin[] = [ this.pool ];
+    if ( boat && boat.visibleProperty.value ) {
+      basins.push( boat.basin );
+      this.pool.childBasin = boat.basin;
+    }
+    else {
+      this.pool.childBasin = null;
+    }
+
+    this.masses.forEach( mass => mass.updateStepInformation() );
+    basins.forEach( basin => {
+      basin.stepMasses = this.masses.filter( mass => basin.isMassInside( mass ) );
+    } );
+
+    let poolFluidVolume = this.pool.fluidVolumeProperty.value;
+
+    // May need to adjust volumes between the boat/pool if there is a boat
+    if ( boat ) {
+      poolFluidVolume = this.updateFluidsForBoat( poolFluidVolume );
+    }
+
+    // Check to see if water "spilled" out of the pool, and set the finalized liquid volume
+    this.pool.fluidVolumeProperty.value = Math.min( poolFluidVolume, this.pool.getEmptyVolume( this.poolBounds.maxY ) );
+
+    this.pool.computeY();
+    boat && boat.basin.computeY();
+
+    // If we have a boat that is NOT underwater, we'll assign masses into the boat's basin where relevant. Otherwise,
+    // anything will go just into the pool's basin.
+    if ( boat && boat.visibleProperty.value && !boat.isUnderwater ) {
+      this.masses.forEach( mass => {
+        mass.containingBasin = boat.basin.isMassInside( mass ) ? boat.basin : ( this.pool.isMassInside( mass ) ? this.pool : null );
+      } );
+    }
+    else {
+      this.masses.forEach( mass => {
+        mass.containingBasin = this.pool.isMassInside( mass ) ? this.pool : null;
+      } );
+    }
+  }
+
 }
 
 densityBuoyancyCommon.register( 'BuoyancyApplicationsModel', BuoyancyApplicationsModel );
\ 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 fcac2779775372bb7a48f68dbfc79333c62b4e2e)
+++ b/js/common/model/DensityBuoyancyModel.ts	(date 1719245708909)
@@ -18,10 +18,8 @@
 import Pool from './Pool.js';
 import Scale, { SCALE_WIDTH } from './Scale.js';
 import optionize from '../../../../phet-core/js/optionize.js';
-import Boat from '../../buoyancy/model/applications/Boat.js';
 import PhysicsEngine, { PhysicsEngineBody } from './PhysicsEngine.js';
 import Mass from './Mass.js';
-import Basin from './Basin.js';
 import Cuboid from './Cuboid.js';
 import TModel from '../../../../joist/js/TModel.js';
 import { PhetioObjectOptions } from '../../../../tandem/js/PhetioObject.js';
@@ -40,16 +38,6 @@
 const GROUND_FRONT_Z = POOL_DEPTH / 2;
 const POOL_BACK_Z = -POOL_DEPTH / 2;
 
-// Faster than normal stepping to fill the boat (kind of like animation speed)
-const FILL_EMPTY_MULTIPLIER = 0.3;
-
-// 90% of the boat is out of the water before spilling out the full boat
-const BOAT_READY_TO_SPILL_OUT_THRESHOLD = 0.9;
-
-// Y model distance of tolerance between the boat basin fluidY level and the boat basin stepTop. This was needed to
-// prevent filling thrashing as a containing mass floats around. See updateLiquidLevel();
-const BOAT_FULL_THRESHOLD = 0.01;
-
 export type DensityBuoyancyModelOptions = {
   usePoolScale?: boolean;
 } & PickRequired<PhetioObjectOptions, 'tandem'>;
@@ -74,8 +62,6 @@
   private barrierBody: PhysicsEngineBody;
   protected readonly availableMasses: ObservableArray<Mass>;
 
-  // Flag that sets an animation to empty the boat of any water inside of it
-  private spillingWaterOutOfBoat = false;
 
   public constructor( providedOptions?: DensityBuoyancyModelOptions ) {
     const options = optionize<DensityBuoyancyModelOptions, DensityBuoyancyModelOptions>()( {
@@ -203,9 +189,6 @@
       mass.interruptedEmitter.emit();
     } );
 
-    let boatVerticalVelocity = 0;
-    let boatVerticalAcceleration = 0;
-
     // The main engine post-step actions, that will determine the net forces applied on each mass. This callback fires
     // once per "physics engine step", and so results in potentially up to "p2MaxSubSteps" calls per simulation frame
     // (30 as of writing). This instance lives for the lifetime of the simulation, so we don't need to remove this
@@ -216,14 +199,7 @@
       // {number}
       const gravity = this.gravityProperty.value.value;
 
-      const boat = this.getBoat();
-
-      if ( boat && dt ) {
-        boat.setUnderwaterState( this.pool.fluidYInterpolatedProperty.currentValue );
-        const nextBoatVerticalVelocity = this.engine.bodyGetVelocity( boat.body ).y;
-        boatVerticalAcceleration = ( nextBoatVerticalVelocity - boatVerticalVelocity ) / dt;
-        boatVerticalVelocity = nextBoatVerticalVelocity;
-      }
+      this.postStepListenerPhase1Emitter.emit( dt );
 
       // Will set the force Properties for all the masses
       this.masses.forEach( mass => {
@@ -291,12 +267,8 @@
 
         let massValue = mass.massProperty.value;
 
-        if ( mass === boat && boat.isUnderwater ) {
-          // Special consideration for when boat is underwater
-          // Don't count the liquid inside the boat as part of the mass
-          submergedVolume = boat.volumeProperty.value;
-          massValue = submergedVolume * boat.materialProperty.value.density;
-        }
+        submergedVolume = this.overrideSubmergedVolume( submergedVolume );
+        massValue = this.overrideMassValue( submergedVolume, massValue );
 
         if ( submergedVolume !== 0 ) {
           const displacedMass = submergedVolume * this.pool.fluidDensityProperty.value;
@@ -352,126 +324,31 @@
     this.pool.scale && this.availableMasses.push( this.pool.scale );
   }
 
-  /**
-   * Returns the boat (if there is one). Overridden in subclasses that have a boat.
-   */
-  public getBoat(): Boat | null {
-    return null;
+  public overrideMassValue( submergedVolume: number, massValue: number ): number {
+    return massValue;
+  }
+
+  public overrideSubmergedVolume( submergedVolume: number ): number {
+    return submergedVolume;
   }
 
   /**
    * Computes the heights of the main pool liquid (and optionally that of the boat)
    */
-  private updateFluid(): void {
-    const boat = this.getBoat();
+  protected updateFluid(): void {
 
-    const basins: Basin[] = [ this.pool ];
-    if ( boat && boat.visibleProperty.value ) {
-      basins.push( boat.basin );
-      this.pool.childBasin = boat.basin;
-    }
-    else {
-      this.pool.childBasin = null;
-    }
+    this.pool.childBasin = null;
 
     this.masses.forEach( mass => mass.updateStepInformation() );
-    basins.forEach( basin => {
-      basin.stepMasses = this.masses.filter( mass => basin.isMassInside( mass ) );
-    } );
-
-    let poolFluidVolume = this.pool.fluidVolumeProperty.value;
-
-    // May need to adjust volumes between the boat/pool if there is a boat
-    if ( boat ) {
-      poolFluidVolume = this.updateFluidsForBoat( poolFluidVolume );
-    }
+    this.pool.stepMasses = this.masses.filter( mass => this.pool.isMassInside( mass ) );
 
     // Check to see if water "spilled" out of the pool, and set the finalized liquid volume
-    this.pool.fluidVolumeProperty.value = Math.min( poolFluidVolume, this.pool.getEmptyVolume( this.poolBounds.maxY ) );
-
+    this.pool.fluidVolumeProperty.value = Math.min( this.pool.fluidVolumeProperty.value, this.pool.getEmptyVolume( this.poolBounds.maxY ) );
     this.pool.computeY();
-    boat && boat.basin.computeY();
 
-    // If we have a boat that is NOT underwater, we'll assign masses into the boat's basin where relevant. Otherwise,
-    // anything will go just into the pool's basin.
-    if ( boat && boat.visibleProperty.value && !boat.isUnderwater ) {
-      this.masses.forEach( mass => {
-        mass.containingBasin = boat.basin.isMassInside( mass ) ? boat.basin : ( this.pool.isMassInside( mass ) ? this.pool : null );
-      } );
-    }
-    else {
-      this.masses.forEach( mass => {
-        mass.containingBasin = this.pool.isMassInside( mass ) ? this.pool : null;
-      } );
-    }
-  }
-
-  private updateFluidsForBoat( poolFluidVolume: number ): number {
-    const boat = this.getBoat()!;
-
-    assert && assert( boat, 'boat needed to update liquids for boat' );
-
-    const boatBasin = boat.basin;
-    if ( boat.visibleProperty.value ) {
-      let boatFluidVolume = boatBasin.fluidVolumeProperty.value;
-      const boatBasinMaximumVolume = boatBasin.getMaximumVolume( boatBasin.stepTop );
-
-      const poolEmptyVolumeToBoatTop = this.pool.getEmptyVolume( Math.min( boat.stepTop, this.poolBounds.maxY ) );
-      const boatEmptyVolumeToBoatTop = boatBasin.getEmptyVolume( boat.stepTop );
-
-      // Calculate adjustments to water volumes to match the current space in the basin
-      let poolExcess = poolFluidVolume - poolEmptyVolumeToBoatTop;
-      let boatExcess = boatFluidVolume - boatEmptyVolumeToBoatTop;
-
-      const boatHeight = boat.shapeProperty.value.getBounds().height;
-
-      if ( boatFluidVolume ) {
-
-        // If the top of the boat is out of the water past the height threshold, spill the water back into the pool
-        // (even if not totally full).
-        if ( boat.stepTop > this.pool.fluidYInterpolatedProperty.currentValue + boatHeight * BOAT_READY_TO_SPILL_OUT_THRESHOLD ) {
-          this.spillingWaterOutOfBoat = true;
-        }
-      }
-      else {
-        // If the boat is empty, stop spilling
-        this.spillingWaterOutOfBoat = false;
-      }
-
-      // If the boat is out of the water, spill the water back into the pool
-      if ( this.spillingWaterOutOfBoat ) {
-        boatExcess = Math.min( FILL_EMPTY_MULTIPLIER * boat.volumeProperty.value, boatFluidVolume );
-      }
-      else if ( boatFluidVolume > 0 &&
-                Math.abs( boatBasin.fluidYInterpolatedProperty.currentValue - boatBasin.stepTop ) >= BOAT_FULL_THRESHOLD ) {
-        // If the boat is neither full nor empty, nor spilling, then it is currently filling up. We will up no matter
-        // the current water leve or the boat AND no matter the boats position. This is because the boat can only
-        // ever be full or empty (or animating to one of those states).
-
-        const excess = Math.min( FILL_EMPTY_MULTIPLIER * boat.volumeProperty.value, boatBasinMaximumVolume - boatFluidVolume ); // This animates the boat spilling in
-        poolExcess = excess;
-        boatExcess = -excess;
-      }
-
-      if ( poolExcess > 0 && boatExcess < 0 ) {
-        const transferVolume = Math.min( poolExcess, -boatExcess );
-        poolFluidVolume -= transferVolume;
-        boatFluidVolume += transferVolume;
-      }
-      else if ( boatExcess > 0 ) {
-        // If the boat overflows, just dump the rest in the pool
-        poolFluidVolume += boatExcess;
-        boatFluidVolume -= boatExcess;
-      }
-      boatBasin.fluidVolumeProperty.value = boatFluidVolume;
-    }
-    else {
-
-      // When the boat is hidden (whether via changing scene or by phet-io), move the fluid from the boat basin to the pool.
-      poolFluidVolume += boatBasin.fluidVolumeProperty.value;
-      boatBasin.fluidVolumeProperty.value = 0;
-    }
-    return poolFluidVolume;
+    this.masses.forEach( mass => {
+      mass.containingBasin = this.pool.isMassInside( mass ) ? this.pool : null;
+    } );
   }
 
   /**
@@ -480,7 +357,6 @@
   public reset(): void {
 
     this.gravityProperty.reset();
-    this.spillingWaterOutOfBoat = false;
 
     this.pool.reset();
     this.masses.forEach( mass => mass.reset() );

@zepumph
Copy link
Member

zepumph commented Jul 8, 2024

@AgustinVallejo will review when it is time.

@samreid
Copy link
Member Author

samreid commented Jul 8, 2024

Working copy:

Subject: [PATCH] Factor out updateFluidForBasins, see https://github.com/phetsims/density-buoyancy-common/issues/234
---
Index: js/buoyancy/model/applications/BuoyancyApplicationsModel.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/buoyancy/model/applications/BuoyancyApplicationsModel.ts b/js/buoyancy/model/applications/BuoyancyApplicationsModel.ts
--- a/js/buoyancy/model/applications/BuoyancyApplicationsModel.ts	(revision cdc6951f7fc5f15ec469a1cc91b4bb27f3cbad90)
+++ b/js/buoyancy/model/applications/BuoyancyApplicationsModel.ts	(date 1720458033005)
@@ -20,6 +20,7 @@
 import StringUnionProperty from '../../../../../axon/js/StringUnionProperty.js';
 import MassTag from '../../../common/model/MassTag.js';
 import Basin from '../../../common/model/Basin.js';
+import Mass from '../../../common/model/Mass.js';
 
 export type BuoyancyApplicationsModelOptions = DensityBuoyancyModelOptions;
 
@@ -42,6 +43,9 @@
   public readonly boat: Boat;
   private readonly scale: Scale; // Scale sitting on the ground next to the pool
 
+  // Flag that sets an animation to empty the boat of any fluid inside of it
+  protected spillingWaterOutOfBoat = false;
+
   public constructor( options: BuoyancyApplicationsModelOptions ) {
 
     const tandem = options.tandem;
@@ -123,10 +127,6 @@
     super.step( dt );
   }
 
-  public override getBoat(): Boat | null {
-    return this.boat;
-  }
-
   /**
    * Moves the boat and block to their initial locations (see https://github.com/phetsims/buoyancy/issues/25)
    */
@@ -139,6 +139,12 @@
     // Move things to the initial position
     this.boat.resetPosition();
     this.block.resetPosition();
+
+    // REVIEW: Can we call boat.reset() here?
+    this.boat.verticalAcceleration = 0;
+    this.boat.verticalVelocity = 0;
+
+    this.spillingWaterOutOfBoat = false;
   }
 
   /**
@@ -153,6 +159,7 @@
     this.boat.reset();
 
     super.reset();
+    this.spillingWaterOutOfBoat = false;
 
     this.sceneProperty.reset();
 
@@ -165,7 +172,7 @@
 
     let poolFluidVolume = this.pool.fluidVolumeProperty.value;
 
-    const boat = this.getBoat()!;
+    const boat = this.boat;
 
     assert && assert( boat, 'boat needed to update liquids for boat' );
 
@@ -256,6 +263,53 @@
 
     this.updateFluidForBasins( basins, assignableBasins );
   }
+
+  protected override getUpdatedSubmergedVolume( mass: Mass, submergedVolume: number ): number {
+
+    if ( mass === this.boat && this.boat.isFullySubmerged ) {
+
+      // Special consideration for when boat is submerged
+      // Don't count the liquid inside the boat as part of the mass
+      return this.boat.volumeProperty.value;
+    }
+    else {
+      return super.getUpdatedSubmergedVolume( mass, submergedVolume );
+    }
+  }
+
+  protected override getUpdatedMassValue( mass: Mass, massValue: number, submergedVolume: number ): number {
+    if ( mass === this.boat && this.boat.isFullySubmerged ) {
+
+      // Special consideration for when boat is submerged
+      // Don't count the liquid inside the boat as part of the mass
+      return submergedVolume * this.boat.materialProperty.value.density;
+    }
+    else {
+      return super.getUpdatedMassValue( mass, massValue, submergedVolume );
+    }
+  }
+
+  protected override getAdditionalVerticalAcceleration( basin: Basin | null ): number {
+    return basin === this.boat.basin ? this.boat.verticalAcceleration : 0;
+  }
+
+  protected override adjustVelocity( basin: Basin | null, velocity: Vector2 ): void {
+
+    // If the boat is moving, assume the liquid moves with it, and apply viscosity due to the movement of our mass
+    // inside the boat's liquid.
+    if ( basin === this.boat.basin ) {
+      velocity.subtract( this.engine.bodyGetVelocity( this.boat.body ) );
+    }
+  }
+
+  protected override updateVerticalMotion( dt: number ): void {
+    super.updateVerticalMotion( dt );
+
+    if ( dt ) {
+      this.boat.updateVerticalMotion( this.pool, dt );
+    }
+  }
+
 }
 
 densityBuoyancyCommon.register( 'BuoyancyApplicationsModel', BuoyancyApplicationsModel );
\ 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 cdc6951f7fc5f15ec469a1cc91b4bb27f3cbad90)
+++ b/js/common/model/DensityBuoyancyModel.ts	(date 1720457948361)
@@ -63,9 +63,6 @@
   private barrierBody: PhysicsEngineBody;
   protected readonly availableMasses: ObservableArray<Mass>;
 
-  // Flag that sets an animation to empty the boat of any fluid inside of it
-  protected spillingWaterOutOfBoat = false;
-
   public constructor( providedOptions?: DensityBuoyancyModelOptions ) {
     const options = optionize<DensityBuoyancyModelOptions, DensityBuoyancyModelOptions>()( {
       usePoolScale: true
@@ -192,9 +189,6 @@
       mass.interruptedEmitter.emit();
     } );
 
-    let boatVerticalVelocity = 0;
-    let boatVerticalAcceleration = 0;
-
     // The main engine post-step actions, that will determine the net forces applied on each mass. This callback fires
     // once per "physics engine step", and so results in potentially up to "p2MaxSubSteps" calls per simulation frame
     // (30 as of writing). This instance lives for the lifetime of the simulation, so we don't need to remove this
@@ -202,17 +196,8 @@
     this.engine.addPostStepListener( dt => {
       this.updateFluid();
 
-      // {number}
       const gravity = this.gravityProperty.value.value;
-
-      const boat = this.getBoat();
-
-      if ( boat && dt ) {
-        boat.setSubmergedState( this.pool.fluidYInterpolatedProperty.currentValue );
-        const nextBoatVerticalVelocity = this.engine.bodyGetVelocity( boat.body ).y;
-        boatVerticalAcceleration = ( nextBoatVerticalVelocity - boatVerticalVelocity ) / dt;
-        boatVerticalVelocity = nextBoatVerticalVelocity;
-      }
+      this.updateVerticalMotion( dt );
 
       // Will set the force Properties for all the masses
       this.masses.forEach( mass => {
@@ -280,26 +265,20 @@
 
         let massValue = mass.massProperty.value;
 
-        if ( mass === boat && boat.isFullySubmerged ) {
-          // Special consideration for when boat is submerged
-          // Don't count the liquid inside the boat as part of the mass
-          submergedVolume = boat.volumeProperty.value;
-          massValue = submergedVolume * boat.materialProperty.value.density;
-        }
+        submergedVolume = this.getUpdatedSubmergedVolume( mass, submergedVolume );
+        massValue = this.getUpdatedMassValue( mass, massValue, submergedVolume );
 
         if ( submergedVolume !== 0 ) {
           const displacedMass = submergedVolume * this.pool.fluidDensityProperty.value;
+
           // Vertical acceleration of the boat will change the buoyant force.
-          const acceleration = gravity + ( ( boat && basin === boat.basin ) ? boatVerticalAcceleration : 0 );
+          const acceleration = gravity + this.getAdditionalVerticalAcceleration( basin );
+
           const buoyantForce = new Vector2( 0, displacedMass * acceleration );
           this.engine.bodyApplyForce( mass.body, buoyantForce );
           mass.buoyancyForceInterpolatedProperty.setNextValue( buoyantForce );
 
-          // If the boat is moving, assume the liquid moves with it, and apply viscosity due to the movement of our mass
-          // inside the boat's liquid.
-          if ( boat && basin === boat.basin ) {
-            velocity.subtract( this.engine.bodyGetVelocity( boat.body ) );
-          }
+          this.adjustVelocity( basin, velocity );
 
           // Increase the generally-visible viscosity effect
           const ratioSubmerged =
@@ -341,10 +320,14 @@
     this.pool.scale && this.availableMasses.push( this.pool.scale );
   }
 
+  protected updateVerticalMotion( dt: number ): void {
+    // no-op
+  }
+
   /**
    * Returns the boat (if there is one). Overridden in subclasses that have a boat.
    */
-  public getBoat(): Boat | null {
+  private getBoat(): Boat | null {
     return null;
   }
 
@@ -390,7 +373,6 @@
   public reset(): void {
 
     this.gravityProperty.reset();
-    this.spillingWaterOutOfBoat = false;
 
     this.pool.reset();
     this.masses.forEach( mass => mass.reset() );
@@ -484,6 +466,22 @@
       mass.transformedEmitter.emit();
     } );
   }
+
+  protected getUpdatedSubmergedVolume( mass: Mass, submergedVolume: number ): number {
+    return submergedVolume;
+  }
+
+  protected getUpdatedMassValue( mass: Mass, massValue: number, submergedVolume: number ): number {
+    return massValue;
+  }
+
+  protected getAdditionalVerticalAcceleration( basin: Basin | null ): number {
+    return 0;
+  }
+
+  protected adjustVelocity( basin: Basin | null, velocity: Vector2 ): void {
+    // no-op
+  }
 }
 
 densityBuoyancyCommon.register( 'DensityBuoyancyModel', DensityBuoyancyModel );
\ No newline at end of file
Index: js/buoyancy/model/applications/Boat.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/buoyancy/model/applications/Boat.ts b/js/buoyancy/model/applications/Boat.ts
--- a/js/buoyancy/model/applications/Boat.ts	(revision cdc6951f7fc5f15ec469a1cc91b4bb27f3cbad90)
+++ b/js/buoyancy/model/applications/Boat.ts	(date 1720456650026)
@@ -26,6 +26,7 @@
 import Vector2 from '../../../../../dot/js/Vector2.js';
 import DensityBuoyancyCommonConstants, { toLiters } from '../../../common/DensityBuoyancyCommonConstants.js';
 import NumberProperty from '../../../../../axon/js/NumberProperty.js';
+import Pool from '../../../common/model/Pool.js';
 
 export type BoatOptions = StrictOmit<ApplicationsMassOptions, 'body' | 'shape' | 'volume' | 'material' | 'massShape'>;
 
@@ -49,6 +50,9 @@
   // Whether the boat is fully submerged
   public isFullySubmerged = false;
 
+  public verticalVelocity = 0;
+  public verticalAcceleration = 0;
+
   public constructor( engine: PhysicsEngine, blockWidthProperty: TReadOnlyProperty<number>, fluidMaterialProperty: TProperty<Material>, providedOptions: BoatOptions ) {
 
     const boatIntersectionVertices = BoatDesign.getIntersectionVertices( blockWidthProperty.value / 2, toLiters( ApplicationsMass.DEFAULT_DISPLACEMENT_VOLUME ) );
@@ -230,9 +234,18 @@
     this.displacementVolumeProperty.reset();
 
     this.basin.reset();
+    this.verticalVelocity = 0;
+    this.verticalAcceleration = 0;
 
     super.reset();
   }
+
+  public updateVerticalMotion( pool: Pool, dt: number ): void {
+    this.setSubmergedState( pool.fluidYInterpolatedProperty.currentValue );
+    const nextBoatVerticalVelocity = this.engine.bodyGetVelocity( this.body ).y;
+    this.verticalAcceleration = ( nextBoatVerticalVelocity - this.verticalVelocity ) / dt;
+    this.verticalVelocity = nextBoatVerticalVelocity;
+  }
 }
 
 densityBuoyancyCommon.register( 'Boat', Boat );
\ No newline at end of file

@samreid
Copy link
Member Author

samreid commented Jul 8, 2024

I moved Boat out of DensityBuoyancyModel to BuoyancyApplicationsModel and some logic from DensityBuoyancyScreenView to BuoyancyApplicationsScreenView. I tried to preserve the logic, and sequencing of actions. @AgustinVallejo volunteered to review, please check on in particular:

  1. The overall strategy. Note there are now several no-op methods in DensityBuoyancyModel which are overridden, like:
    // Placeholders for the physics update sequence, which must be overridden in BuoyancyApplicationsModel.
    protected getUpdatedSubmergedVolume( mass: Mass, submergedVolume: number ): number {
    return submergedVolume;
    }
    protected getUpdatedMassValue( mass: Mass, massValue: number, submergedVolume: number ): number {
    return massValue;
    }
    protected getAdditionalVerticalAcceleration( basin: Basin | null ): number {
    return 0;
    }
    protected adjustVelocity( basin: Basin | null, velocity: Vector2 ): void {
    // no-op
    }
    . Please comment on whether this is acceptable or whether we should take more steps to streamline it further.
  2. The commits that were more complex and warrant a closer look are: cdc6951 and 02b26ae, but feel free to examine all commits as in-scope for the review.

@AgustinVallejo
Copy link
Contributor

I pushed a reordering of some functions so there's better consistency and readability between DensityBuoyancyModel and BuoyancyApplicationsModel. Also added a little documentation note.

Everything else seems functional and well designed. Some questions and ideas below:

  1. The function getPoolFluidVolume has a if ( boat.visibleProperty.value ) to encompass the boat's logic... Should other functions such as updateVerticalMotion do the same? Otherwise, I feel like this calculation is made even in the Bottle scene.
  2. In the documentation of updateFluid I had to look up the definition of 'interspersed'. Just pointing out that it's not the most common word, and maybe a simpler synonym would be better for readability?
  3. As a sidenote: It feels like these kinds of major changes that need review would greatly benefit from being done in branches,
    because you can see the total diff of the entire scope, instead of commit-wise.

Other than that, seems like a really good change. Thanks @samreid for working on it. Happy to discuss

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants