phetsims / neuron

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

Optimization: Use a path and update the shape instead of adding lots of little lines #32

Closed jbphet closed 9 years ago

jbphet commented 9 years ago

MembranePotentialChart.js currently contains the following code:

    thisChart.dataSeries.addDataSeriesListener( function( x, y, xPrevious, yPrevious ) {
      if ( xPrevious && yPrevious && (xPrevious !== 0 || yPrevious !== 0 ) ) {
        var line = new Line( thisChart.chartMvt.modelToViewX( xPrevious ), thisChart.chartMvt.modelToViewY( yPrevious ), thisChart.chartMvt.modelToViewX( x ), thisChart.chartMvt.modelToViewY( y ), {
            stroke: thisChart.dataSeries.color}
        );
        line.computeShapeBounds = computeShapeBounds;
        chartContentNode.addChild( line );
      }

      thisChart.dataSeries.on( 'cleared', function() {
        chartContentNode.removeAllChildren();
      } );

    } );

This can create potentially thousands of little line nodes. It would be better to have a single Path node and update a Kite Shape on each change to the data series. Note that in order to have this work for the path, you'll need to set the shape to null. In pseudo code, you'll need to do something like this:

path.shape = null; update the shape by adding the new data point path.shape = shape

This was found during code review, see #29.

samreid commented 9 years ago

Why do you need to set path.shape=null before updating the shape?

jbphet commented 9 years ago

From the client standpoint, the answer is that if you don't do this, it doesn't work.

If you're asking about why Scenery requires this, my guess is that it is necessary because otherwise the Path assumes that the shape is immutable once supplied (@jonathanolson mentioned something about this), so no update will occur if the shape is set to the same value.

AshrafSharf commented 9 years ago

Graph Lines are created using moveTo, LineTo Instead adding multiple line nodes.

jbphet commented 9 years ago

Looks good, closing.