From 59f3874b87655c8b6f0e93e8e921f20508868f50 Mon Sep 17 00:00:00 2001
From: Jesse <jesse.greenberg@colorado.edu>
Date: Tue, 25 Feb 2020 16:39:47 -0500
Subject: [PATCH] remove SkaterSamples in batch to redraw canvas less
 frequently, see #198

---
 .../model/EnergySkateParkSaveSampleModel.js   | 25 ++++++++++++++++---
 .../common/model/SkaterSample.js              | 10 ++++----
 .../graphs/view/EnergyPlot.js                 | 13 +++++++++-
 3 files changed, 39 insertions(+), 9 deletions(-)

diff --git a/js/energy-skate-park/common/model/EnergySkateParkSaveSampleModel.js b/js/energy-skate-park/common/model/EnergySkateParkSaveSampleModel.js
index 60e5e880..ab5bab6f 100644
--- a/js/energy-skate-park/common/model/EnergySkateParkSaveSampleModel.js
+++ b/js/energy-skate-park/common/model/EnergySkateParkSaveSampleModel.js
@@ -20,6 +20,7 @@ define( require => {
   const merge = require( 'PHET_CORE/merge' );
   const NumberProperty = require( 'AXON/NumberProperty' );
   const ObservableArray = require( 'AXON/ObservableArray' );
+  const Property = require( 'AXON/Property' );
   const SkaterSample = require( 'ENERGY_SKATE_PARK/energy-skate-park/common/model/SkaterSample' );
 
   class EnergySkateParkSaveSampleModel extends EnergySkateParkTrackSetModel {
@@ -65,6 +66,11 @@ define( require => {
 
       // @protected {ObservableArray.<SkaterSample>} - observable list of all saved SkaterSamples
       this.skaterSamples = new ObservableArray();
+
+      // @public (read-only) - an array of SkaterSamples that have just been removed from the model. Necessary
+      // for performance so that we can redraw once after removing many samples rather than redrawing every time
+      // a single sample is removed
+      this.batchRemoveSamplesProperty = new Property( [] );
     }
 
     /**
@@ -73,7 +79,9 @@ define( require => {
      * @returns {}
      */
     clearEnergyData() {
+      const samplesToRemove = this.skaterSamples.getArray().slice();
       this.skaterSamples.clear();
+      this.batchRemoveSamplesProperty.set( samplesToRemove );
       this.sampleTimeProperty.reset();
     }
 
@@ -120,7 +128,7 @@ define( require => {
         }
       }
 
-      // remove oldest samples if we have collected too many
+      // old samples fade out if we have collected too many
       if ( this.limitNumberOfSamples && this.skaterSamples.length > this.maxNumberOfSamples ) {
         const numberToRemove = this.skaterSamples.length - this.maxNumberOfSamples;
         for ( let i = 0; i < numberToRemove; i++ ) {
@@ -131,12 +139,23 @@ define( require => {
         }
       }
 
-      // step all samples - use ObservableArray.forEach to do on a copy of the array because this may involve array
-      // modification
+      // update opacity of SkaterSamples and determine if it is time for them to be removed from model
+      const samplesToRemove = [];
       this.skaterSamples.forEach( sample => {
         sample.step( dt );
+
+        if ( sample.opacityProperty.get() < SkaterSample.MIN_OPACITY ) {
+          samplesToRemove.push( sample );
+          sample.removalEmitter.emit();
+        }
       } );
 
+      // for performance, we batch removal of SkaterSamples so that we can redraw once after many have been removed
+      // rather than redraw on each data point, see https://github.com/phetsims/energy-skate-park/issues/198
+      if ( samplesToRemove.length > 0 ) {
+        this.batchRemoveSamplesProperty.set( samplesToRemove );
+      }
+
       return updatedState;
     }
 
diff --git a/js/energy-skate-park/common/model/SkaterSample.js b/js/energy-skate-park/common/model/SkaterSample.js
index 1aa90bd9..a1d9a81b 100644
--- a/js/energy-skate-park/common/model/SkaterSample.js
+++ b/js/energy-skate-park/common/model/SkaterSample.js
@@ -22,7 +22,7 @@ define( require => {
   const Vector2 = require( 'DOT/Vector2' );
 
   // constants
-  // inspect-able samples will fade out to this opacity before being fully removed from the model
+  // samples will fade out to this opacity before being fully removed from the model
   const MIN_OPACITY = 0.05;
 
   class SkaterSample {
@@ -109,10 +109,6 @@ define( require => {
     step( dt ) {
       if ( this._initiateRemove ) {
         this.opacityProperty.set( this.opacityProperty.get() * 0.95 );
-
-        if ( this.opacityProperty.get() < MIN_OPACITY ) {
-          this.removalEmitter.emit();
-        }
       }
 
       this.timeSinceAdded += dt;
@@ -134,5 +130,9 @@ define( require => {
     }
   }
 
+  // @public
+  // @static
+  SkaterSample.MIN_OPACITY = MIN_OPACITY;
+
   return energySkatePark.register( 'SkaterSample', SkaterSample );
 } );
diff --git a/js/energy-skate-park/graphs/view/EnergyPlot.js b/js/energy-skate-park/graphs/view/EnergyPlot.js
index d56c36d3..3da5484a 100644
--- a/js/energy-skate-park/graphs/view/EnergyPlot.js
+++ b/js/energy-skate-park/graphs/view/EnergyPlot.js
@@ -191,7 +191,6 @@ define( require => {
         const removalListener = removedSample => {
           if ( removedSample === addedSample ) {
             removedSample.opacityProperty.unlink( opacityListener );
-            this.forEachDataSeries( dataSeries => dataSeries.removePointAtX( independentVariable ) );
             model.skaterSamples.removeItemRemovedListener( removalListener );
           }
         };
@@ -199,6 +198,18 @@ define( require => {
 
         this.setCursorValue( independentVariable );
       } );
+
+      // remove a batch of of SkaterSamples, but only redraw once after all have been removed for performance
+      model.batchRemoveSamplesProperty.lazyLink( samplesToRemove => {
+        const plotTime = model.independentVariableProperty.get() === GraphsModel.IndependentVariable.TIME;
+        for ( let i = 0; i < samplesToRemove.length; i++ ) {
+          const sampleToRemove = samplesToRemove[ i ];
+          const independentVariable = plotTime ? sampleToRemove.time : sampleToRemove.position.x + 5;
+
+          this.forEachDataSeries( dataSeries => dataSeries.removePointAtX( independentVariable, false ) );
+        }
+        this.invalidateDataSeriesNodes();
+      } );
     }
 
     /**