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

New component: ScaleHeightSlider #107

Closed
zepumph opened this issue Mar 25, 2024 · 19 comments
Closed

New component: ScaleHeightSlider #107

zepumph opened this issue Mar 25, 2024 · 19 comments

Comments

@zepumph
Copy link
Member

zepumph commented Mar 25, 2024

From phetsims/buoyancy#113, the scale in the water on the lab screen will be a bit different.

image

image

The slider should be "attached" to the scale, and the height adjusted accordingly.

  • Scale isn't draggable
  • Scale doesn't move down when items are placed on it.
  • Can items go under it?
  • Noting that likely this is needed for Buoyancy Basics too.
  • Probably it is best to have this slider on top of the THREE scene, and to not try to put it into THREE (since we need to handle the slider input).
@AgustinVallejo
Copy link
Contributor

AgustinVallejo commented Mar 29, 2024

A worry I have about this, upon first draft implementation, is that it seems the slider wont fit when the panels are set for dev bounds. Any ideas @DianaTavares ?

image

(Again, this is a first draft, far from the final look of the feature)

@AgustinVallejo
Copy link
Contributor

Above commit implements most of the feature, but there's some necessary work: For this to properly function, the scale has to have canMove: true. However, if that's done, the Scale will try to have keyboard and mouse interaction, which we don't want and are also breaking the sim. If canMove: false, then we don't have interaction but also it's really difficult to change the position of the body.

Pushing and leaving a TODO for us to work on this together.

@AgustinVallejo
Copy link
Contributor

Also, this is how it looks. Still worried about possible overlap with the panel:

image

@zepumph
Copy link
Member Author

zepumph commented Apr 1, 2024

@AgustinVallejo took a look about how if canMove is false, the physics body is static (can't get pushed around), but if canMove is true, then we get input listeners. We should determine how best to handle this, but when canMove is true, we still get the scale "bouncing" as an object is put on it, so probably we want to investigate using isStatic:true in some form (maybe needing to toggle it to update the position??).

@zepumph
Copy link
Member Author

zepumph commented Apr 5, 2024

  • We will want to fix the overlap at some point:
    image

@zepumph
Copy link
Member Author

zepumph commented Apr 5, 2024

@samreid and I basically got to a commit point on this today. It involved recognizing the scale as a "KINEMATIC" body type, which isn't supported by density/buoyancy, but is in P2. We just need to add support for this. @samreid saw this as a pretty standard third body type, so a general supporting feature to PhysicsEngine seems possible and nice.

Subject: [PATCH] can't move ground scale, https://github.com/phetsims/buoyancy/issues/113
---
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 fef8d2d2d41e83302a53f0e7e7aba6ed3b78c567)
+++ b/js/buoyancy/model/BuoyancyLabModel.ts	(date 1712356266269)
@@ -28,6 +28,7 @@
   public readonly densityExpandedProperty: Property<boolean>;
   public readonly showFluidDisplacedProperty: Property<boolean>;
   public readonly poolScaleHeightProperty: NumberProperty;
+  public readonly poolScale: Scale;
 
   public constructor( options: BuoyancyLabModelOptions ) {
 
@@ -65,9 +66,9 @@
     } );
 
     // Pool scale
