Incorrect geometry is sometimes drawn for rays.

[pixelzoom]

EDIT: This occurs for all objects and lights sources, for real and virtual rays.

While dragging the second light source around, I sometimes see odd geometry being drawn, presumably related to the rays. For example, see the pink triangle in the screenshot below. I have not seen this with the first light source.

I saw this on MacBookPro16,1 + macOS 11.5 + Chrome 94.0.4606.54. Not tested on other platforms.

@arouinfar Should this be addressed for the Prototype milestone?

[pixelzoom]

I also see this problem in 1.0.0-dev.9 from 7/30/21, before I started working on the sim. So this has been an existing problem. For example:

[pixelzoom]

This problem seems to be specific to rays for a virtual image. I have not seen this problem with a real image.

Here's a particularly bad example - and note that this is with an object, not a light source.

I've also seen this with the rays for the first object/light, see below. So it's not specific to the second object/light.

[pixelzoom]

#216 makes this a moot issue - we're no longer showing virtual rays for light sources.

Closing.

[pixelzoom]

Reopening. This is still occurring (frequently!) for real rays, for both light sources. macOS 11.6 + Chrome 94.0.4606.71, untested on other platforms. I was also seeing it on macOS 10.5 before updating.

[pixelzoom]

I'm not seeing this with Safari 15.0 or Firefox 93.0.

[pixelzoom]

If I disable the drag bounds for Objects, I can also make this happen with Objects. It seems specific to the lower-left area of the screen, and the drag bounds for Objects (and their height) precludes them from being dragged there.

[pixelzoom]

@arouinfar Does this issue need to be addressed for the Prototype? I'll assume it is, until I hear otherwise. I have not started investigating, so I have no idea how much time will be involved in fixing this.

[pixelzoom]

Ugh... It's also a problem for virtual rays associated with an Object. Again, in the same lower-left quadrant of the screen:

[pixelzoom]

The code that creates the Shape for each individual ray is in LightRay.js. In updateShape, this consists of calls to Shape moveToPoint and lineTo.

The code that combines the individual rays is in LightRays.js. Because we'll want to use different strokes for real vs virtual rays, there are separate Shapes for real and virtual rays, this.realRaysShape and this.virtualRaysShape respectively. The Shapes of individual rays are combined in LightRays addRayShape by calling Shape addSubpath.

I don't see any calls to Shape close, which is what I'd typically look for when I see closed (filled) paths like this. And the Paths used to render the rays also have no fill, so it's curious that they are getting filled.

@jonathanolson any thoughts about what might be happening here? What would cause a Path with no fill to render like this?

[pixelzoom]

Another example, this time NOT in the lower-left quadrant, and using a mirror:

[samreid]

I was able to reproduce the problem on macOS 11.6 on Chrome Version 94.0.4606.71 (Official Build) (arm64), and was surprised to see this problem with rootRenderer=canvas as well. As a workaround, I tried separate Line instances for each Ray, and it seems to work around the problem (though would not be as fast performance unless we added pooling--but perhaps this sim is not performance constrained?). This could be a good practical case to understand the limits of our renderer, but I'm supplying the workaround in case we want to move forward with it in the meantime:

Index: js/common/model/LightRay.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/model/LightRay.js b/js/common/model/LightRay.js
--- a/js/common/model/LightRay.js	(revision 31bcd280e2eed3f36eb7aae045f862c353f0bfe1)
+++ b/js/common/model/LightRay.js	(date 1633478051956)
@@ -44,10 +44,10 @@
     assert && assert( typeof getProjectorScreenBisectorLine === 'function' );
 
     // @public (read-only) shape of the real rays - will be updated later
-    this.realShape = new Shape();
+    this.realShape = [];
 
     // @public (read-only) shape of the virtual rays - will be updated later
-    this.virtualShape = new Shape();
+    this.virtualShape = [];
 
     // {number} maximum travel distance if ray is unimpeded
     const distanceTraveled = GeometricOpticsQueryParameters.lightSpeed * time;
@@ -477,7 +477,8 @@
     const endPoint = ray.pointAtDistance( travelDistance );
 
     // add line to shape
-    shape.moveToPoint( ray.position ).lineToPoint( endPoint );
+    // shape.moveToPoint( ray.position ).lineToPoint( endPoint );
+    shape.push( { start: ray.position, end: endPoint } );
   }
 }
 
Index: js/common/model/LightRays.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/model/LightRays.js b/js/common/model/LightRays.js
--- a/js/common/model/LightRays.js	(revision 31bcd280e2eed3f36eb7aae045f862c353f0bfe1)
+++ b/js/common/model/LightRays.js	(date 1633478051961)
@@ -35,10 +35,10 @@
     this.targetPositionProperty = target.positionProperty;
 
     // @public (read-only) shape of a bundle of real rays at a point in time
-    this.realRay = new Shape();
+    this.realRay = [];
 
     // @public (read-only) shape of a bundle of virtual rays at a point in time
-    this.virtualRay = new Shape();
+    this.virtualRay = [];
 
     // @public tells view that it needs to update, fires after all rays are processed.
     this.raysProcessedEmitter = new Emitter();
