Problem with Point Tool background color.

[amanda-phet]

I can tell the point tool is "on" the curve because it feels attached. When I try to drag it away, it doesn't move until I drag quite a bit. So even though the point tool is on the curve, the background is white instead of the color of the curve. I get to this point by dragging the tool along the curve, but right at the vertex the background color changed.

I can reproduce this using other curves, and I'm not sure if there is a pattern yet to when this happens and when it doesn't.

I consistently get this behavior with

y = -6x^2 + 6x y = 6x^2 - 6x + 3 y = 6x^2 + 6x y = 6x^2 + 3x

but not with y=2x^2 - 6x + 3

It's tricky to get curves with values for a that aren't decimals, so a=6 is the easiest for me to test.

[amanda-phet]

This seems to be somewhat present in 1.2.0-rc.1, and 1.1.0 even, because the point tool will flicker right around that point. However it seems to still stop right on the vertex with the filled background, whereas in 1.2.0-rc.2 the tool only has the white background when at this vertex point. It seems unique to this rc perhaps because of the additional decimal point?

[pixelzoom]

@amanda-phet I'm assuming that you want this addressed for the 1.2 release, so I'm labeling as "blocks-sim-publication".

[Nancy-Salpepi]

Some observations:

1. In general, the point tool seems less latched on when you get towards the vertex--it pops off from one side of the curve to the other pretty easily. I have to slow down in order to avoid that from happening.
2. The point tool not having a background seems to occur when the curve is very narrow. I couldn't reproduce when a=1 or a=2. I also didn't observe it at y = -5x^2 + 6x + 3.
3. In older versions, I can get the point tool to have no background, but when I wiggle my finger without changing the coordinates at all, the background becomes colored. I was unable to do that in the newer version.

[KatieWoe]

This is on published. It takes a very slight movement to cause it.

[pixelzoom]

@amanda-phet @Nancy-Salpepi @KatieWoe please provide information on what type of device (mouse or touch) you're using for the tests you've reported above. It would also be helpful to know if you can reproduce the same behavior on mouse and touch devices.

[pixelzoom]

I took a quick look at this. Notes for myself:

Relevant code in PointTool (model), here's where the quadratic that the point tool is on is derived. Note the call to getQuadraticNear.

    this.quadraticProperty = new DerivedProperty(
      [ this.positionProperty, quadraticsProperty ],
      ( position, quadratics ) => {
        if ( graph.contains( position ) ) {
          return this.getQuadraticNear( position,
            GQQueryParameters.pointToolThreshold, GQQueryParameters.pointToolThreshold );
        }
        else {
          return null;
        }
      }

In PointToolNode (view), there's a listener that observes the model and changes the tool's background color:

        // update colors
        if ( graph.contains( position ) && onQuadratic && graphContentsVisible ) {

          // color code the display to onQuadratic
          coordinatesNode.foreground = options.foregroundHighlightColor;
          backgroundNode.fill = onQuadratic.color;
        }
        else {
          coordinatesNode.foreground = options.foregroundNormalColor;
          backgroundNode.fill = options.backgroundNormalColor;
        }

In PointToolDragListener (view) we snap to the curve. Note the call to getQuadraticNear.

          // If we're close enough to a quadratic, snap to that quadratic.
          const snapQuadratic = pointTool.getQuadraticNear( position,
            GQQueryParameters.snapOffDistance, GQQueryParameters.snapOnDistance );
          if ( snapQuadratic ) {

            // Get the closest point that is on the quadratic.
            position = snapQuadratic.getClosestPoint( position );

            // We will be snapping the x value as it will be displayed by the point tool.
            // See See https://github.com/phetsims/graphing-quadratics/issues/169.
            let x = Utils.toFixedNumber( position.x, GQConstants.POINT_TOOL_DECIMALS );

            // If x is close to an integer value, snap to that integer value.
            // See https://github.com/phetsims/graphing-quadratics/issues/169.
            const closestInteger = Utils.toFixedNumber( x, 0 );
            if ( Math.abs( x - closestInteger ) < xSnapTolerance ) {
              x = closestInteger;
            }

            const y = snapQuadratic.solveY( x );
            position = new Vector2( x, y );
          }

The model and view are both calling getQuadraticNear. The problem (I believe) is that after the view calls getQuadraticNear, it modifies the x and y coordinates, in order to "snap". So by the time we set pointTool.positionProperty, the modified point may no longer be on the quadratic, as determined by the model when it calls getQuadraticNear. So then snapping thinks the tool IS on the quadratic, but the code that sets background color thinks the tool IS NOT on the quadratic.

I should probably investigate moving "snap" responsibility to the model.

[Nancy-Salpepi]

I was using a trackpad initially on my MacBook air when I was able to change the background color of the vertex. I can reproduce the same results with a mouse. I was able to do this on the published sim as well as 1.1.0-rc.1 and 1.2.0-rc.1. I can also reproduce on the iPad (I looked at published). In all cases, I am able to get the background to be colored/white just by moving my finger/mouse slightly without changing the coordinates.

[KatieWoe]

I was on Win 10 Chrome with a trackpad

[pixelzoom]

@amanda-phet and I discussed this issue via Zoom. Since this a problem in the published version, and the priority for 1.2 is PhET-iO support (I think?), then I'm tempted to defer this until a future release. But neither of us are clear on priorities for this release, we've gone far down the "improve the point tool" path, and this problem seems to be more pronounced because of that. So both of us would like to see this fixed for 1.2. I'll continue to work on this.

[pixelzoom]

I have a new theory about the cause of this problem. There appears to be a problem with Quadratic getClosestPoint, the method that gets the point on a curve that is closest to a specified point. If I print the results of this method to the console, here's what I see near the vertex, whenever the background color of the point tool is incorrect:

closestPoint=Vector2(NaN, NaN)

getClosestPoint starts by getting the roots, by calling Utils.solveCubicRootsReal. And that method is returning values that are not finite, verified by adding this assertion (which fails):

assert && assert( _.every( roots, root => isFinite( root ) ), 
  `roots must be finite: ${roots.toString()}` );

[pixelzoom]

Adding this to the end of Quadratic getClosestPoint:

    if ( !closestPoint.isFinite() ) {
      console.log( '---------' );
      console.log( `closestPoint=${closestPoint}` );
      console.log( `roots=${roots.toString()}` );
      console.log( `x0=${x0} y0=${y0} a=${this.a} b=${this.b} c=${this.c}` );
      console.log( `Utils.solveCubicRootsReal args: ${2 * a * a}, ${3 * a * b}, ${b * b + 2 * a * c - 2 * a * y0 + 1}, ${b * c - b * y0 - x0}` );
    }

For y = -6x² + 6x + 0 in the Explore screen, I see console output like this when dragging the point tool around the vertex (0.5, 1.5) of the curve:

closestPoint=Vector2(NaN, NaN)
roots=NaN,NaN,NaN
x0=0.5 y0=1.5 a=-6 b=6 c=0
Utils.solveCubicRootsReal args: 72, -108, 55, -9.5

Confirming by calling solveCubicRootsReal directly in the console:

> phet.dot.Utils.solveCubicRootsReal( 72, -108, 55, -9.5 )
(3) [NaN, NaN, NaN]

The documentation for solveCubicRootsReal says:

   * @returns {Array.<number>|null} - The real roots of the equation, or null if all values are roots. If the root has
   *                                  a multiplicity larger than 1, it will be repeated that many times.
   *

While NaN is indeed a {number}, I doubt that it's considered a "real root". So I suspect that Utils.solveCubicRootsReal is buggy.

[pixelzoom]

I added a break point to Utils.solveCubicRootsReal, then typed my failing example into the console:

phet.dot.Utils.solveCubicRootsReal( 72, -108, 55, -9.5 )

Stepping through the code, I hit the problem in these lines:

472 // all unique
473 let qX = -q * q * q;
474 qX = Math.acos( r / Math.sqrt( qX ) );
475 const rr = 2 * Math.sqrt( -q );
476 return [
        -b3 + rr * Math.cos( qX / 3 ),
        -b3 + rr * Math.cos( ( qX + 2 * Math.PI ) / 3 ),
        -b3 + rr * Math.cos( ( qX + 4 * Math.PI ) / 3 )
      ];

Inspecting values in the debugger:

At Line 473: q = 0.004629629629629613 → qX = -9.922903012752013e-8, At Line 474:r = 0, Math.sqrt( qX ) = NaN → qX = NaN At Line 475: q = 0.004629629629629613, Math.sqrt( -q ) = NaN → rr = NaN At Line 476: b3 = -0.5, rr = NaN, qX = NaN → [ NaN, NaN, NaN ]

[pixelzoom]

This is as far as I can take this. I'll need @jonathanolson's assistance.

[jonathanolson]

It looks like this is definitely a single-real-root scenario (roots are 0.5 - 0.117851i, 0.5 + 0.117851i, 0.5). The discriminant was barely above the 1e-7 threshold. It looks like the best option is to increase the threshold, since we've seen a case where numerical imprecision will trigger the wrong case.

[jonathanolson]

Committed a hopeful fix above, can you verify?

[pixelzoom]

Testing with y = -6x² + 6x + 0 in the Explore screen, dragging the point tool along the curve ...

It's even worse now. It returning [NaN, NaN, NaN] more often near the vertex, and failing for points that are not at the vertex. For example:

closestPoint=Vector2(NaN, NaN)
Quadratic.js:358 roots=NaN,NaN,NaN
Quadratic.js:359 x0=0.5733964870309585 y0=1.4660832405076079 a=-6 b=6 c=0
Quadratic.js:360 Utils.solveCubicRootsReal args: 72, -108, 54.592998886091294, -9.369895930076606

[pixelzoom]

@jonathanolson fixed his fix in the above commit, and it's better. But it's still failing, for example:

> phet.dot.Utils.solveCubicRootsReal( 72, -108, 54.309261152632615, -9.157483833649408 )
[ NaN, NaN, NaN ]

[pixelzoom]

If I decrease the discriminate threshold further in Utils.solveCubicRootsReal, from 1e-8 to 1e-9, the problem appears to go away. Changing this threshold (as has already been done in https://github.com/phetsims/dot/commit/272fb9f54a8365317739f3d11e72c1f7f374b359) is a concern because it could break other sims. How about if we revert to the original threshold of 1e-7, and add an optional parameter that sims can use to specify a custom threshold? The function signature would be:

   * @param {number} a
   * @param {number} b
   * @param {number} c
   * @param {number} d
   * @param {number} [discriminateThreshold]
...
solveCubicRootsReal( a, b, c, d, discriminateThreshold = 1e-7 )

[pixelzoom]

Proposed patch:

Index: js/Utils.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/Utils.js b/js/Utils.js
--- a/js/Utils.js   (revision 272fb9f54a8365317739f3d11e72c1f7f374b359)
+++ b/js/Utils.js   (date 1630360627590)
@@ -425,10 +425,11 @@
    * @param {number} b
    * @param {number} c
    * @param {number} d
+   * @param {number} [discriminantThreshold] - for determining whether we have a single real root
    * @returns {Array.<number>|null} - The real roots of the equation, or null if all values are roots. If the root has
    *                                  a multiplicity larger than 1, it will be repeated that many times.
    */
-  solveCubicRootsReal( a, b, c, d ) {
+  solveCubicRootsReal( a, b, c, d, discriminantThreshold = 1e-7 ) {
     // TODO: a Complex type!

     // Check for a degenerate case where we don't have a cubic
@@ -455,7 +456,7 @@
     const discriminant = q * q * q + r * r;
     const b3 = b / 3;

-    if ( discriminant > 1e-8 ) {
+    if ( discriminant > discriminantThreshold ) {
       // a single real root
       const dsqrt = Math.sqrt( discriminant );
       return [ Utils.cubeRoot( r + dsqrt ) + Utils.cubeRoot( r - dsqrt ) - b3 ];

[jonathanolson]

Handled above, and integrated assertions to every single type of way roots could be returned. Aqua fuzz passed. @pixelzoom can you verify?

[pixelzoom]

Thanks @jonathanolson, verified in master.

I change graphing-quadratics Quadratic.js to use discriminantThreshold = 1e-9.

Patched the 1.2 release like this:

% grunt checkout-target --repo=graphing-quadratics --target=1.2
% cd graphing-quadratics
% git cherry-pick 5daae58dfc24d71ecca15a39096f90e14f0da8cd
% cd dot
% git checkout -b graphing-quadratics-1.2
% git cherry-pick 6ac2f0579fb330e15f959d1ecece4a937f9e4941
% git cherry-pick 272fb9f54a8365317739f3d11e72c1f7f374b359
% git cherry-pick f291f519cb6c5919e1152dd83b9d4c7e1674eaee
% cd graphing-quadratics
% grunt --brands=phet
// test
% cp build/phet/dependencies.json .
% git add dependencies.json 
% git commit -m 'patch for https://github.com/phetsims/graphing-quadratics/issues/170'
% git push origin 1.2
% cd dot
% git push origin graphing-quadratics-1.2

[pixelzoom]

@amanda-phet please review in master before I create the next RC. Take a close look at the point tool behavior of course. But also please look at the Point On Parobola manipulator - it uses the same code as point tool to keep the point on the curve.

When you're done, assign back to me.

[amanda-phet]

The point tool seems to be working great. Is there something specific to test for the point on parabola manipulator? The point is staying attached to the curve just fine with various curves, and the coordinates are accurate. Let me know if there's anything else to verify for that, but I think it all looks good!

Thanks for hunting down the source of the problem.

[pixelzoom]

Thanks @amanda-phet. Nothing specific to test for Point On Parabola, just that it remains attached to the curve.

[pixelzoom]

This issue is fixed and ready for testing in the next RC.

[KatieWoe]

This looks ok in rc.3. Will reopen if something comes up.