-    const poolScaleDefaultPosition = new Vector2( 0.35, -Scale.SCALE_BASE_BOUNDS.minY + 0.5 * this.poolBounds.minY );
-    const poolScale = new Scale( this.engine, this.gravityProperty, {
-      matrix: Matrix3.translation( poolScaleDefaultPosition.x, poolScaleDefaultPosition.y ),
+    this.poolScale = new Scale( this.engine, this.gravityProperty, {
+      // TODO: DUplicateion with propert listener in screen view,https://github.com/phetsims/density-buoyancy-common/issues/107
+      matrix: Matrix3.translation( 0.35, this.poolBounds.minY + Scale.SCALE_HEIGHT / 2 ),
       displayType: DisplayType.NEWTONS,
       tandem: tandem.createTandem( 'poolScale' ),
       canMove: false, //TODO This should be true, but first some work is needed https://github.com/phetsims/density-buoyancy-common/issues/107
@@ -76,17 +77,11 @@
       }
     } );
 
-    poolScale.startDrag( poolScale.matrix.translation );
-
-    this.poolScaleHeightProperty.lazyLink( height => {
-      const modelHeight = -0.5 * ( 2 - height ) * this.poolBounds.height - 2 * Scale.SCALE_BASE_BOUNDS.minY;
-      poolScale.updateDrag( poolScale.matrix.translation.setXY( poolScaleDefaultPosition.x, modelHeight ) );
-    } );
-
-    this.availableMasses.push( poolScale );
+    // Make sure to render it
+    this.availableMasses.push( this.poolScale );
 
     // Adjust pool volume so that it's at the desired value WITH the pool scale inside.
-    this.pool.liquidVolumeProperty.value -= poolScale.volumeProperty.value;
+    this.pool.liquidVolumeProperty.value -= this.poolScale.volumeProperty.value;
     this.pool.liquidVolumeProperty.setInitialValue( this.pool.liquidVolumeProperty.value );
   }
 
Index: js/common/model/P2Engine.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/model/P2Engine.ts b/js/common/model/P2Engine.ts
--- a/js/common/model/P2Engine.ts	(revision fef8d2d2d41e83302a53f0e7e7aba6ed3b78c567)
+++ b/js/common/model/P2Engine.ts	(date 1712352270856)
@@ -319,7 +319,7 @@
    */
   public createBox( width: number, height: number, isStatic?: boolean ): PhysicsEngineBody {
     const body = new p2.Body( {
-      type: isStatic ? p2.Body.STATIC : p2.Body.DYNAMIC,
+      type: isStatic === 'KINEMATIC' ? p2.Body.KINEMATIC : isStatic ? p2.Body.STATIC : p2.Body.DYNAMIC,
       fixedRotation: true
     } );
 
Index: js/buoyancy/view/BuoyancyLabScreenView.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/buoyancy/view/BuoyancyLabScreenView.ts b/js/buoyancy/view/BuoyancyLabScreenView.ts
--- a/js/buoyancy/view/BuoyancyLabScreenView.ts	(revision fef8d2d2d41e83302a53f0e7e7aba6ed3b78c567)
+++ b/js/buoyancy/view/BuoyancyLabScreenView.ts	(date 1712356268749)
@@ -32,6 +32,8 @@
 import ScaleView from '../../common/view/ScaleView.js';
 import Bounds2 from '../../../../dot/js/Bounds2.js';
 import ScaleHeightSlider from '../../common/view/ScaleHeightSlider.js';
+import Utils from '../../../../dot/js/Utils.js';
+import Scale from '../../common/model/Scale.js';
 
 // constants
 const MARGIN = DensityBuoyancyCommonConstants.MARGIN;
@@ -183,6 +185,25 @@
       waterLevelSlider.bottom = bottomRightPoolPoint.y;
       waterLevelSlider.left = bottomRightPoolPoint.x + 5;
     };
