Properly reset the height scale

AgustinVallejo commented 5 months ago

Currently, the pool scale appears in the middle of the screen after reset. I know somebody already solved this for Buoyancy a while ago, so I'm creating the issue in case somebody knows the easy fix, otherwise, I'll look into it later.

AgustinVallejo commented 5 months ago

This patch fixes this, but wanted to discuss before implementing it:

I moved poolScaleHeightProperty from 3 individual models into DensityBuoyancyModel... But that means adding a property into a base model with a bunch of children who won't ever use that property.
It would also imply changes to density API again. And density doesn't use that property.

So I'm wondering what the optimal solution would be here. As I think creating an intermediate model for B-Lab, BB-Explore and BB-Compare is probably not the best option and would likely make things more complicated. Paging @samreid and @zepumph for their opinion

```diff Subject: [PATCH] Unifying the implementation of poolScaleHeightProperty --- Index: js/buoyancy/model/BuoyancyLabModel.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/buoyancy/model/BuoyancyLabModel.ts b/js/buoyancy/model/BuoyancyLabModel.ts --- a/js/buoyancy/model/BuoyancyLabModel.ts (revision efe889aabb88b1f1faf967aade453dd769076dd8) +++ b/js/buoyancy/model/BuoyancyLabModel.ts (date 1714757019216) @@ -28,7 +28,6 @@ public readonly primaryMass: Cube; public readonly densityExpandedProperty: Property; public readonly showFluidDisplacedProperty: Property; - public readonly poolScaleHeightProperty: NumberProperty; public readonly poolScale: Scale; public constructor( options: BuoyancyLabModelOptions ) { @@ -63,11 +62,6 @@ tandem: tandem.createTandem( 'showFluidDisplacedProperty' ) } ); - this.poolScaleHeightProperty = new NumberProperty( 1, { - range: new Range( 0, 1 ), - tandem: tandem.createTandem( 'poolScaleHeightProperty' ) - } ); - // Pool scale this.poolScale = new VolumelessScale( this.engine, this.gravityProperty, { displayType: DisplayType.NEWTONS, @@ -91,10 +85,6 @@ this.primaryMass.reset(); this.densityExpandedProperty.reset(); - - // The model position of the pool is reset before, so even if this Property value doesn't change, we need to reposition via listeners - this.poolScaleHeightProperty.reset(); - this.poolScaleHeightProperty.notifyListenersStatic(); } } Index: js/buoyancy-basics/model/BuoyancyBasicsExploreModel.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/buoyancy-basics/model/BuoyancyBasicsExploreModel.ts b/js/buoyancy-basics/model/BuoyancyBasicsExploreModel.ts --- a/js/buoyancy-basics/model/BuoyancyBasicsExploreModel.ts (revision efe889aabb88b1f1faf967aade453dd769076dd8) +++ b/js/buoyancy-basics/model/BuoyancyBasicsExploreModel.ts (date 1714756997340) @@ -19,8 +19,6 @@ import densityBuoyancyCommon from '../../densityBuoyancyCommon.js'; import MassTag from '../../common/model/MassTag.js'; import { combineOptions } from '../../../../phet-core/js/optionize.js'; -import NumberProperty from '../../../../axon/js/NumberProperty.js'; -import Range from '../../../../dot/js/Range.js'; import VolumelessScale from '../../common/model/VolumelessScale.js'; type BuoyancyBasicsExploreModelOptions = DensityBuoyancyModelOptions; @@ -32,7 +30,6 @@ public readonly secondaryMass: Cube; public readonly densityExpandedProperty = new BooleanProperty( false ); public readonly percentageSubmergedExpandedProperty = new BooleanProperty( false ); - public readonly poolScaleHeightProperty: NumberProperty; public readonly poolScale: Scale; public constructor( options: BuoyancyBasicsExploreModelOptions ) { @@ -77,12 +74,6 @@ } } ) ); - // TODO: Three repeat declarations for ScaleHeightSliders, https://github.com/phetsims/buoyancy-basics/issues/4 - this.poolScaleHeightProperty = new NumberProperty( 1, { - range: new Range( 0, 1 ), - tandem: tandem.createTandem( 'poolScaleHeightProperty' ) - } ); - // Pool scale this.poolScale = new VolumelessScale( this.engine, this.gravityProperty, { displayType: DisplayType.NEWTONS, Index: js/buoyancy-basics/model/BuoyancyBasicsCompareModel.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/buoyancy-basics/model/BuoyancyBasicsCompareModel.ts b/js/buoyancy-basics/model/BuoyancyBasicsCompareModel.ts --- a/js/buoyancy-basics/model/BuoyancyBasicsCompareModel.ts (revision efe889aabb88b1f1faf967aade453dd769076dd8) +++ b/js/buoyancy-basics/model/BuoyancyBasicsCompareModel.ts (date 1714757017090) @@ -38,7 +38,6 @@ export default class BuoyancyBasicsCompareModel extends BlockSetModel { public readonly densityExpandedProperty = new BooleanProperty( false ); public readonly percentageSubmergedExpandedProperty = new BooleanProperty( false ); - public readonly poolScaleHeightProperty: NumberProperty; public readonly poolScale: Scale; public readonly massProperty: NumberProperty; @@ -279,11 +278,6 @@ } } ) ); - this.poolScaleHeightProperty = new NumberProperty( 1, { - range: new Range( 0, 1 ), - tandem: tandem.createTandem( 'poolScaleHeightProperty' ) - } ); - // Pool scale this.poolScale = new VolumelessScale( this.engine, this.gravityProperty, { displayType: DisplayType.NEWTONS, 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 efe889aabb88b1f1faf967aade453dd769076dd8) +++ b/js/common/model/DensityBuoyancyModel.ts (date 1714757019365) @@ -103,6 +103,8 @@ // Flag that sets an animation to empty the boat of any water inside of it private spillingWaterOutOfBoat = false; + public readonly poolScaleHeightProperty: NumberProperty; + // Scale for the pool, if we are using it protected readonly scale2: Scale | null; @@ -234,6 +236,11 @@ tandem: tandem.createTandem( 'pool' ) } ); + this.poolScaleHeightProperty = new NumberProperty( 1, { + range: new Range( 0, 1 ), + tandem: tandem.createTandem( 'poolScaleHeightProperty' ) + } ); + this.boat = null; this.engine = new P2Engine(); @@ -557,6 +564,10 @@ this.pool.reset(); this.masses.forEach( mass => mass.reset() ); + + // The model position of the pool is reset before, so even if this Property value doesn't change, we need to reposition via listeners + this.poolScaleHeightProperty.reset(); + this.poolScaleHeightProperty.notifyListenersStatic(); } /**

zepumph commented 5 months ago

@samreid had some interesting words about that yesterday for me. I'm going through similar questions about subtypes in the model hierarchy for the B:B compare screen. He mentioned in PDL that there were many subclasses that fulfilled small tasks, and that it worked well in his cases.

I believe that putting this in DBModel is not ideal, and we should try to factor it out in another way. Here are some ideas:

Maybe it could be a special Property where the internals of it's reset function call listeners statically.
I want to call attention to some other Properties that are duplicated a lot. Like densityExpandedProperty. With this info, either:
- (a) recognize that it is ok to "let code be code" and create simple things like Properties where they are needed.
- (b) use other forms of duplication to inform the new subtyping that is desired, since if we do it for this Property, we may want to for others.

Happy to discuss further, but I'd like to hear from @samreid too.

samreid commented 5 months ago

Add class BuoyancyBasicsModel extends DensityBuoyancyModel, and has all of these:

  public readonly densityExpandedProperty = new BooleanProperty( false );
  public readonly percentageSubmergedExpandedProperty = new BooleanProperty( false );
  public readonly poolScale: Scale;
  public readonly poolScaleHeightProperty:

Then BuoyancyBasicsCompareModel extends BuoyancyBasicsModel and BuoyancyBasicsExploreModel extends BuoyancyBasicsModel

Maybe a class or static method to create the poolScaleHeightProperty. It will be instantiated or called twice.

For densityExpandedProperty, is it designed to be in all the Buoyancy screens but none of the Buoyancy Basics screens? Then it would be in BuoyancyModel and not in BuoyancyBasicsModel.

AgustinVallejo commented 5 months ago

But the problem is that BuoyancyLabModel also shares the property addressed in the patch. And the cause of the bug was logic from that model that wasn't replicated in the BuoyancyBasics one. So we can create a model for BuoyancyBasics, but there'll still be repeated code over in the LabModel.

samreid commented 5 months ago

This amount of duplication is OK and is preferable to (a) having poolScaleHeightProperty where it shouldn't be or (b) mixins. Is there another recommendation? Happy to discuss in person.

class BuoyancyLabModel{
  constructor(){
    this.poolScaleHeightProperty = new PoolScaleHeightProperty(tandem);
  }
  reset(){
    this.poolScaleHeightProperty.resetPoolScale();
  }
}
class BuoyancyBasicsModel{
  constructor(){
    this.poolScaleHeightProperty = new PoolScaleHeightProperty(tandem);
  }
  reset(){
    this.poolScaleHeightProperty.resetPoolScale();
  }
}

zepumph commented 5 months ago

I think having a class (PoolScaleHeightProperty) like you have above that obscures the notifiyListenersStatic is best.

phetsims / buoyancy-basics

Properly reset the height scale #7