phetsims / calculus-grapher

"Calculus Grapher" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
4 stars 4 forks source link

Slider image of curves is more acute than actual curve #332

Closed KatieWoe closed 1 year ago

KatieWoe commented 1 year ago

Test device Samsung and iPad Operating System Win 11 and iPadOS 16 Browser Chrome and Safari Problem description For https://github.com/phetsims/qa/issues/924 The curve that is created on a line is more obtuse than the image shown on the slider. This is easiest to see with the triangle since it is easiest to see it's angle. Discussed with @amanda-phet and we thought it was worth a look. Steps to reproduce

  1. Go to the third or forth screen
  2. Choose the triangle option
  3. Create a curve on the line
  4. Compare the image above the width slider and the curve you made

Visuals anglelast angle2

Troubleshooting information:

!!!!! DO NOT EDIT !!!!! Name: ‪Calculus Grapher‬ URL: https://phet-dev.colorado.edu/html/calculus-grapher/1.0.0-rc.1/phet/calculus-grapher_all_phet.html Version: 1.0.0-rc.1 2023-03-28 17:54:45 UTC Features missing: applicationcache, applicationcache, touch Flags: pixelRatioScaling User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/111.0.0.0 Safari/537.36 Language: en-US Window: 1536x714 Pixel Ratio: 1.25/1 WebGL: WebGL 1.0 (OpenGL ES 2.0 Chromium) GLSL: WebGL GLSL ES 1.0 (OpenGL ES GLSL ES 1.0 Chromium) Vendor: WebKit (WebKit WebGL) Vertex: attribs: 16 varying: 31 uniform: 4096 Texture: size: 8192 imageUnits: 32 (vertex: 32, combined: 64) Max viewport: 8192x8192 OES_texture_float: true Dependencies JSON: {}
pixelzoom commented 1 year ago

It's unclear what is being proposed here. @amanda-phet how would you change things?

@veillette this one is more in your wheelhouse.

Let me know if you want to discuss.

veillette commented 1 year ago

Yes will do

veillette commented 1 year ago

The CurveManipulationDisplayNode has the same x-range has the actual curve, that is from 0-10, but the yRange is different. It goes -0.25 to 1.25 whereas the graph y range goes from -2 to 2. The slider aspect ration is 4/10 just as the graph.

As a result of the different model y-range for the slider, the actual aspect ratio is off by a factor of 8/3 or 2.66.

The reason for the non symmetric y range was of the slider is that most of our functions are not symmetric over the y-range (the only exception being the sinusoid function). As a result we handle the sinuosoid function with a vertical shift (see below)

if ( mode === CurveManipulationMode.HILL ) {
          curve.hill( width, xCenter, yMax );
        }
        else if ( mode === CurveManipulationMode.TRIANGLE ) {
          curve.triangle( width, xCenter, yMax );
        }
        else if ( mode === CurveManipulationMode.PEDESTAL ) {
          curve.pedestal( width, xCenter, yMax );
        }
        else if ( mode === CurveManipulationMode.PARABOLA ) {
          curve.parabola( width, xCenter, yMax );
        }
        else if ( mode === CurveManipulationMode.SINUSOID ) {
          curve.sinusoid( width, xCenter, yMax / 2 );
          curve.shift( xCenter, yMax );
        }
        else if ( mode === CurveManipulationMode.FREEFORM ) {
          CurveManipulationIconNode.freeformIconCurve( curve, yMin, yMax );
        }
        else if ( mode === CurveManipulationMode.TILT ) {
          curve.tilt( xMax, yMax );
        }
        else if ( mode === CurveManipulationMode.SHIFT ) {
          curve.shift( xMax, yMin );
        }
        else {
          throw new Error( `unsupported mode: ${mode}` );
        }

We could potentially use a symmetric y-range and draw all the curves with an offset, except for the sinusoid. If we did so, the triangle slope would match on the slider and the graph.

if ( mode === CurveManipulationMode.HILL ) {
          curve.hill( width, xCenter, 2*yMax );
          curve.shift( 0,- yMax);
        }
        else if ( mode === CurveManipulationMode.TRIANGLE ) {
          curve.triangle( width, xCenter, 2*yMax );
          curve.shift( 0,- yMax );
        }
        else if ( mode === CurveManipulationMode.PEDESTAL ) {
          curve.pedestal( width, xCenter, 2*yMax );
          curve.shift( 0,- yMax );
        }
        else if ( mode === CurveManipulationMode.PARABOLA ) {
          curve.parabola( width, xCenter, 2*yMax );
          curve.shift( 0,- yMax );
        }
        else if ( mode === CurveManipulationMode.SINUSOID ) {
          curve.sinusoid( width, xCenter, yMax );
        }
        else if ( mode === CurveManipulationMode.FREEFORM ) {
          CurveManipulationIconNode.freeformIconCurve( curve, yMin, 2*yMax );
         curve.shift( 0,- yMax );
        }
        else if ( mode === CurveManipulationMode.TILT ) {
          curve.tilt( xMax, 2*yMax );
          curve.shift( 0,- yMax );
        }
        else if ( mode === CurveManipulationMode.SHIFT ) {
          curve.shift( xMax, yMin );
        }
        else {
          throw new Error( `unsupported mode: ${mode}` );
        }