@@ -57,10 +57,10 @@
       ( sourcePosition, lightRayMode, time, representation ) => {
 
         // @public (read-only)
-        this.realRay = new Shape();
+        this.realRay = [];
 
         // @public (read-only)
-        this.virtualRay = new Shape();
+        this.virtualRay = [];
 
         // {Vector2} the position the target
         const targetPoint = this.targetPositionProperty.value;
@@ -203,8 +203,8 @@
    * @param {Shape} typeRayShape
    */
   addRayShape( rayShape, typeRayShape ) {
-    rayShape.subpaths.forEach( subPath => {
-      typeRayShape.addSubpath( subPath );
+    rayShape.forEach( element => {
+      typeRayShape.push( element );
     } );
   }
 }
Index: js/common/view/LightRaysNode.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/view/LightRaysNode.js b/js/common/view/LightRaysNode.js
--- a/js/common/view/LightRaysNode.js	(revision 31bcd280e2eed3f36eb7aae045f862c353f0bfe1)
+++ b/js/common/view/LightRaysNode.js	(date 1633478070388)
@@ -12,7 +12,7 @@
 import merge from '../../../../phet-core/js/merge.js';
 import ModelViewTransform2 from '../../../../phetcommon/js/view/ModelViewTransform2.js';
 import Node from '../../../../scenery/js/nodes/Node.js';
-import Path from '../../../../scenery/js/nodes/Path.js';
+import Line from '../../../../scenery/js/nodes/Line.js';
 import geometricOptics from '../../geometricOptics.js';
 import LightRays from '../model/LightRays.js';
 
@@ -39,14 +39,9 @@
       virtualRayLineWidth: 2
     }, options );
 
-    const realRayPath = new Path( modelViewTransform.modelToViewShape( lightRays.realRay ), {
-      stroke: options.realRayStroke,
-      lineWidth: options.realRayLineWidth
-    } );
+    const realRayPath = new Node();
 
-    const virtualRayPath = new Path( modelViewTransform.modelToViewShape( lightRays.virtualRay ), {
-      stroke: options.virtualRayStroke,
-      lineWidth: options.virtualRayLineWidth,
+    const virtualRayPath = new Node( {
 
       // Show virtual rays only for objects, not for light source. See https://github.com/phetsims/geometric-optics/issues/216
       visibleProperty: new DerivedProperty(
@@ -62,8 +57,25 @@
 
     // Update this Node when the model tells us that it's time to update.
     lightRays.raysProcessedEmitter.addListener( () => {
-      realRayPath.shape = modelViewTransform.modelToViewShape( lightRays.realRay );
-      virtualRayPath.shape = modelViewTransform.modelToViewShape( lightRays.virtualRay );
+      realRayPath.children = lightRays.realRay.map( term => {
+        return new Line(
+          modelViewTransform.modelToViewPosition( term.start ),
+          modelViewTransform.modelToViewPosition( term.end ), {
+            stroke: options.realRayStroke,
+            lineWidth: options.realRayLineWidth
+          }
+        );
+      } );
+
+      virtualRayPath.children = lightRays.virtualRay.map( term => {
+        return new Line(
+          modelViewTransform.modelToViewPosition( term.start ),
+          modelViewTransform.modelToViewPosition( term.end ), {
+            stroke: options.virtualRayStroke,
+            lineWidth: options.virtualRayLineWidth
+          }
+        );
+      } );
     } );
   }

UPDATE: I made a few minor improvements to the patch.

[arouinfar]

Reopening. This is still occurring (frequently!) for real rays, for both light sources. macOS 11.6 + Chrome 94.0.4606.71.

I've seen this in macOS 10.15.7/Chrome 94.0.4606.61, but only as a brief flicker when using the sim in the color editor. I don't know how to reliably reproduce. I haven't seen it at all when using the regular unbuilt sim on master.

@arouinfar Does this issue need to be addressed for the Prototype? I'll assume it is, until I hear otherwise.

Since this seems to happen often on some platforms, I'd say it's blocking. The glitch looks pretty bad in the screenshots.

[pixelzoom]

Thanks for the patch @samreid. I had considered going this route (a Path for each ray). But I was trying to change the existing implementation as little as possible for the Prototype, especially code like this that involves animation. I'll keep this in mind if I can't make forward progress on the existing implementation.

[pixelzoom]

In #219, @samreid noted that Shape addSubPath is private. So that tips the balance in favor of using the alternative approach (a Path for each ray) identified in #209 (comment).

[arouinfar]

Just saw this in master on macOS 10.15.7 & Chrome 94.0.4606.71 (latest). The sim wasn't in an iframe and the dev tools were closed.

I used the sim's built-in screenshot function. When the Chrome download bar popped up and resized the viewport, the glitch went away. However, when I closed the download bar, it came back.

[pixelzoom]

In the above commit, the implementation of rays (model and view) was changed in some major ways. I started down the path that @samreid established in #209 (comment). But I made a couple of improvements: (1) the model creates instances of LightRaySegment instead of an untyped object literal, and (2) the view creates only 2 Paths (with 2 Shapes) for rendering the real and virtual rays. And we're no longer using Shape addSubPath, just a series of moveToPoint and lineToPoint calls.

And after all that work, I'm still seeing the same behavior - maybe even worse. 🤯

[pixelzoom]

Before making these changes, I should have publised a dev version, so that we could verify whether anything "broken" was already broken. So in the above commits, I reverted changes, published 1.0.0-dev.15, restored the changes, and published 1.0.0-dev.16.

Before changes: 1.0.0-dev.15

After changes: 1.0.0-dev.16

[pixelzoom]

In the above commit, I rendered each line segment of the rays as a separate scenery.Line Node. And the problem is gone. No idea why this is a problem as a single Path, but I don't have time to investigate. And I'm not seeing any performance problems.

@arouinfar please review in master or 1.0.0-dev.17.

[arouinfar]

@pixelzoom I haven't been able to reproduce in dev.17 and I'm not seeing a difference in performance compared to dev.15. Closing.