+    const halfScaleHeight = Scale.SCALE_HEIGHT / 2;
+    const maxY = model.pool.liquidYInterpolatedProperty.value - halfScaleHeight;
+
+    model.poolScaleHeightProperty.link( height => {
+
+      const minY = model.poolBounds.minY + halfScaleHeight;
+
+      const currentHeight = Utils.linear( 0, 1, minY, maxY, height );
+
+      model.poolScale.matrix.set12( currentHeight );
+      model.poolScale.writeData();
+      model.poolScale.transformedEmitter.emit();
+
+      // const modelHeight = -0.5 * ( 2 - height ) * model.poolBounds.height - 2 * Scale.SCALE_BASE_BOUNDS.minY;
+      // const newPosition = poolScale.matrix.translation.setXY( poolScaleDefaultPosition.x, modelHeight );
+      // poolScale.body.position[ 1 ] = currentHeight;
+    } );
+
+
   }
 
   private static getFluidDisplacedPanelScaleIcon(): Node {
Index: js/common/model/Scale.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/model/Scale.ts b/js/common/model/Scale.ts
--- a/js/common/model/Scale.ts	(revision fef8d2d2d41e83302a53f0e7e7aba6ed3b78c567)
+++ b/js/common/model/Scale.ts	(date 1712352270860)
@@ -79,7 +79,7 @@
 
   public constructor( engine: PhysicsEngine, gravityProperty: TProperty<Gravity>, providedOptions: ScaleOptions ) {
     const config = optionize<ScaleOptions, SelfOptions, InstrumentedMassOptions>()( {
-      body: engine.createBox( SCALE_WIDTH, SCALE_HEIGHT, providedOptions.canMove === false ),
+      body: engine.createBox( SCALE_WIDTH, SCALE_HEIGHT, 'KINEMATIC' ),
       shape: Shape.rect( -SCALE_WIDTH / 2, -SCALE_HEIGHT / 2, SCALE_WIDTH, SCALE_HEIGHT ),
       volume: SCALE_VOLUME,
       massShape: MassShape.BLOCK,

zepumph added a commit that referenced this issue Apr 8, 2024
Signed-off-by: Michael Kauzmann <michael.kauzmann@colorado.edu>
zepumph added a commit that referenced this issue Apr 8, 2024
Signed-off-by: Michael Kauzmann <michael.kauzmann@colorado.edu>
@zepumph
Copy link
Member Author

zepumph commented Apr 8, 2024

I got to a commit point. It turns out we didn't need KINEMATIC after all, you can move static objects around too. Still some TODOs and design questions.

@zepumph
Copy link
Member Author

zepumph commented Apr 8, 2024

Next steps here:

  1. @DianaTavares: The scale needs to decide if it wants to be just out of the water, or just below the surface. It doesn't change the physics, it just can't be equal (because it looks graphically weird). @DianaTavares would you like the scale to be able to emerge just slightly? Like this?
    image
  2. @DianaTavares, the length of the slider may want some fine tuning. Note how it isn't totally aligned with the path of the scale, since the scale's position is tracked in its center, but it goes from where the bottom of the scale is at the bottom of the pool, to where the top of the scale is at the top of the pool. We may want to make sure the length of the slider means that it is a 1:1 dragging relationship. While doing this, we may choose positionWaterLevelSlider.
  3. I wanted to confirm that the green slider thumb is the color you were shooting for.
  4. I'd like to experiment with moving this out into its own class. I'm unsure if it is worth it though.
  5. It seems like there is a bug, when the scale is all the way out of the water, I am still .3N less than the scale on the ground. I'll need to take a look.

zepumph added a commit that referenced this issue Apr 8, 2024
Signed-off-by: Michael Kauzmann <michael.kauzmann@colorado.edu>
zepumph added a commit that referenced this issue Apr 8, 2024
Signed-off-by: Michael Kauzmann <michael.kauzmann@colorado.edu>
@zepumph zepumph changed the title New component: StickyScale New component: ScaleHeightSlider Apr 8, 2024
zepumph added a commit that referenced this issue Apr 8, 2024
Signed-off-by: Michael Kauzmann <michael.kauzmann@colorado.edu>
zepumph added a commit that referenced this issue Apr 9, 2024
Signed-off-by: Michael Kauzmann <michael.kauzmann@colorado.edu>
@zepumph
Copy link
Member Author

zepumph commented Apr 9, 2024

@DianaTavares, please note that I believe it will be fastest and easiest to discuss this issue in person from here. All work for me is done.

It seems like there is a bug, when the scale is all the way out of the water, I am still .3N less than the scale on the ground. I'll need to take a look.

This is because the p2 engine uses a stiffness that allows for some overlap.

@zepumph
Copy link
Member Author

zepumph commented Apr 15, 2024

From design discussion:

(1): scale taking up volume and block buoyant force

  • We want the scale to be able to be above water enough such that the cube weighs the same amount as the ground scale.
  • Let's try fudging the model, and having the Scale have 0 volume in the p2 engine so that bringing it out above the water doesn't change the displaced fluid.

(2) slider movement vs scale movement

  • It is important to have a 1:1 distance relationship between the scale and the slider controlling it. We care less if it is aligned with the bottom/middle/top of the scale, it just needs to be consistent.

(3) slider thumb overlapping with panels

  • Make the margins for the panels much smaller (5 instead of 10)
  • Change the slider to have a custom thumb, please use the same one for the mass/volume control (note the pointy edges)
  • Make it the same color as the default slider thumb (with highlight coloring too) instead of green to start.

@AgustinVallejo
Copy link
Contributor

AgustinVallejo commented Apr 15, 2024

It is important to have a 1:1 distance relationship between the scale and the slider controlling it. We care less if it is aligned with the bottom/middle/top of the scale, it just needs to be consistent.

Achieved this, but I'm not confortable with the solution. Currently it's hard-coded so the slider's height is 125 (previously it was 150)... I would very much like this number to be obtained from the pool and scale's dimensions, but my first attempts failed. Left a TODO and will check later.

@AgustinVallejo
Copy link
Contributor

@DianaTavares Please review and close if ready :)

@DianaTavares
Copy link

The slider is amazing!! Thanks!!

@zepumph
Copy link
Member Author

zepumph commented Apr 18, 2024

I'm seeing that the slider doesn't quite align with the bottom of the scale. I believe this is because of the z dimention, and should be an easy offset, especially now that we have a THREE-supported MVT from #113. I'll take a look.

image

@zepumph
Copy link
Member Author

zepumph commented Apr 19, 2024

Two thoughts here:

  1. It seems better to use the front of the scale z instead of the front of the pool z:
Subject: [PATCH] custom margins when space is tight, https://github.com/phetsims/buoyancy/issues/144
---
Index: js/buoyancy/view/BuoyancyLabScreenView.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/buoyancy/view/BuoyancyLabScreenView.ts b/js/buoyancy/view/BuoyancyLabScreenView.ts
--- a/js/buoyancy/view/BuoyancyLabScreenView.ts	(revision 905b50bcb4a7becc66fe06087a93acb6324e9b2d)
+++ b/js/buoyancy/view/BuoyancyLabScreenView.ts	(date 1713562889583)
@@ -213,7 +213,7 @@
       const bottomRightPoolPoint = this.modelToViewPoint( new Vector3(
         this.model.poolBounds.maxX,
         this.model.poolBounds.minY,
-        this.model.poolBounds.maxZ
+        this.model.poolScale.getBounds().maxZ
       ) );
       scaleHeightSlider.bottom = bottomRightPoolPoint.y;
       scaleHeightSlider.left = bottomRightPoolPoint.x + DensityBuoyancyCommonConstants.MARGIN / 2;
  1. Setting the bottom of the slider seems to be the problem. We really want to set the bottom of the track. That will line things up better. Perhaps there is an API for that, or something we can create easily in slider to provide that track-based positioning.

zepumph added a commit that referenced this issue Apr 22, 2024
Signed-off-by: Michael Kauzmann <michael.kauzmann@colorado.edu>
zepumph added a commit that referenced this issue Apr 22, 2024
Signed-off-by: Michael Kauzmann <michael.kauzmann@colorado.edu>
@zepumph
Copy link
Member Author

zepumph commented Apr 22, 2024

Alright. I think I have gotten this. @AgustinVallejo, can you give me a spot check? It is complex, but it was the best I could think of. Two things here:

  1. I like having the layout responsibility in the ScaleHeightSlider so that it is the only thing that knows how tall the thumb is to offset for the track bottom.
  2. Two different modelToViewPoint() calls helps us pinpoint exactly where we want the x and y, since I don't think we can do it all in one go (but do let me know if you know an easier way).

@zepumph zepumph assigned AgustinVallejo and unassigned zepumph Apr 22, 2024
@zepumph
Copy link
Member Author

zepumph commented Apr 22, 2024

Looks like we aren't handling the reset case.

@zepumph zepumph assigned zepumph and unassigned AgustinVallejo Apr 22, 2024
zepumph added a commit that referenced this issue Apr 22, 2024
Signed-off-by: Michael Kauzmann <michael.kauzmann@colorado.edu>
@zepumph zepumph assigned AgustinVallejo and unassigned zepumph Apr 22, 2024
@zepumph
Copy link
Member Author

zepumph commented Apr 22, 2024

Ok. We are now resetting the Property. @AgustinVallejo, ready for review now (I hope).

@AgustinVallejo
Copy link
Contributor

Really happy with the final look of this component, thanks!!

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