Incorrect transformation of arcs in edge cases

[jonathanolson]

From phetsims/scenery-phet#488

Details include:

I'm mostly confirming that this is manifesting only as a display-only problem.

It looks like Shape.transformed is not behaving correctly for a certain class of transforms (depending on the exact scale/translation) for the probe shape. This is used for the display, but NOT in the hit testing.

For instance a buggy case is the Shape:

M 0.00000000000000000000 59.00000000000000000000 L -2.99999999999999911182 59.00000000000000000000 A 12.00000000000000000000 12.00000000000000000000 0 0 1 -15.00000000000000000000 47.00000000000000000000 L -15.00000000000000000000 44.00000000000000000000 Q -15.00000000000000000000 34.00000000000000000000 -27.50657780874820801387 19.98469857794409065832 A 34.00000000000000000000 34.00000000000000000000 0 1 1 27.50657780874821156658 19.98469857794408355289 Q 15.00000000000000000000 34.00000000000000000000 15.00000000000000000000 44.00000000000000000000 L 15.00000000000000000000 47.00000000000000000000 A 12.00000000000000000000 12.00000000000000000000 0 0 1 3.00000000000000088818 59.00000000000000000000 L 0.00000000000000000000 59.00000000000000000000 Z

Transforming with the matrix:

4.55902603864401e-17 -0.7445454545454545 274.80218736910194
0.7445454545454545 4.55902603864401e-17 460.99406445080257
0 0 1

ends with the buggy case:

M 230.87400555092011700253 460.99406445080256844449 L 230.87400555092011700253 458.76042808716618992548 A 8.93454545454545367988 8.93454545454545367988 0 0 1 239.80855100546557423513 449.82588263262073269289 L 242.04218736910195275414 449.82588263262073269289 Q 249.48764191455649097406 449.82588263262073269289 259.92267088243266925929 440.51416697319820059420 A 25.31454545454545268512 25.31454545454545268512 0 1 1 289.68170385577121805909 440.51416697319820059420 Q 249.48764191455649097406 472.16224626898440419609 242.04218736910195275414 472.16224626898440419609 L 239.80855100546557423513 472.16224626898440419609 A 8.93454545454545367988 8.93454545454545367988 0 0 1 230.87400555092011700253 463.22770081443894696349 L 230.87400555092011700253 460.99406445080256844449 Z

resulting in the bad touch area displayed:

[pixelzoom]

This was reported again in phetsims/ph-scale#222, and continues to be reported by QA. I realize that it's a "display only" problem, and doesn't impact usability. But it would be nice to address, so that's we're not spending QA/dev resources on the same issue repeatedly.

[samreid]

This may be due to a sign error in EllitpicalArc.js, I should run a pixel comparison test and see if it shows any discrepancies. @jonathanolson please chime in if the sign change seems good to you:

Index: js/segments/EllipticalArc.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/segments/EllipticalArc.js b/js/segments/EllipticalArc.js
--- a/js/segments/EllipticalArc.js	(revision bc7e09afea97bab68aad30aeb0fd9e57bd4fd515)
+++ b/js/segments/EllipticalArc.js	(date 1626210327688)
@@ -641,8 +641,8 @@
     }
     else if ( this._radiusX === this._radiusY ) {
       // reduce to an Arc
-      const startAngle = this._startAngle - this._rotation;
-      let endAngle = this._endAngle - this._rotation;
+      const startAngle = this._startAngle + this._rotation;
+      let endAngle = this._endAngle + this._rotation;
 
       // preserve full circles
       if ( Math.abs( this._endAngle - this._startAngle ) === Math.PI * 2 ) {

[samreid]

I ran a snapshot comparison with that delta, without showPointerAreas and the following sims showed differences:

• balloons-and-static-electricity
• faradays-law
• fourier-making-waves
• friction
• gravity-force-lab
• gravity-force-lab-basics
• john-travoltage
• natural-selection
• neuron

Everything else is clear. Let's try to narrow these down.

[samreid]

I tested the top 3, natural selection, and neuron and couldn't see any differences anywhere. But I did notice many of these sims have keyboard navigation features. I'm inclined to push the fix and ask people to keep an eye out.

[samreid]

I slacked dev-public:

I fixed a sign error in EllipticalArc when it reduces to a regular Arc. This was manifesting in the probe pointer area as described in #77. The snapshot comparison tool indicates changes in 9 sims. I looked carefully at more than half of them and couldn’t see the discrepancy. Probably this means something minor imperceptibly changed. But I thought I should let you know in case you see something odd.

@jonathanolson can you please review?

[samreid]

In slack @pixelzoom said:

Thanks. That came up today for the Nth time in ph-scale, see phetsims/ph-scale#222. Good to know it’s fixed. I’ll verify and close related issues in all of my sims that have probes. I don’t think there’s a need to patch any release branches, because this was a display-only bug.

Do you think there might be anything that was relying on this bug in EllipticalArc ?

I replied:

We have hypothesized that it was a display-only bug, but from the fix, it looks like it would affect anything that was using EllipticalArc.getNondegenerateSegments when (a) radiusX===radiusY and (b) the rotation is nonzero.

I can only hope that no code was relying on the buggy behavior. I tested several sims that rendered differently with the fix, and couldn’t pinpoint it.

[pixelzoom]

I've tested in my sims that use ProbeNode, and this fix looks good. But I'm going to wait to close sim-specific issues until @jonathanolson has reviewed and closed this issue.

[jonathanolson]

Looks like a perfect fix, thank you!

[samreid]

Thanks, closing.