veillette commented 1 year ago

Let me work on a patch which would better for a side-by-side comparison.

veillette commented 1 year ago

By the way the flash simulation shows a similar behavior as our current implementation.

image

image

veillette commented 1 year ago

So there is a tension about respecting the aspect ratio and maximizing the vertical real estate. As you can see in the graph below, no portion of the triangle function is visible for the negative y-axis.

Our CurveManipulationDisplayNode attempts to maximize the usage of vertical space. However, as @KatieWoe pointed out, this result in not preserving the aspect ratio.

The patch below preserved the aspect ratio but since most of the functions are one-sided, that it, their y-values are solely positive, than there is a bit more blank space.

```js Subject: [PATCH] more accurate description of cusps (see #263) --- Index: js/common/view/CurveManipulationDisplayNode.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/common/view/CurveManipulationDisplayNode.ts b/js/common/view/CurveManipulationDisplayNode.ts --- a/js/common/view/CurveManipulationDisplayNode.ts (revision 8e93633415d07ce3bbf4db4e0e48fcb06bcfac9b) +++ b/js/common/view/CurveManipulationDisplayNode.ts (date 1680639037331) @@ -87,14 +87,14 @@ curve.parabola( width, xCenter, yMax ); } else if ( mode === CurveManipulationMode.SINUSOID ) { - curve.sinusoid( width, xCenter, yMax / 2 ); - curve.shift( xCenter, yMax ); + curve.sinusoid( width, xCenter, -yMax ); } else if ( mode === CurveManipulationMode.FREEFORM ) { CurveManipulationIconNode.freeformIconCurve( curve, yMin, yMax ); } else if ( mode === CurveManipulationMode.TILT ) { - curve.tilt( xMax, yMax ); + curve.tilt( xMax, 2 * yMax ); + curve.shift( 0, -yMax ); } else if ( mode === CurveManipulationMode.SHIFT ) { curve.shift( xMax, yMin ); Index: js/common/CalculusGrapherConstants.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/common/CalculusGrapherConstants.ts b/js/common/CalculusGrapherConstants.ts --- a/js/common/CalculusGrapherConstants.ts (revision 8e93633415d07ce3bbf4db4e0e48fcb06bcfac9b) +++ b/js/common/CalculusGrapherConstants.ts (date 1680635576579) @@ -77,8 +77,8 @@ 0.5 * CURVE_X_LENGTH, 0.20 * CURVE_X_LENGTH ), - // model height associated with curveManipulationDisplay (in the same unit as x-Range) - CURVE_MANIPULATION_Y_RANGE: new Range( -0.25, TYPICAL_Y + 0.25 ), + // model height associated with curveManipulationDisplay + CURVE_MANIPULATION_Y_RANGE: new Range( -2, 2 ), // maximum of undo actions (See https://github.com/phetsims/calculus-grapher/issues/64) MAX_UNDO: 20, ````
veillette commented 1 year ago

@pixelzoom and I went back and forth about an appropriate solution. We settled on maintaining the aspect ratio of the curve manipulation display node such that the y -range is the same on the graph.

// model height associated with curveManipulationDisplay
  CURVE_MANIPULATION_Y_RANGE: new Range( -2, 2 ),

The curves themselves have been shifted a bit downward display has been shifted a bit, such that there is a bit of negative space above and below the curve. See the triangle image

and parabola image

On the fourth screen, the graphs themselves are not isometric, whereas our slider display is isometric, so there is a slight mismatch in the aspect ratio (although it is still an improvement over the previous mismatch noted by @KatieWoe) See for the triangle

image

It is a lot of small tweaks that affects all the curve manipulation modes so perhaps it would be easier for @amanda-phet to take a look on master. I would be happy to walk you through the changes.

pixelzoom commented 1 year ago

This is an excellent improvement. Nice suggestion @KatieWoe.

amanda-phet commented 1 year ago

This is looking great to me. Thanks @KatieWoe for suggesting it.

veillette commented 1 year ago

Thanks @amanda-phet for the thumbs up. At this point, the commit is already on master, but we will need to cheery pick it on the RC branch.

veillette commented 1 year ago

The commit was pushed on the 1.0 branch.

veillette commented 1 year ago

To verify in the next RC:

In PhET brand, verify that:

Nancy-Salpepi commented 1 year ago

Following https://github.com/phetsims/calculus-grapher/issues/332#issuecomment-1505949723, this looks good in rc.2. Closing.

KatieWoe commented 1 year ago

Reopening because I also see this with the hill and the width of the pedestal, and even the sin wave to an extent. Should those be looked at too?

Nancy-Salpepi commented 1 year ago

@KatieWoe FYI From slack this morning: Nancy Salpepi 10:09 AM Hey Martin! For https://github.com/phetsims/calculus-grapher/issues/332, the slope should only match the triangle and parabola? For other modes if I only press slightly above the x-axis to make a curve, it is usually wider than what it shown for the slider. Is that OK?

Martin Veillette :house_with_garden: 10:12 AM That's right, for the other modes, the width should match but in proportion of the x-range, so if the width is 20% of the slider with, than its width will be 20% of the x-range on the graph.

amanda-phet commented 1 year ago

Correct, this is just for the triangle and parabola.