Document why the electrons are positioned as position.x + radius

[samreid]

During code review #103, we observed this code in ElectronsNode.ts

    if ( this.electron.getLayer() === this.coilLayer ) {

      // Move to the electron's position (at the electron's center) and apply inverse scale.
      const position = this.electron.position;
      this.matrix.rowMajor( ELECTRON_INVERSE_SCALE, 0, position.x + ELECTRON_RADIUS, 0,
        ELECTRON_INVERSE_SCALE, position.y + ELECTRON_RADIUS, 0, 0, 1 );
      assert && assert( this.matrix.isFinite(), 'matrix should be finite' );

We were surprised to see the transform set to the position plus the radius. Normally we would expect to see position minus the radius to locate it correctly. If the sign is correct, please add documentation explaining why.

[pixelzoom]

The offset (2nd arg) to new SpriteImage looks suspicious in ElectronNode.ts:

    electronNode.toCanvas( ( canvas, x, y, width, height ) => {
      spriteImage = new SpriteImage( canvas, new Vector2( ( x + electronNode.width / 2 ), ( y + electronNode.height / 2 ) ), {

If I change to:

spriteImage = new SpriteImage( canvas, new Vector2( electronNode.width / 2, electronNode.height / 2 ), {

... then I can also make this change:

      this.matrix.rowMajor(
-       ELECTRON_INVERSE_SCALE, 0, this.electron.x + ELECTRON_RADIUS,
-       0, ELECTRON_INVERSE_SCALE, this.electron.y + ELECTRON_RADIUS,
+       ELECTRON_INVERSE_SCALE, 0, this.electron.x,
+       0, ELECTRON_INVERSE_SCALE, this.electron.y,
        0, 0, 1
      );

But.... This pattern for the offset to new SpriteImage came from FieldNode.ts, and I recall @jonathanolson helping me to get it right. In FieldNode.ts:

    compassNeedleNode.toCanvas( ( canvas, x, y, width, height ) => {
      spriteImage = new SpriteImage( canvas, new Vector2( ( x + compassNeedleNode.width / 2 ), ( y + compassNeedleNode.height / 2 ) ), {

But then I do not see an adjustment of position in CompassNeedleSpriteInstance:

    // Inlined translation/rotation/scale for efficiency
    this.matrix.setToScaleTranslationRotationPoint( COMPASS_NEEDLE_INVERSE_SCALE, this.position, rotation );

That leads me to believe that the position of the needles may not be correct in FieldNode.

I'll need to consult with @jonathanolson on this.

[jonathanolson]

Hacked a patch to highlight center of sprites (green circle):

Subject: [PATCH] Circle in Canvas sprites
---
Index: js/display/drawables/SpritesCanvasDrawable.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/display/drawables/SpritesCanvasDrawable.js b/js/display/drawables/SpritesCanvasDrawable.js
--- a/js/display/drawables/SpritesCanvasDrawable.js	(revision 378e69cea0b2a4d3b1272406f635f8fc9bec2359)
+++ b/js/display/drawables/SpritesCanvasDrawable.js	(date 1711694106562)
@@ -93,6 +93,12 @@
           );
         }
 
+        wrapper.context.fillStyle = 'green';
+        // draw a circle at the origin
+        wrapper.context.beginPath();
+        wrapper.context.arc( 0, 0, 5, 0, 2 * Math.PI );
+        wrapper.context.fill();
+
         wrapper.context.restore();
       }

So my mistake was the assumption that the sprite node sources were things with bounds of 0,0,width,height (in which has the math above with width/2 and height/2 would be their centers). When these are fixed with their actual centers, we get the following:

Explaining with words would be a lot, so here's a pic of why "x + node.centerX" is correct when using toCanvas. Note the padding between the what the Canvas holds (blue), and the object/Node bounds (pink). IT IS NOT THE SAME ON ALL SIDES also (since it is pixel aligned). The x/y that toCanvas computes is unfortunately "origin - upperLeftOfCanvas" (in the Node's parent coordinate frame), it wasn't the best choice years ago, or the best naming.

[jonathanolson]

Presumably the stroke width changed the bounds off of the (0,0,width,height) for the field lines, that's why it was slightly off. Electrons were probably centered (approximately) and way off.

Committed a fix above, let me know how it looks.

[jonathanolson]

To further clarify, if you remove the x + part of things, the center will be off by a handful of pixels (since it removes the consideration of the padding).

[samreid]

The explanation from @jonathanolson sounds reasonable, I'll leave this assigned to @pixelzoom to review. Self-unassigning.

[pixelzoom]

Thanks @jonathanolson, your explanation makes sense, and the fix looks good.

I should apply this same fix to FieldNode, where I suspect that the position of the needles is slightly off for the same reason. (And I should take a look at the particles in Gas Properties too -- see phetsims/gas-properties#219.)

[pixelzoom]

I should apply this same fix to FieldNode, ...

Looks like @jonathanolson already did this -- thanks!

As a debugging tool (which I should have added a long time ago) I added FieldPositionsNode and the ?showFieldPositions query parameter. It puts a yellow dot where the center of each needle should be in the B-field visualization. For example:

Prior to @jonathanolson's fix, the positions were off by quite a bit:

After @jonathanolson's fix, the positions are spot on:

So thanks all for working on this, it exposed an important problem that is now fixed.

Closing.