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

bottle.displacementVolumeProperty doesn't do anything, is it only needed for boat? At least instrumented as such? #178

Closed samreid closed 2 months ago

samreid commented 2 months ago

From https://github.com/phetsims/buoyancy/issues/86

bottle.displacementVolumeProperty doesn't do anything, is it only needed for boat? At least instrumented as such?

samreid commented 2 months ago

This patch passes all precommit hooks and uninstruments the displacementVolumeProperty. I did not yet regenerate the phet-io APIs (so I'm actually not sure why that didn't fail a precommit hook).

```diff Subject: [PATCH] Streamline maxVolume/minVolume, factor out cuboid min/max volume, remove TOLERANCE from cube volume range, see https://github.com/phetsims/density-buoyancy-common/issues/224 --- Index: density-buoyancy-common/js/buoyancy/view/applications/BuoyancyApplicationsScreenView.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/density-buoyancy-common/js/buoyancy/view/applications/BuoyancyApplicationsScreenView.ts b/density-buoyancy-common/js/buoyancy/view/applications/BuoyancyApplicationsScreenView.ts --- a/density-buoyancy-common/js/buoyancy/view/applications/BuoyancyApplicationsScreenView.ts (revision 5d208582d7c5af181f1821c06466beb1b2a7bd54) +++ b/density-buoyancy-common/js/buoyancy/view/applications/BuoyancyApplicationsScreenView.ts (date 1718987659661) @@ -253,7 +253,6 @@ constrainValue: ( value: number ) => { return boatVolumeRange.constrainValue( Utils.roundToInterval( value, 0.1 ) ); }, - phetioLinkedProperty: model.boat.displacementVolumeProperty, majorTickLength: 5, majorTicks: [ { value: boatVolumeRange.min, Index: axon/js/UnitConversionProperty.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/axon/js/UnitConversionProperty.ts b/axon/js/UnitConversionProperty.ts --- a/axon/js/UnitConversionProperty.ts (revision 950e37137c9117cbe6b0fb154ce00403b1780151) +++ b/axon/js/UnitConversionProperty.ts (date 1718987515542) @@ -30,6 +30,7 @@ import { DEFAULT_RANGE } from './NumberProperty.js'; import optionize from '../../phet-core/js/optionize.js'; import TRangedProperty, { isTRangedProperty } from './TRangedProperty.js'; +import TReadOnlyProperty from './TReadOnlyProperty.js'; type SelfOptions = { // The multiplicative factor to convert from INPUT => OUTPUT, e.g. @@ -44,10 +45,10 @@ public readonly rangeProperty: TProperty; - private _property: ( TProperty | TRangedProperty ); - private _rangeListener?: ( range: Range ) => void; + private readonly _property: ( TReadOnlyProperty | TRangedProperty ); + private readonly _rangeListener?: ( range: Range ) => void; - public constructor( property: ( TProperty | TRangedProperty ), providedOptions: UnitConversionPropertyOptions ) { + public constructor( property: ( TReadOnlyProperty | TRangedProperty ), providedOptions: UnitConversionPropertyOptions ) { const map = ( input: number ) => input * providedOptions.factor; const inverseMap = ( output: number ) => output / providedOptions.factor; Index: density-buoyancy-common/js/buoyancy/model/applications/ApplicationsMass.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/density-buoyancy-common/js/buoyancy/model/applications/ApplicationsMass.ts b/density-buoyancy-common/js/buoyancy/model/applications/ApplicationsMass.ts --- a/density-buoyancy-common/js/buoyancy/model/applications/ApplicationsMass.ts (revision 5d208582d7c5af181f1821c06466beb1b2a7bd54) +++ b/density-buoyancy-common/js/buoyancy/model/applications/ApplicationsMass.ts (date 1718987445766) @@ -14,6 +14,7 @@ import Vector3 from '../../../../../dot/js/Vector3.js'; import Bounds3 from '../../../../../dot/js/Bounds3.js'; import StrictOmit from '../../../../../phet-core/js/types/StrictOmit.js'; +import TReadOnlyProperty from '../../../../../axon/js/TReadOnlyProperty.js'; export type ApplicationsMassOptions = StrictOmit; @@ -23,9 +24,9 @@ protected static readonly DEFAULT_DISPLACEMENT_VOLUME = 0.01; // The volume that the boat or bottle can hold inside them, in m^3. - public displacementVolumeProperty: NumberProperty; + public readonly displacementVolumeProperty: TReadOnlyProperty; - protected massLabelOffsetVector3: Vector3; + protected readonly massLabelOffsetVector3: Vector3; protected constructor( engine: PhysicsEngine, options: ApplicationsMassOptions ) { Index: density-buoyancy-common/js/buoyancy/model/applications/Boat.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/density-buoyancy-common/js/buoyancy/model/applications/Boat.ts b/density-buoyancy-common/js/buoyancy/model/applications/Boat.ts --- a/density-buoyancy-common/js/buoyancy/model/applications/Boat.ts (revision 5d208582d7c5af181f1821c06466beb1b2a7bd54) +++ b/density-buoyancy-common/js/buoyancy/model/applications/Boat.ts (date 1718987674761) @@ -216,7 +216,6 @@ * Resets values to their original state */ public override reset(): void { - this.displacementVolumeProperty.reset(); this.basin.reset(); ```

If this looks good so far, can we go even further by making it a non-Property constant number?

zepumph commented 2 months ago

@samreid and I discussed, and we think it is best and easiest to just opt out of instrumenting the displacementVolumeProperty for the bottle. We can keep the boat implementation the same.

zepumph commented 2 months ago

Commits above. Things are much better with an abstract implementation in the base class.