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

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

Closed
jbphet opened this issue Dec 3, 2014 · 4 comments
Assignees

Comments

@jbphet
Copy link
Contributor

jbphet commented Dec 3, 2014

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
Copy link
Member

samreid commented Dec 3, 2014

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

@jbphet
Copy link
Contributor Author

jbphet commented Dec 4, 2014

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
Copy link

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

@AshrafSharf AshrafSharf assigned jbphet and unassigned AshrafSharf Dec 19, 2014
@jbphet
Copy link
Contributor Author

jbphet commented Dec 29, 2014

Looks good, closing.

@jbphet jbphet closed this as completed Dec 29, 2014
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

3 participants