all SkaterSample removal goes through batchRemoveSamples, see #198

phetsims · Feb 25, 2020 · b93da85 · b93da85
1 parent 59f3874
commit b93da85
Show file tree

Hide file tree

Showing 3 changed files with 24 additions and 17 deletions.
diff --git a/js/energy-skate-park/common/model/EnergySkateParkSaveSampleModel.js b/js/energy-skate-park/common/model/EnergySkateParkSaveSampleModel.js
@@ -76,15 +76,32 @@ define( require => {
     /**
      * Clear all saved data immediately and prepare to save data again.
      * @public
-     * @returns {}
      */
     clearEnergyData() {
-      const samplesToRemove = this.skaterSamples.getArray().slice();
-      this.skaterSamples.clear();
-      this.batchRemoveSamplesProperty.set( samplesToRemove );
+      this.batchRemoveSamples( this.skaterSamples.getArray().slice() );
       this.sampleTimeProperty.reset();
     }
 
+    /**
+     * SkaterSamples are removed from the model in batches for performance. This way we can remove many
+     * SkaterSamples and then update the view once after several are removed. The behavior of this sim
+     * is such that hundreds of SkaterSamples are frequencly removed at a time.
+     *
+     * Assumes that samplesToRemove is a sub-array of this.skaterSamples, in the right order.
+     *
+     * @public
+     * @param samplesToRemove
+     */
+    batchRemoveSamples( samplesToRemove ) {
+      this.batchRemoveSamplesProperty.set( samplesToRemove );
+
+      const indexOfFirstSample = this.skaterSamples.indexOf( samplesToRemove[ 0 ] );
+      this.skaterSamples.splice( indexOfFirstSample, samplesToRemove.length );
+
+      // broadcast that this batch of samples has been removed
+      this.batchRemoveSamplesProperty.set( samplesToRemove );
+    }
+
     /**
      * Begin to remove all samples, indicating that all existing samples should fade away.
      * @protected
@@ -114,13 +131,6 @@ define( require => {
         if ( !this.preventSampleSave && this.timeSinceSampleSave > this.saveSampleInterval ) {
           const newSample = new SkaterSample( updatedState, this.sampleTimeProperty.get() );
 
-          // listener that removes sample from list when it it time
-          const removalListener = () => {
-            newSample.removalEmitter.removeListener( removalListener );
-            this.skaterSamples.remove( newSample );
-          };
-          newSample.removalEmitter.addListener( removalListener );
-
           this.skaterSamples.add( newSample );
 
           this.timeSinceSampleSave = 0;
@@ -146,14 +156,13 @@ define( require => {
 
         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 );
+        this.batchRemoveSamples( samplesToRemove );
       }
 
       return updatedState;

diff --git a/js/energy-skate-park/common/model/SkaterSample.js b/js/energy-skate-park/common/model/SkaterSample.js
@@ -57,9 +57,6 @@ define( require => {
       // @public - the opacity of this skater sample, tied to visual representation
       this.opacityProperty = new NumberProperty( 1 );
 
-      // @public - emits an event when the skater sample has been removed from the model
-      this.removalEmitter = new Emitter();
-
       // @public - emits an event when this SkaterSample has updated in some way, like when energies change
       // due to a change in reference height
       this.updatedEmitter = new Emitter();

diff --git a/js/energy-skate-park/graphs/view/EnergyPlot.js b/js/energy-skate-park/graphs/view/EnergyPlot.js
@@ -133,7 +133,8 @@ define( require => {
         const indexOfSample = model.skaterSamples.indexOf( closestSample );
 
         assert && assert( indexOfSample >= 0, 'time of cursor needs to align with a skater sample' );
-        model.skaterSamples.splice( indexOfSample, model.skaterSamples.length - indexOfSample );
+
+        model.batchRemoveSamples( model.skaterSamples.getArray().slice( indexOfSample ) );
       } );
 
       // calculate new range of plot when zooming in or out