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

Address the confusion about whether the wall does work. #220

Closed
Tracked by #224
pixelzoom opened this issue Apr 1, 2024 · 10 comments
Closed
Tracked by #224

Address the confusion about whether the wall does work. #220

pixelzoom opened this issue Apr 1, 2024 · 10 comments

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Apr 1, 2024

@arouinfar mentioned that she gets lots of questions about the Explore screen, and that perhaps it should be renamed Work.

If we are going to rename any screens, it should be done before first PhET-iO publication.

@pixelzoom pixelzoom changed the title Rename Explore screen to Work. Address the confusion about whether the wall does work. Apr 11, 2024
@pixelzoom
Copy link
Contributor Author

4/11/24 design meeting with @arouinfar @kathy-phet @Nancy-Salpepi @ariel-phet @pixelzoom

  • Keep the name of the screen "Explore"

  • Add a control that turns work on/off for the draggable left wall. @arouinfar will design the control. The consensus was that it's important to have the word "work" in the control label. When work is "off", it will behave like the Ideal screen.

@arouinfar Assign back to me when you have designed the control. Let me know if you want any feedback.

@arouinfar
Copy link
Contributor

The consensus was that it's important to have the word "work" in the control label.

We generally talk about work done "by" something "on" something else. The wording is a bit tricky here because the direction the wall moves matters. When compressed, the wall does work on the gas. When expanded, the gas does work on the wall. Collectively, we call this "Work Done by a System", see https://openstax.org/books/university-physics-volume-2/pages/3-2-work-heat-and-internal-energy
image

When work is "off", it will behave like the Ideal screen.

We should also change the color of the wall handle. On the Ideal Screen the handle is a bit brassy looking, and on the Explore Screen the color matches the lid handle.
image image

@arouinfar Assign back to me when you have designed the control. Let me know if you want any feedback.

I'm leaning towards a RadioButtonGroup called "System Work" with options "on" and "off".

We often use checkboxes to toggle representations or model behaviors on/off, such as Air Resistance in Projectile Motion:
image

However, I think it would be more easily interpreted if we had an explicit on/off control. We do this in Gravity and Orbits, which seems like an analogous case:
image

Here's a mockup. It feels a bit heavy-handed, but I also think the control deserves to have a bit more visual weight/prominence given the amount of feedback we've received about this issue.

image

@pixelzoom your feedback is definitely welcome!

@arouinfar arouinfar assigned pixelzoom and unassigned arouinfar Apr 23, 2024
@pixelzoom
Copy link
Contributor Author

pixelzoom commented Apr 23, 2024

@arouinfar and I discussed #220 (comment), and

We're feeling like adding this on/off control may be the wrong direction, because:

  • It's unclear (to me anyway) if the gas is really doing work on the wall in this sim. That may be the qualitative impression, but it's not how the model works (see model.md). So a control labeled "System Work" may be even more confusing.
  • The control is kind of heavy-handed and klunky. And it's probably not going to resolve the confusion, because the control is nowhere near the container's resize handle.
  • Making the container toggle between the 2 behaviors (work vs no work) is going to be a big code change. It's not something that was designed to be dynamic. It will require changing some fundamental classes (IdealGasLawScreenView, IdealGasLawContainerNode, maybe more) that I'd prefer not to touch.

Brainstorming other ideas...

  • We like the idea of showing something in the vicinity of the resize handle that appears while you're resizing. Something that wasn't present in the Ideal screen, to indicate that something different is happening in the Explore screen.
  • We'll try a green arrow, inside the container, vertically centered on the resize handle. The arrow will be present while dragging, tail sized to speed of wall, pointing in the same direction that the resize handle is moving.
  • We might need a vertical row of arrows, but start with one arrow as “proof of concept” and to get team feedback.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented May 16, 2024

For my first attempt (in the patch below), I converted BaseContainer leftWallVelocity: Vector2 to leftWallVelocityProperty: Property<Vector2>. Then update the length of the arrow whenever leftWallVelocityProperty changes, and hide the arrow when magnitude is zero. See screenshot below.

With this approach, the arrow was very jerky. I'm going to investigate doing something similar to ParticleFlowRateModel, which uses a running average.

patch
Subject: [PATCH] remove duplicate options
---
Index: js/common/model/CollisionDetector.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/model/CollisionDetector.ts b/js/common/model/CollisionDetector.ts
--- a/js/common/model/CollisionDetector.ts	(revision 055b28d3c7603e979a0e1b12904adaa1c4fcf3df)
+++ b/js/common/model/CollisionDetector.ts	(date 1715882950295)
@@ -150,7 +150,7 @@
     let numberOfParticleContainerCollisions = 0;
     for ( let i = this.particleArrays.length - 1; i >= 0; i-- ) {
       numberOfParticleContainerCollisions += CollisionDetector.doParticleContainerCollisions( this.particleArrays[ i ],
-        this.container.bounds, this.container.leftWallVelocity );
+        this.container.bounds, this.container.leftWallVelocityProperty.value );
     }
     return numberOfParticleContainerCollisions;
   }
