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

investigate graph range error #100

Closed
pixelzoom opened this issue Nov 1, 2018 · 6 comments
Closed

investigate graph range error #100

pixelzoom opened this issue Nov 1, 2018 · 6 comments
Assignees

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Nov 1, 2018

Related to #43, discovered while @chrisklus and I were experimenting with changing constants in GQConstants.

Change these constants:

    // range of the graph's axes
    X_AXIS_RANGE: new Range( -10, 10 ),
    Y_AXIS_RANGE: new Range( -10, 10 ),

to ( -12, 12 ) causes this Error:

PointTool.js?bust=1541115502422:91 Uncaught TypeError: Cannot read property 'value' of undefined
    at PointTool.getQuadraticNear (PointTool.js?bust=1541115502422:91)
    at PointTool.onQuadraticProperty.DerivedProperty (PointTool.js?bust=1541115502422:65)
    at new DerivedProperty (DerivedProperty.js?bust=1541115502422:37)
    at new PointTool (PointTool.js?bust=1541115502422:61)
    at new GQModel (GQModel.js?bust=1541115502422:87)
    at new StandardFormModel (StandardFormModel.js?bust=1541115502422:85)
    at new ExploreModel (ExploreModel.js?bust=1541115502422:26)
    at ExploreScreen [as createModel] (ExploreScreen.js?bust=1541115502422:42)
    at ExploreScreen.initializeModel (Screen.js?bust=1541115502422:239)
    at Array.<anonymous> (Sim.js?bust=1541115502422:780)
@pixelzoom pixelzoom self-assigned this Nov 1, 2018
@pixelzoom
Copy link
Contributor Author

pixelzoom commented Nov 2, 2018

( -9, 9 ) and ( -11, 11 ) work just fine. Trouble starts with ( -12, 12 ).

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Nov 2, 2018

The initial locations of the point tools are (-2,12) and (2,-12), so expanding the graph's yRange to (-12,12) means that the point tools are initially on the graph. And that exposes a problem with this bit in PointTool at line 60:

// @public {DerivedProperty.<Quadratic|null>}
this.onQuadraticProperty = new DerivedProperty( 
  [ this.locationProperty, quadraticsProperty ],
  ( location, quadratics ) => {
    if ( graph.contains( location ) ) {
      return this.getQuadraticNear( location,
        GQQueryParameters.pointToolThreshold, GQQueryParameters.pointToolThreshold );
    }
    else {
      return null;
    }
  }

The derivation function calls getQuadraticNear, which then calls this.onQuadraticProperty.value - which is the DerivedProperty. So if the initial state is such that the point tool is on the graph, this.onQuadraticProperty will be undefined. In general, a DerivedProperty's derivation function should not refer to itself.

Options:
(1) Fix the problem so that the point tools can be initially on the graph. This is the better solution, which I'll try first.
(2) Guarantee that the point tools are not on the graph initially by computing their initial y coordinates based on yRange. This is the more expedient solution, which will be the fallback.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Nov 2, 2018

I verified that option (2) fixes the problem by setting the initial locations of the point tools to:
( -2, this.graph.yRange.min - 2)
( 2, this.graph.yRange.min - 2 )

pixelzoom added a commit that referenced this issue Nov 2, 2018
@pixelzoom
Copy link
Contributor Author

Fixed using option (1) in the above commit. The point tools can now be anywhere (on or off the graph) initially. See screenshot below:

screenshot_824

@pixelzoom
Copy link
Contributor Author

Since we want to have the point tools start off the graph initially, I also applied option (1), so that the y coordinate of the point tools are computed based on yRange.

@pixelzoom
Copy link
Contributor Author

Tested with a bunch of different graph ranges and all is well. 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

1 participant