Combine Boat and Bottle implementations wherever possible

[AgustinVallejo]

In some places, the Bottle and Boat implementations have repeated code or could benefit from common patterns. This is a meta-issue to identify the places this could be done in.

Also, relieving https://github.com/phetsims/density-buoyancy-common/issues/86 of some TODO's

[zepumph]

@AgustinVallejo and I will take a first pass and get back to @jonathanolson with questions.

[AgustinVallejo]

The following is a patch for this work. We can review it together. As far as I tested, it works:

```diff Subject: [PATCH] Batch Patch for Bottle and Boat Parent Class --- Index: js/buoyancy/model/ApplicationsMass.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/buoyancy/model/ApplicationsMass.ts b/js/buoyancy/model/ApplicationsMass.ts new file mode 100644 --- /dev/null (date 1708532641327) +++ b/js/buoyancy/model/ApplicationsMass.ts (date 1708532641327) @@ -0,0 +1,134 @@ +// Copyright 2019-2024, University of Colorado Boulder + +/** + * A general class for shared functionality between the boat and bottle. + * + * @author Agustín Vallejo + */ + +import NumberProperty from '../../../../axon/js/NumberProperty.js'; +import ThreeUtils from '../../../../mobius/js/ThreeUtils.js'; +import Mass, { InstrumentedMassOptions, MassOptions } from '../../common/model/Mass.js'; +import densityBuoyancyCommon from '../../densityBuoyancyCommon.js'; +import PhysicsEngine from '../../common/model/PhysicsEngine.js'; +import Ray3 from '../../../../dot/js/Ray3.js'; +import optionize, { EmptySelfOptions } from '../../../../phet-core/js/optionize.js'; +import Vector2 from '../../../../dot/js/Vector2.js'; +import Vector3 from '../../../../dot/js/Vector3.js'; +import { Bounds3 } from '../../../../dot/js/imports.js'; + +export type ApplicationsMassOptions = InstrumentedMassOptions; + +export default class ApplicationsMass extends Mass { + + // The volume that the boat or bottle can hold inside them. + public displacementVolumeProperty: NumberProperty; + + protected massLabelOffsetVector3: Vector3; + + public readonly intersectionGroup: THREE.Group; + + public constructor( engine: PhysicsEngine, providedOptions: ApplicationsMassOptions ) { + + const options = optionize()( {}, providedOptions ); + + assert && assert( !options.canRotate ); + + super( engine, options ); + + this.displacementVolumeProperty = new NumberProperty( 0.01 ); + + const bounds = this.shapeProperty.value.getBounds(); + + // Mass label on the bottom left of the mass, top because the Y is flipped. + this.massLabelOffsetVector3 = new Vector3( bounds.left, bounds.top, 0 ); + + this.massLabelOffsetOrientationProperty.value = new Vector2( 1, -1 / 2 ); + this.massLabelOffsetProperty.value = this.massLabelOffsetVector3; + + this.intersectionGroup = new THREE.Group(); + } + + /** + * A box that contains the geometry of the Mass + */ + public override getLocalBounds(): Bounds3 { + const bounds2 = this.shapeProperty.value.bounds; + return new Bounds3( bounds2.minX, bounds2.minY, -bounds2.minY, bounds2.maxX, bounds2.maxY, bounds2.minY ); + } + + /** + * If there is an intersection with the ray and this mass, the t-value (distance the ray would need to travel to + * reach the intersection, e.g. ray.position + ray.distance * t === intersectionPoint) will be returned. Otherwise + * if there is no intersection, null will be returned. + */ + public override intersect( ray: Ray3, isTouch: boolean, scale = 1 ): number | null { + const translation = this.matrix.translation; + const adjustedPosition = ray.position.minusXYZ( translation.x, translation.y, 0 ).dividedScalar( scale ); + + const raycaster = new THREE.Raycaster( ThreeUtils.vectorToThree( adjustedPosition ), ThreeUtils.vectorToThree( ray.direction ) ); + const intersections: THREE.Intersection[] = []; + raycaster.intersectObject( this.intersectionGroup, true, intersections ); + + return intersections.length ? intersections[ 0 ].distance * scale : null; + } + + + // TODO: should this be abstract?? https://github.com/phetsims/density-buoyancy-common/issues/91 + protected evaluatePiecewiseLinearArea( ratio: number ): number { + // see subclass for implementation + return 1; + } + + protected evaluatePiecewiseLinearVolume( ratio: number ): number { + // see subclass for implementation + return 1; + } + + /** + * Returns the displayed area of this object at a given y level + * + * Assumes step information was updated. + */ + public getDisplacedArea( liquidLevel: number ): number { + const bottom = this.stepBottom; + const top = this.stepTop; + + if ( liquidLevel < bottom || liquidLevel > top ) { + return 0; + } + + const ratio = ( liquidLevel - bottom ) / ( top - bottom ); + + return this.evaluatePiecewiseLinearArea( ratio ); + } + + /** + * Returns the displaced volume of this object up to a given y level, assuming a y value for the given liquid level. + * + * Assumes step information was updated. + */ + public getDisplacedVolume( liquidLevel: number ): number { + const bottom = this.stepBottom; + const top = this.stepTop; + + if ( liquidLevel <= bottom ) { + return 0; + } + else if ( liquidLevel >= top ) { + return this.displacementVolumeProperty.value; + } + else { + const ratio = ( liquidLevel - bottom ) / ( top - bottom ); + + return this.evaluatePiecewiseLinearVolume( ratio ); + } + } + + public setRatios( widthRatio: number, heightRatio: number ): void { + // See subclass for implementation + } + +} + +densityBuoyancyCommon.register( 'ApplicationsMass', ApplicationsMass ); 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 a76908fb6a1cd8013bf88141ea5781345efc0edc) +++ b/js/buoyancy/model/Boat.ts (date 1708532641333) @@ -11,7 +11,6 @@ import StrictOmit from '../../../../phet-core/js/types/StrictOmit.js'; import Utils from '../../../../dot/js/Utils.js'; import { Shape } from '../../../../kite/js/imports.js'; -import ThreeUtils from '../../../../mobius/js/ThreeUtils.js'; import Mass, { InstrumentedMassOptions, MassOptions } from '../../common/model/Mass.js'; import Material from '../../common/model/Material.js'; import densityBuoyancyCommon from '../../densityBuoyancyCommon.js'; @@ -25,16 +24,13 @@ import IOType from '../../../../tandem/js/types/IOType.js'; import { MassShape } from '../../common/model/MassShape.js'; import optionize, { EmptySelfOptions } from '../../../../phet-core/js/optionize.js'; -import Vector2 from '../../../../dot/js/Vector2.js'; -import Vector3 from '../../../../dot/js/Vector3.js'; -import { Bounds3 } from '../../../../dot/js/imports.js'; +import ApplicationsMass from './ApplicationsMass.js'; export type BoatOptions = StrictOmit; -export default class Boat extends Mass { +export default class Boat extends ApplicationsMass { // The volume that the boat can hold inside it. - public readonly displacementVolumeProperty: NumberProperty; public readonly liquidMaterialProperty: TProperty; public readonly basin: BoatBasin; @@ -44,8 +40,6 @@ // How to multiply our one-liter size up to the model coordinates public stepMultiplier: number; - public readonly intersectionGroup: THREE.Group; - public constructor( engine: PhysicsEngine, blockWidthProperty: TReadOnlyProperty, liquidMaterialProperty: TProperty, providedOptions: BoatOptions ) { const displacementVolumeProperty = new NumberProperty( 0.01 ); @@ -63,12 +57,8 @@ material: Material.BOAT_BODY }, providedOptions ); - assert && assert( !options.canRotate ); - super( engine, options ); - const massLabelOffsetVector3 = new Vector3( 0, 0, 0 ); - // Update the shape when the block width or displacement changes Multilink.multilink( [ blockWidthProperty, displacementVolumeProperty ], ( blockWidth, displacementVolume ) => { if ( displacementVolume === 0 ) { @@ -83,7 +73,7 @@ // Mass label on the bottom left of the boat, top because the shape is flipped. const bounds = this.shapeProperty.value.getBounds(); - massLabelOffsetVector3.setXYZ( bounds.left, bounds.top, 0 ); + this.massLabelOffsetVector3.setXYZ( bounds.left, bounds.top, 0 ); this.volumeLock = true; this.volumeProperty.value = volume; @@ -95,11 +85,12 @@ this.displacementVolumeProperty = displacementVolumeProperty; this.liquidMaterialProperty = liquidMaterialProperty; - this.massLabelOffsetOrientationProperty.value = new Vector2( 1, -1 / 2 ); - this.massLabelOffsetProperty.value = massLabelOffsetVector3; this.basin = new BoatBasin( this ); + this.stepInternalVolume = 0; + this.stepMultiplier = 0; + Multilink.multilink( [ this.liquidMaterialProperty, this.basin.liquidVolumeProperty ], ( material, volume ) => { this.containedMassProperty.value = material.density * volume; } ); @@ -107,18 +98,10 @@ this.stepInternalVolume = 0; this.stepMultiplier = 0; - this.intersectionGroup = new THREE.Group(); const intersectionMesh = new THREE.Mesh( BoatDesign.getPrimaryGeometry( 1 ), new THREE.MeshLambertMaterial() ); this.intersectionGroup.add( intersectionMesh ); } - - public override getLocalBounds(): Bounds3 { - const bounds2 = this.shapeProperty.value.bounds; - // TODO: Boat geometry is more complex than this, could be improved https://github.com/phetsims/buoyancy/issues/76 - return new Bounds3( bounds2.minX, bounds2.minY, -bounds2.minY, bounds2.maxX, bounds2.maxY, bounds2.minY ); - } - /** * Steps forward in time. */ @@ -166,55 +149,16 @@ */ public override intersect( ray: Ray3, isTouch: boolean ): number | null { const scale = Math.pow( this.displacementVolumeProperty.value / 0.001, 1 / 3 ); - // TODO: somewhat borrowed with Bottle, let's combine https://github.com/phetsims/density-buoyancy-common/issues/91 - const translation = this.matrix.translation; - const adjustedPosition = ray.position.minusXYZ( translation.x, translation.y, 0 ).dividedScalar( scale ); - - const raycaster = new THREE.Raycaster( ThreeUtils.vectorToThree( adjustedPosition ), ThreeUtils.vectorToThree( ray.direction ) ); - const intersections: THREE.Intersection[] = []; - raycaster.intersectObject( this.intersectionGroup, true, intersections ); - - return intersections.length ? intersections[ 0 ].distance * scale : null; + return super.intersect( ray, isTouch, scale ); } - /** - * Returns the displayed area of this object at a given y level - * - * Assumes step information was updated. - */ - public getDisplacedArea( liquidLevel: number ): number { - const bottom = this.stepBottom; - const top = this.stepTop; - if ( liquidLevel < bottom || liquidLevel > top ) { - return 0; - } - - const ratio = ( liquidLevel - bottom ) / ( top - bottom ); - + public override evaluatePiecewiseLinearArea( ratio: number ): number { return Mass.evaluatePiecewiseLinear( BoatDesign.ONE_LITER_DISPLACED_AREAS, ratio ) * this.stepMultiplier * this.stepMultiplier; } - /** - * Returns the displaced volume of this object up to a given y level, assuming a y value for the given liquid level. - * - * Assumes step information was updated. - */ - public getDisplacedVolume( liquidLevel: number ): number { - const bottom = this.stepBottom; - const top = this.stepTop; - - if ( liquidLevel <= bottom ) { - return 0; - } - else if ( liquidLevel >= top ) { - return this.displacementVolumeProperty.value; - } - else { - const ratio = ( liquidLevel - bottom ) / ( top - bottom ); - - return Mass.evaluatePiecewiseLinear( BoatDesign.ONE_LITER_DISPLACED_VOLUMES, ratio ) * this.stepMultiplier * this.stepMultiplier * this.stepMultiplier; - } + public override evaluatePiecewiseLinearVolume( ratio: number ): number { + return Mass.evaluatePiecewiseLinear( BoatDesign.ONE_LITER_DISPLACED_VOLUMES, ratio ) * this.stepMultiplier * this.stepMultiplier * this.stepMultiplier; } /** @@ -258,10 +202,6 @@ } } - public setRatios( widthRatio: number, heightRatio: number ): void { - // See subclass for implementation - } - /** * Resets values to their original state */ Index: js/buoyancy/model/Bottle.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/buoyancy/model/Bottle.ts b/js/buoyancy/model/Bottle.ts --- a/js/buoyancy/model/Bottle.ts (revision a76908fb6a1cd8013bf88141ea5781345efc0edc) +++ b/js/buoyancy/model/Bottle.ts (date 1708532314840) @@ -74,19 +74,17 @@ import Vector2 from '../../../../dot/js/Vector2.js'; import Vector3 from '../../../../dot/js/Vector3.js'; import { Shape } from '../../../../kite/js/imports.js'; -import ThreeUtils from '../../../../mobius/js/ThreeUtils.js'; import optionize, { EmptySelfOptions } from '../../../../phet-core/js/optionize.js'; import Mass, { InstrumentedMassOptions } from '../../common/model/Mass.js'; import Material from '../../common/model/Material.js'; import densityBuoyancyCommon from '../../densityBuoyancyCommon.js'; import DensityBuoyancyCommonStrings from '../../DensityBuoyancyCommonStrings.js'; import PhysicsEngine from '../../common/model/PhysicsEngine.js'; -import Ray3 from '../../../../dot/js/Ray3.js'; import Multilink from '../../../../axon/js/Multilink.js'; import IOType from '../../../../tandem/js/types/IOType.js'; import { MassShape } from '../../common/model/MassShape.js'; import ReadOnlyProperty from '../../../../axon/js/ReadOnlyProperty.js'; -import { Bounds3 } from '../../../../dot/js/imports.js'; +import ApplicationsMass from './ApplicationsMass.js'; // constants (in logical coordinates) const BODY_CORNER_RADIUS = 0.02; // Used both between the taper/body and between the body/base @@ -173,7 +171,7 @@ export type BottleOptions = StrictOmit; -export default class Bottle extends Mass { +export default class Bottle extends ApplicationsMass { // model-coordinate bounds in x,y private readonly bottleBounds: Bounds2; @@ -186,7 +184,6 @@ public readonly primaryGeometry: THREE.BufferGeometry; public readonly capGeometry: THREE.BufferGeometry; - public readonly intersectionGroup: THREE.Group; public constructor( engine: PhysicsEngine, providedOptions: BottleOptions ) { @@ -202,21 +199,11 @@ massShape: MassShape.BLOCK }, providedOptions ); - assert && assert( !options.canRotate ); - super( engine, options ); - const massLabelOffsetVector3 = new Vector3( 0, 0, 0 ); - this.bottleBounds = Bounds2.NOTHING.copy(); Bottle.getFlatIntersectionVertices().forEach( p => this.bottleBounds.addPoint( p ) ); - // Mass label on the bottom left of the boat, top because the shape is flipped. - massLabelOffsetVector3.setXYZ( this.bottleBounds.left, this.bottleBounds.top, 0 ); - - this.massLabelOffsetOrientationProperty.value = new Vector2( 1, -1 / 2 ); - this.massLabelOffsetProperty.value = massLabelOffsetVector3; - this.interiorMaterialProperty = new Property( BOTTLE_INITIAL_INTERIOR_MATERIAL, { valueType: Material, reentrant: true, @@ -244,9 +231,10 @@ this.primaryGeometry = Bottle.getPrimaryGeometry(); this.capGeometry = Bottle.getCapGeometry(); - this.intersectionGroup = new THREE.Group(); this.intersectionGroup.add( new THREE.Mesh( this.primaryGeometry, new THREE.MeshLambertMaterial() ) ); this.intersectionGroup.add( new THREE.Mesh( this.capGeometry, new THREE.MeshLambertMaterial() ) ); + + this.displacementVolumeProperty.value = BOTTLE_VOLUME; } /** @@ -266,65 +254,12 @@ this.stepTop = yOffset + this.bottleBounds.maxY; } - /** - * If there is an intersection with the ray and this mass, the t-value (distance the ray would need to travel to - * reach the intersection, e.g. ray.position + ray.distance * t === intersectionPoint) will be returned. Otherwise - * if there is no intersection, null will be returned. - */ - public override intersect( ray: Ray3, isTouch: boolean ): number | null { - const translation = this.matrix.translation; - const adjustedPosition = ray.position.minusXYZ( translation.x, translation.y, 0 ); - - const raycaster = new THREE.Raycaster( ThreeUtils.vectorToThree( adjustedPosition ), ThreeUtils.vectorToThree( ray.direction ) ); - const intersections: THREE.Intersection[] = []; - raycaster.intersectObject( this.intersectionGroup, true, intersections ); - - return intersections.length ? intersections[ 0 ].distance : null; - } - - public override getLocalBounds(): Bounds3 { - const bounds2 = this.shapeProperty.value.bounds; - return new Bounds3( bounds2.minX, bounds2.minY, -bounds2.minY, bounds2.maxX, bounds2.maxY, bounds2.minY ); - } - - /** - * Returns the cumulative displaced volume of this object up to a given y level. - * - * Assumes step information was updated. - */ - public getDisplacedArea( liquidLevel: number ): number { - const bottom = this.stepBottom; - const top = this.stepTop; - - if ( liquidLevel < bottom || liquidLevel > top ) { - return 0; - } - - const ratio = ( liquidLevel - bottom ) / ( top - bottom ); - + public override evaluatePiecewiseLinearArea( ratio: number ): number { return Mass.evaluatePiecewiseLinear( TEN_LITER_DISPLACED_AREAS, ratio ); } - /** - * Returns the displaced volume of this object up to a given y level, assuming a y value for the given liquid level. - * - * Assumes step information was updated. - */ - public getDisplacedVolume( liquidLevel: number ): number { - const bottom = this.stepBottom; - const top = this.stepTop; - - if ( liquidLevel <= bottom ) { - return 0; - } - else if ( liquidLevel >= top ) { - return BOTTLE_VOLUME; - } - else { - const ratio = ( liquidLevel - bottom ) / ( top - bottom ); - - return Mass.evaluatePiecewiseLinear( TEN_LITER_DISPLACED_VOLUMES, ratio ); - } + public override evaluatePiecewiseLinearVolume( ratio: number ): number { + return Mass.evaluatePiecewiseLinear( TEN_LITER_DISPLACED_VOLUMES, ratio ); } /** @@ -1238,10 +1173,6 @@ return canvas; } - public setRatios( widthRatio: number, heightRatio: number ): void { - // See subclass for implementation - } - // The number to scale the original values by to get a 10L-volume bottle public static readonly TEN_LITER_SCALE_MULTIPLIER = TEN_LITER_SCALE_MULTIPLIER;

[phet-dev]

Reopening because there is a TODO marked for this issue.

[AgustinVallejo]

Looked at this for some time and for the life of me couldn't figure out either what the duplication was or even what the code meant in the first place. So I moved the TODO to the legacy list: https://github.com/phetsims/density-buoyancy-common/issues/86

Closing this one