Index: js/common/view/IdealGasLawContainerNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/view/IdealGasLawContainerNode.ts b/js/common/view/IdealGasLawContainerNode.ts
--- a/js/common/view/IdealGasLawContainerNode.ts	(revision 055b28d3c7603e979a0e1b12904adaa1c4fcf3df)
+++ b/js/common/view/IdealGasLawContainerNode.ts	(date 1715883494793)
@@ -31,6 +31,7 @@
 import DerivedProperty from '../../../../axon/js/DerivedProperty.js';
 import BooleanProperty from '../../../../axon/js/BooleanProperty.js';
 import ResizeHandleDragDelegate from './ResizeHandleDragDelegate.js';
+import ArrowNode from '../../../../scenery-phet/js/ArrowNode.js';
 
 const LID_X_SPEED = -50; // pixels/second
 const LID_Y_SPEED = -150; // pixels/second
@@ -133,6 +134,29 @@
 
     options.children = [ previousBoundsNode, resizeHandleNode, wallsNode, lidNode ];
 
+    if ( container.leftWallDoesWork ) {
+
+      const workIndicatorNode = new ArrowNode( 0, 0, 100, 0, {
+        headWidth: 30,
+        headHeight: 30,
+        tailWidth: 10,
+        fill: 'green' //TODO color profile
+      } );
+      options.children.push( workIndicatorNode );
+
+      container.leftWallVelocityProperty.link( velocity => {
+        const speed = velocity.magnitude;
+        workIndicatorNode.visible = ( speed !== 0 );
+        const length = Utils.linear( 0, IdealGasLawContainer.WALL_SPEED_LIMIT, 35, 120, speed );
+        workIndicatorNode.setTip( length * Math.sign( velocity.x ), 0 );
+      } );
+
+      Multilink.multilink( [ container.widthProperty, workIndicatorNode.localBoundsProperty ], () => {
+        workIndicatorNode.left = modelViewTransform.modelToViewX( container.left );
+        workIndicatorNode.centerY = modelViewTransform.modelToViewY( container.centerY );
+      } );
+    }
+
     super( options );
 
     this.addLinkedElement( container );
Index: js/common/model/IdealGasLawContainer.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/model/IdealGasLawContainer.ts b/js/common/model/IdealGasLawContainer.ts
--- a/js/common/model/IdealGasLawContainer.ts	(revision 055b28d3c7603e979a0e1b12904adaa1c4fcf3df)
+++ b/js/common/model/IdealGasLawContainer.ts	(date 1715883447667)
@@ -22,9 +22,6 @@
 import WithRequired from '../../../../phet-core/js/types/WithRequired.js';
 import Tandem from '../../../../tandem/js/Tandem.js';
 
-// Speed limit for the container's left movable wall, in pm/ps. Relevant when reducing the container size.
-const WALL_SPEED_LIMIT = GasPropertiesQueryParameters.wallSpeedLimit;
-
 type SelfOptions = {
   leftWallDoesWork?: boolean;  // true if the left wall does work on particles, as in the Explore screen
 };
@@ -59,6 +56,9 @@
   // Is the lid open?
   public readonly lidIsOpenProperty: TReadOnlyProperty<boolean>;
 
