phetsims / curve-fitting

"Curve Fitting" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
6 stars 3 forks source link

In CurveShape, can the approach to generate a curve shape be simplified? #122

Closed veillette closed 5 years ago

veillette commented 5 years ago

in #103, the approach to generate the shape of the best fit curve was modified in order to get better performance on the iPad2. Although the proposed solution improved the performance on the iPad2, it is a bit forbidding mathematically, hard to debug (see #113) and may be no longer be warranted if the iPad2 is no longer supported.

SaurabhTotey commented 5 years ago

In my latest commit, I removed the majority of the CurveShape code and switched to using linear interpolation as was suggested. The entire constructor now looks like as follows.

      super();

      // model bounds of the graph area
      const graphBounds = CurveFittingConstants.GRAPH_MODEL_BOUNDS;

      // convenience variables
      const xMin = graphBounds.minX; // minimum value of the x range
      const xMax = graphBounds.maxX; // maximum value of the x range

      // how many segments: more segments means a finer/more accurate curve
      const steps = 500;

      // separation between adjacent x coordinates
      const interval = ( xMax - xMin ) / steps;

      // move shape to initial location
      this.moveTo( xMin, getYValueAt( xMin ) );

      // create lines connecting each point
      for ( let x = xMin + interval; x <= xMax; x += interval ) {
        const y = getYValueAt( x );
        this.lineTo( x, y );
      }

With this new code, I have noticed some slight performance dips, but nothing huge or apparent. The original linear interpoloation code used 1000 segments, which caused a very noticeable performance drop, so in this version, I only used 500 segments.

veillette commented 5 years ago

@SaurabhTotey, that looks good. We can close this issue. If there are any performance issues that are raised during RC testing, you should know that this change is the likely culprit. WIth the appropriate SHA https://github.com/phetsims/curve-fitting/commit/0d0018423f0a855f552fcf2a1b7f587ecaa94db4, you can revert to the more performing (but more complicated code-wise) approach.