phetsims / faradays-electromagnetic-lab

"Faraday's Electromagnetic Lab" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
1 stars 0 forks source link

Odd sign in Electron.ts #138

Closed samreid closed 8 months ago

samreid commented 8 months ago

During review #103, we observed this code:

https://github.com/phetsims/faradays-electromagnetic-lab/blob/c95a7b72f185d5cf982b7833cce6f7c7bbcbecf5/js/common/model/Electron.ts#L138-L141

We were surprised to see that new value = old value - change instead of new value = old value + change. This may indicate a sign error earlier in the code. If there is no sign error earlier, then this should be well-documented.

pixelzoom commented 8 months ago

Here's the reason why deltaPosition is subtracted:

  // Electron's position [0,1] along the coil segment that it occupies: 0=endPoint, 1=startPoint
  private coilSegmentPosition: number;

I admit that these "start" and "end" semantics are odd, but there's history here. This "position" along the coil segment was specified by Mike Dubson back in 2004/2005. It's baked into Electron, CoilSegment, and QuadraticBezierSpline.evaluate (another potential reason not to switch to Quadratic.ts). While we were repeatedly refining this, I saw no reason to make life more difficult by trying to invert the semantics to 0=startPoint, 1=endPoint. And once it was working, clearly documenting seemed fine.

So rather than spend time changing inverting the semantics (and probably breaking things in the process), I've duplicated the documentation where deltaPositon is subtracted:

      // Move the electron along the path. coilSegmentPosition is 1=start and 0=end, so we subtract the delta here.
      const deltaPosition = dt * MAX_COIL_SEGMENT_POSITION_DELTA * this.speedAndDirection *
                            this.speedScaleProperty.value * this.getSpeedScale();
      const newPosition = this.coilSegmentPosition - deltaPosition;

Back to @samreid for review, close if OK.

samreid commented 8 months ago

That is helpful, thanks! Closing.