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

KatieWoe · 2023-04-04T17:57:39Z

Test device
Samsung and iPad
Operating System
Win 11 and iPadOS 16
Browser
Chrome and Safari
Problem description
For phetsims/qa#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

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

Visuals

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 · 2023-04-04T18:27:10Z

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 · 2023-04-04T18:36:27Z

Yes will do

veillette · 2023-04-04T19:02:33Z

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 · 2023-04-04T19:06:38Z

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

veillette · 2023-04-04T19:25:53Z

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

veillette · 2023-04-04T20:33:08Z

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.

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,

Signed-off-by: Martin Veillette <[email protected]>

veillette · 2023-04-04T22:07:30Z

@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

and parabola

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

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 · 2023-04-04T22:38:03Z

This is an excellent improvement. Nice suggestion @KatieWoe.

amanda-phet · 2023-04-07T15:15:07Z

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

veillette · 2023-04-07T16:37:01Z

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.

Signed-off-by: Martin Veillette <[email protected]> (cherry picked from commit bfc57fb)

veillette · 2023-04-10T14:03:07Z

The commit was pushed on the 1.0 branch.

veillette · 2023-04-12T21:08:45Z

To verify in the next RC:

In PhET brand, verify that:

Go to the third screen
select the triangle mode
verify that the slope of the triangle on the slider display visually matches the slope of the triangle on the graph
similarly when selecting parabola mode, the curvature of the parabola on the slider display visually matches the curvature of the parabola on the graph
check that slider display behave appropriately for the other curve modes

Nancy-Salpepi · 2023-04-14T14:33:34Z

Following #332 (comment), this looks good in rc.2.
Closing.

KatieWoe · 2023-04-14T19:34:30Z

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 · 2023-04-14T19:54:59Z

@KatieWoe FYI
From slack this morning:
Nancy Salpepi
10:09 AM
Hey Martin! For #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
🏡 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 · 2023-04-14T20:12:17Z

Correct, this is just for the triangle and parabola.

KatieWoe added the type:suggestion label Apr 4, 2023

KatieWoe assigned veillette and amanda-phet Apr 4, 2023

pixelzoom assigned veillette and unassigned veillette Apr 4, 2023

veillette added a commit that referenced this issue Apr 4, 2023

maintain aspect ratio of CurveManipulationDisplayNode (see #332)

bfc57fb

Signed-off-by: Martin Veillette <[email protected]>

veillette removed their assignment Apr 4, 2023

veillette mentioned this issue Apr 6, 2023

screenshots #321

Closed

veillette added the priority:2-high label Apr 6, 2023

veillette assigned veillette and unassigned amanda-phet Apr 7, 2023

veillette removed the priority:2-high label Apr 10, 2023

veillette added a commit that referenced this issue Apr 10, 2023

maintain aspect ratio of CurveManipulationDisplayNode (see #332)

ce027b9

Signed-off-by: Martin Veillette <[email protected]> (cherry picked from commit bfc57fb)

veillette added the status:fixed-awaiting-deploy label Apr 10, 2023

veillette removed their assignment Apr 10, 2023

veillette mentioned this issue Apr 13, 2023

RC Spot-Check Test: Calculus Grapher 1.0.0-rc.2 phetsims/qa#933

Closed

14 tasks

Nancy-Salpepi closed this as completed Apr 14, 2023

KatieWoe reopened this Apr 14, 2023

KatieWoe assigned veillette and amanda-phet Apr 14, 2023

amanda-phet removed their assignment Apr 14, 2023

KatieWoe closed this as completed Apr 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

KatieWoe commented Apr 4, 2023

pixelzoom commented Apr 4, 2023 •

edited

Loading

veillette commented Apr 4, 2023

veillette commented Apr 4, 2023

veillette commented Apr 4, 2023

veillette commented Apr 4, 2023

veillette commented Apr 4, 2023 •

edited

Loading

veillette commented Apr 4, 2023

pixelzoom commented Apr 4, 2023 •

edited

Loading

amanda-phet commented Apr 7, 2023

veillette commented Apr 7, 2023

veillette commented Apr 10, 2023

veillette commented Apr 12, 2023 •

edited

Loading

Nancy-Salpepi commented Apr 14, 2023

KatieWoe commented Apr 14, 2023

Nancy-Salpepi commented Apr 14, 2023

amanda-phet commented Apr 14, 2023

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

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

Comments

KatieWoe commented Apr 4, 2023

pixelzoom commented Apr 4, 2023 • edited Loading

veillette commented Apr 4, 2023

veillette commented Apr 4, 2023

veillette commented Apr 4, 2023

veillette commented Apr 4, 2023

veillette commented Apr 4, 2023 • edited Loading

veillette commented Apr 4, 2023

pixelzoom commented Apr 4, 2023 • edited Loading

amanda-phet commented Apr 7, 2023

veillette commented Apr 7, 2023

veillette commented Apr 10, 2023

veillette commented Apr 12, 2023 • edited Loading

Nancy-Salpepi commented Apr 14, 2023

KatieWoe commented Apr 14, 2023

Nancy-Salpepi commented Apr 14, 2023

amanda-phet commented Apr 14, 2023

pixelzoom commented Apr 4, 2023 •

edited

Loading

veillette commented Apr 4, 2023 •

edited

Loading

pixelzoom commented Apr 4, 2023 •

edited

Loading

veillette commented Apr 12, 2023 •

edited

Loading