+  // Speed limit for the container's left movable wall, in pm/ps. Relevant when reducing the container size.
+  public static readonly WALL_SPEED_LIMIT = GasPropertiesQueryParameters.wallSpeedLimit;
+
   public constructor( providedOptions: IdealGasLawContainerOptions ) {
 
     const options = optionize<IdealGasLawContainerOptions, SelfOptions, BaseContainerOptions>()( {
@@ -154,7 +154,7 @@
       // See https://github.com/phetsims/gas-properties/issues/90.
       if ( this.leftWallDoesWork ) {
 
-        const widthStep = dt * WALL_SPEED_LIMIT;
+        const widthStep = dt * IdealGasLawContainer.WALL_SPEED_LIMIT;
 
         if ( widthStep < Math.abs( widthDifference ) ) {
           if ( widthDifference > 0 ) {
@@ -173,11 +173,11 @@
     // velocity is irrelevant and should remain set to zero, so that it doesn't contribute to collision detection.
     if ( this.leftWallDoesWork ) {
       const velocityX = ( this.left - this.previousLeftProperty.value ) / dt;
-      this.leftWallVelocity.setXY( velocityX, 0 );
+      this.leftWallVelocityProperty.value = new Vector2( velocityX, 0 );
       this.previousLeftProperty.value = this.left;
     }
     else {
-      assert && assert( this.leftWallVelocity.magnitude === 0, 'wall velocity should be zero' );
+      assert && assert( this.leftWallVelocityProperty.value.magnitude === 0, 'wall velocity should be zero' );
     }
   }
 
Index: js/common/model/BaseContainer.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/model/BaseContainer.ts b/js/common/model/BaseContainer.ts
--- a/js/common/model/BaseContainer.ts	(revision 055b28d3c7603e979a0e1b12904adaa1c4fcf3df)
+++ b/js/common/model/BaseContainer.ts	(date 1715883349673)
@@ -24,6 +24,7 @@
 import Particle from './Particle.js';
 import Tandem from '../../../../tandem/js/Tandem.js';
 import GasPropertiesConstants from '../GasPropertiesConstants.js';
+import Vector2Property from '../../../../dot/js/Vector2Property.js';
 
 type SelfOptions = {
   position?: Vector2; // position of the container's bottom right corner, in pm
@@ -46,10 +47,8 @@
   // maximum inside bounds, in pm. Used for canvasBounds for the particle system inside the container.
   public readonly maxBounds: Bounds2;
 
-  // Velocity of the left (movable) wall, pm/ps. This vector will be MUTATED! It does not need to be PhET-iO stateful
-  // because it is recomputed on each call to step (for containers where the left wall does work), then used by
-  // CollisionDetector to do container-particle collisions.
-  public readonly leftWallVelocity: Vector2;
+  // Velocity of the left (movable) wall, pm/ps.
+  public readonly leftWallVelocityProperty: Property<Vector2>;
 
   // Indicates whether the user is adjusting widthProperty. The width will also change automatically in
   // HoldConstant 'pressureV' mode. This is used to suppress model updates in the Ideal screen, when the user
@@ -112,7 +111,9 @@
       this.position.x, this.position.y + this.height
     );
 
-    this.leftWallVelocity = new Vector2( 0, 0 );
+    this.leftWallVelocityProperty = new Vector2Property( new Vector2( 0, 0 ), {
+      valueComparisonStrategy: 'equalsFunction'
+    } );
 
     this.userIsAdjustingWidthProperty = new BooleanProperty( false, {
       tandem: this.isFixedWidth ? Tandem.OPT_OUT : options.tandem.createTandem( 'userIsAdjustingWidthProperty' ),
@@ -154,6 +155,8 @@
 
   public get top(): number { return this.bounds.maxY; }
 
+  public get centerY(): number { return this.bounds.centerY; }
+
   /**
    * Determines whether the container fully contains a particle.
    */
screenshot_3309

@pixelzoom
Copy link
Contributor Author

In the above commits, I replaced BaseContainer leftWallVelocity: Vector2 with leftWallVelocityX: number. Since the left wall only moves horizontally, we only need the x-component. And if we're going to keep samples for a running average, it's preferrable not to allocate Vector2 instances.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented May 16, 2024

@arouinfar please test-drive in main, then let's discuss next steps.

Notes...

This version keeps a running average of the left wall's velocity. The average is over 50 samples (configurable). If there are 2 consecutive samples where vx === 0, then the average is immediately set to zero. This causes the vector arrow to immediate disappear, instead of gradually (and noticeably) shrinking and then disappearing.

The function for computing the arrow length is:

Utils.linear( 0, 800, 35, 100, Math.abs( velocityX ) )

So for speed range (0,800] pm/ps, the arrow length varies from [35,100] in view coordinates. When speed is 0, the arrow is hidden.

The arrow configuration is:

        headWidth: 30,
        headHeight: 20,
        tailWidth: 10,
        fill: GasPropertiesColors.workVectorColorProperty

@pixelzoom
Copy link
Contributor Author

pixelzoom commented May 20, 2024

Design meeting 5/20/24: @arouinfar @kathy-phet @Nancy-Salpepi @matthew-blackman @pixelzoom

  • Add a "Wall Velocity" checkbox, first in `toolsPanel, on by default.
  • Center the tail of the velocity vector on the left wall, above the resize handle.
  • Use PhetColorScheme.VELOCITY for the vector color.
  • @arouinfar and I will work on polishing the look of the vector, the number of samples, and the number of consecutive non-zero samples that cause the vector to immediately go to zero magnitude.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented May 20, 2024

@arouinfar Let's meet to finish off this item:

@arouinfar and I will work on polishing the look of the vector, the number of samples, and the number of consecutive non-zero samples that cause the vector to immediately go to zero magnitude.

See WallVelocityVectorNode and IdealGasLawContainer.leftWallAverageVelocityXProperty.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented May 21, 2024

In b434c17, the velocity vector is hidden if the wall has not moved for 0.5 ps. This value can be changed in IdealGasLawContainer.ts:

      // Tuned so that the vector behaves smoothly while dragging the resize handle, but disappears quickly after dragging ends.
      if ( this.dtSinceWallMoved >= 0.5 ) {

        // The wall has not moved, so immediately set the running average to zero.
        this.dxSamples.length = 0;
        this.dtSamples.length = 0;
        this._leftWallAverageVelocityXProperty.value = 0;
      }
      else {

@pixelzoom
Copy link
Contributor Author

@arouinfar @Nancy-Salpepi and I tweaked this together, see 1b0ace1.

Closing.

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

2 participants