Code review for TemporalMask and usages

[samreid]

A significant new amount of code was developed in order to address #258. Once that solution is approved and vetted by the design team, and perhaps gone through dev testing, it would be good to have a code review. Pinging @pixelzoom and @ariel-phet so they are aware this is coming up.

[pixelzoom]

Is this something new for the 1.0 release?

[samreid]

Yes

[samreid]

@ariel-phet asked me to check if @pixelzoom is available to work on this. Leaving self-assigned to make sure everything is all set.

[samreid]

@pixelzoom are you available to review this new component? TemporalMask.js was introduced to address #258. The code to be reviewed would be TemporalMask.js (119 lines of code) and its usages in Scene.js. Ready when you are.

[pixelzoom]

Sure. I can get to it first thing Monday 1/14. Is that soon enough?

[samreid]

You bet, thanks!

[pixelzoom]

I had time to review this today. Looks good, but a few questions/observations...

(1) In matches, I see:

      // search back through time to see if the source contributed to the value at (i,j) at the current time
      for ( let k = 0; k < this.deltas.length; k++ ) {

"search back in time" seems to indicate that we'll be looking at elements from newest to oldest. But
set adds elements to deltas using push, so this is searching oldest to newest. Either the comment is misleading or the for iteration is incorrect.

(2) Elements in deltas are objects of type {on: boolean, time: number, j: number}. These field names could use some work. time is misleading, it's actually a "number of times" that something has been done. j is not at all descriptive, it's the vertical lattice coordinate. Maybe something like:

{
  isSourceOn: boolean // true if the source is on, false if the source is off
  numberOfSteps: number // integer number of times the wave has been stepped on the lattice
  yLattice: number // vertical lattice coordinate
}

(3) The elements in deltas are objects. Are there a lot of these? Would it be more efficient to use parallel arrays?

[samreid]

I committed proposed fixes for (1) and (2). For (2) I used the name verticalLatticeCoordinate because it seemed cleared. (3) shouldn't be problematic because a delta is only created when the wave is turned on/off or moved, and hence are rare.

[pixelzoom]

👍 Closing.