zepumph
commented
2 years ago I found that HSeparator and VSeparato were already set as strokePickable:true by default. This is nice because it limits down the number of cases that may have regressed. I'm running snapshot comparison against this patch right now and will report back:
```
Index: scenery/js/nodes/Line.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/scenery/js/nodes/Line.ts b/scenery/js/nodes/Line.ts
--- a/scenery/js/nodes/Line.ts (revision 39a6f77c6eb678487d059accdaf371780f16cfe0)
+++ b/scenery/js/nodes/Line.ts (date 1646425798540)
@@ -52,6 +52,12 @@
constructor( x1?: number | Vector2 | LineOptions, y1?: number | Vector2, x2?: number | LineOptions, y2?: number, options?: LineOptions ) {
super( null );
+
+ // TODO: DELETE ME
+ const strokePickableFalse = _.some( [ ...arguments ], arg => arg && arg.hasOwnProperty && arg.hasOwnProperty( 'strokePickable' ) && !arg.strokePickable );
+ if ( strokePickableFalse ) {
+ debugger;
+ }
this._x1 = 0;
this._y1 = 0;
this._x2 = 0;
@@ -72,18 +78,22 @@
y1: x1.y,
// Second Vector2 is under the y1 name
x2: ( y1 as Vector2 ).x,
- y2: ( y1 as Vector2 ).y
+ y2: ( y1 as Vector2 ).y,
+
+ strokePickable: true
}, x2 ); // Options object (if available) is under the x2 name
}
else {
// assumes Line( { ... } ), init to zero for now
assert && assert( y1 === undefined );
- // Options object is under the x1 name
- options = x1;
-
- assert && assert( options === undefined || Object.getPrototypeOf( options ) === Object.prototype,
+ assert && assert( x1 === undefined || Object.getPrototypeOf( x1 ) === Object.prototype,
'Extra prototype on Node options object is a code smell' );
+
+ // Options object is under the x1 name
+ options = extendDefined( {
+ strokePickable: true
+ }, x1 ); // Options object (if available) is under the x2 name
}
}
else {
@@ -99,11 +109,16 @@
x1: x1,
y1: y1,
x2: x2,
- y2: y2
+ y2: y2,
+ strokePickable: true
}, options );
}
this.mutate( options );
+ if ( !strokePickableFalse && !this.strokePickable ) {
+ debugger;
+ }
+ assert && !strokePickableFalse && assert( this.strokePickable, 'should be stroke pickable unless passed through' );
}
Index: energy-skate-park/js/common/view/ReferenceHeightLine.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/energy-skate-park/js/common/view/ReferenceHeightLine.js b/energy-skate-park/js/common/view/ReferenceHeightLine.js
--- a/energy-skate-park/js/common/view/ReferenceHeightLine.js (revision 3fba31fae1d5107c4a3e2ece29fdd21010d2c37e)
+++ b/energy-skate-park/js/common/view/ReferenceHeightLine.js (date 1646425018818)
@@ -42,9 +42,6 @@
const lineStart = new Vector2( 0, 0 );
const lineEnd = new Vector2( lineLength, 0 );
const frontLine = new Line( lineStart, lineEnd, {
-
- // strokes are not generally pickable, this allows the line itself to be dragged
- strokePickable: true,
lineWidth: 4,
lineDash: lineDash,
stroke: 'rgb(74,133,208)'
Index: geometric-optics/js/common/view/OpticalAxisNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/geometric-optics/js/common/view/OpticalAxisNode.ts b/geometric-optics/js/common/view/OpticalAxisNode.ts
--- a/geometric-optics/js/common/view/OpticalAxisNode.ts (revision be961c159683622627913a23cd2aa79639b31a01)
+++ b/geometric-optics/js/common/view/OpticalAxisNode.ts (date 1646425018825)
@@ -41,7 +41,6 @@
// LineOptions
stroke: GOColors.opticalAxisStrokeProperty,
- strokePickable: true,
lineWidth: 2,
lineDash: [ 8, 5 ]
}, providedOptions ) );
Index: sun/js/HSeparator.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/sun/js/HSeparator.js b/sun/js/HSeparator.js
--- a/sun/js/HSeparator.js (revision 51d6a7b8d7522a31a819dfd54b926a5a0e7d56ec)
+++ b/sun/js/HSeparator.js (date 1646425018821)
@@ -19,8 +19,7 @@
constructor( width, options ) {
options = merge( {
- stroke: 'rgb( 100, 100, 100 )',
- strokePickable: true
+ stroke: 'rgb( 100, 100, 100 )'
}, options );
super( 0, 0, width, 0, options );
Index: molecule-polarity/js/common/view/BondNode.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/molecule-polarity/js/common/view/BondNode.js b/molecule-polarity/js/common/view/BondNode.js
--- a/molecule-polarity/js/common/view/BondNode.js (revision 3652cc38bcd2cf915f224a1716e04a22da2ebd95)
+++ b/molecule-polarity/js/common/view/BondNode.js (date 1646425018805)
@@ -28,7 +28,6 @@
options = merge( {
stroke: MPColors.BOND,
lineWidth: 12,
- strokePickable: true, // include stroke in hit-testing
tandem: Tandem.REQUIRED,
visiblePropertyOptions: { phetioReadOnly: true }
}, options );
Index: sun/js/VSeparator.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/sun/js/VSeparator.js b/sun/js/VSeparator.js
--- a/sun/js/VSeparator.js (revision 51d6a7b8d7522a31a819dfd54b926a5a0e7d56ec)
+++ b/sun/js/VSeparator.js (date 1646425018810)
@@ -19,8 +19,7 @@
constructor( height, options ) {
options = merge( {
- stroke: 'rgb( 100, 100, 100 )',
- strokePickable: true
+ stroke: 'rgb( 100, 100, 100 )'
}, options );
super( 0, 0, 0, height, options );
```
I'm also running load-only tests to see if I hit any of the debuggers.
This is from https://github.com/phetsims/geometric-optics/issues/351, it seems like this case will come up much more often than just that one case.
From discussion with @jonathanolson, we like the idea @pixelzoom had in https://github.com/phetsims/geometric-optics/issues/351#issuecomment-1055826651 to have
strokePickable:truebe the default for allLineinstances.For testing, @jonathanolson and I are a bit worried about regressions, since if any ancestor to the Line as an input listener, it will cause this hit to be absorbed. In addition to pixel comparison, we will want to have QA take a good pass on a variety of sims.