Skip to content

Commit

Permalink
remove SkaterSamples in batch to redraw canvas less frequently, see #198
Browse files Browse the repository at this point in the history
  • Loading branch information
jessegreenberg committed Feb 25, 2020
1 parent 265dc3f commit 59f3874
Show file tree
Hide file tree
Showing 3 changed files with 39 additions and 9 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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( [] );
}

/**
Expand All @@ -73,7 +79,9 @@ define( require => {
* @returns {}
*/
clearEnergyData() {
const samplesToRemove = this.skaterSamples.getArray().slice();
this.skaterSamples.clear();
this.batchRemoveSamplesProperty.set( samplesToRemove );
this.sampleTimeProperty.reset();
}

Expand Down Expand Up @@ -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++ ) {
Expand All @@ -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;
}

Expand Down
10 changes: 5 additions & 5 deletions js/energy-skate-park/common/model/SkaterSample.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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;
Expand All @@ -134,5 +130,9 @@ define( require => {
}
}

// @public
// @static
SkaterSample.MIN_OPACITY = MIN_OPACITY;

return energySkatePark.register( 'SkaterSample', SkaterSample );
} );
13 changes: 12 additions & 1 deletion js/energy-skate-park/graphs/view/EnergyPlot.js
Original file line number Diff line number Diff line change
Expand Up @@ -191,14 +191,25 @@ define( require => {
const removalListener = removedSample => {
if ( removedSample === addedSample ) {
removedSample.opacityProperty.unlink( opacityListener );
this.forEachDataSeries( dataSeries => dataSeries.removePointAtX( independentVariable ) );
model.skaterSamples.removeItemRemovedListener( removalListener );
}
};
model.skaterSamples.addItemRemovedListener( removalListener );

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();
} );
}

/**
Expand Down

0 comments on commit 59f3874

Please sign in to comment.