Long strings can throw off pointer areas

[KatieWoe]

Test device Dell Operating System Win 10 Browser Firefox Problem description For https://github.com/phetsims/QA/issues/580. The way strings in the timer changes under string tests, such as ?stringTest=long, means that in the new rc, the timer looks longer. This throws off the grabbable area when in the toolbox. If you try to grab the measure tape, you often accidentally grab the timer.

Visuals

Troubleshooting information:

!!!!! DO NOT EDIT !!!!! Name: 12345678901234567890123456789012345678901234567890 URL: https://phet-dev.colorado.edu/html/waves-intro/1.1.0-rc.3/phet/waves-intro_all_phet.html?stringTest=long Version: 1.1.0-rc.3 2020-12-08 22:49:34 UTC Features missing: touch Flags: pixelRatioScaling User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:84.0) Gecko/20100101 Firefox/84.0 Language: en-US Window: 1280x687 Pixel Ratio: 1.5/1 WebGL: WebGL 1.0 GLSL: WebGL GLSL ES 1.0 Vendor: Mozilla (Mozilla) Vertex: attribs: 16 varying: 30 uniform: 4096 Texture: size: 16384 imageUnits: 16 (vertex: 16, combined: 32) Max viewport: 32767x32767 OES_texture_float: true Dependencies JSON: {}

[KatieWoe]

I also noticed that the wave inspector tool handles long strings differently now too, and it looks a bit odd. Now:

Published:

[samreid]

I confirmed the problem. Here are the bounds according to the scenery inspector:

I'll move the preceding comment to a new issue.

[samreid]

Even when I set the mouse and touch areas, the pickable bounds remains incorrect. Here is what ?showPointerAreas shows:

But I can still select the area indicated in the scenery inspector.

This seems like it should block publication, and I'll likely need @jonathanolson's help. The code for this is in wave-interference/js/common/view/ToolboxPanel.js

[samreid]

From slack:

Jonathan Olson 9:59 AM I think solving it is going to require rethinking how node.rasterized() sets up bounds and mouse/touch areas

Sam Reid 10:00 AM I see

Jonathan Olson 10:01 AM The actual Image is larger, but it's contained in a Node that has a smaller constrained localBounds overridden, but for input purposes, the larger Image's bounds are used, unless we want to do use hitTestPixels or something

Sam Reid 10:19 AM I don’t understand why the larger bounds have the wrong aspect ratio

Jonathan Olson 10:19 AM Text's getSafeSelfBounds(), which is used so text doesn't get clipped for purposes like rasterization

Sam Reid 10:22 AM Would it help to temporarily wrap the node to be rasterized in a Node({clip:…}) or will that get getSafeSelfBounds as well?

Jonathan Olson 10:24 AM Hmm, not sure yet

[samreid]

This patch hides the numbers before creating the icon, and doesn't suffer from the problem:

```diff Index: main/wave-interference/js/common/view/ToolboxPanel.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/wave-interference/js/common/view/ToolboxPanel.js b/main/wave-interference/js/common/view/ToolboxPanel.js --- a/main/wave-interference/js/common/view/ToolboxPanel.js (revision 81d3aa0070077681f8a9bb037fe3c57ef4b06c31) +++ b/main/wave-interference/js/common/view/ToolboxPanel.js (date 1611953572187) @@ -51,7 +51,10 @@ // Node used to create the icon isStopwatchVisibleProperty.value = true; + const x = stopwatchNode.getNumberFormatter(); + stopwatchNode.setNumberFormatter( () => '' ); const stopwatchNodeIcon = stopwatchNode.rasterized().mutate( { scale: 0.45 } ); + stopwatchNode.setNumberFormatter( x ); isStopwatchVisibleProperty.value = false; // The draggable icon, which has an overlay to make the buttons draggable instead of pressable Index: main/scenery-phet/js/StopwatchNode.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/scenery-phet/js/StopwatchNode.js b/main/scenery-phet/js/StopwatchNode.js --- a/main/scenery-phet/js/StopwatchNode.js (revision 18007999e489b1f4709ac13fb18695329d80b9d4) +++ b/main/scenery-phet/js/StopwatchNode.js (date 1611953511200) @@ -250,6 +250,10 @@ this.numberDisplay.setNumberFormatter( numberFormatter ); } + getNumberFormatter(){ + return this.numberDisplay.getNumberFormatter(); + } + // @public - redraw the text when something other than the numberProperty changes (such as units, formatter, etc). redrawNumberDisplay() { this.numberDisplay.recomputeText(); Index: main/scenery-phet/js/NumberDisplay.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/scenery-phet/js/NumberDisplay.js b/main/scenery-phet/js/NumberDisplay.js --- a/main/scenery-phet/js/NumberDisplay.js (revision 18007999e489b1f4709ac13fb18695329d80b9d4) +++ b/main/scenery-phet/js/NumberDisplay.js (date 1611953572205) @@ -217,6 +217,10 @@ this.recomputeText(); } + getNumberFormatter(){ + return this.numberFormatter; + } + // @public - redraw the text when something other than the numberProperty changes (such as units, formatter, etc). recomputeText() { this._recomputeText(); ```

[samreid]

Even an eroded clip doesn't solve this problem:

Index: js/common/view/ToolboxPanel.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/view/ToolboxPanel.js b/js/common/view/ToolboxPanel.js
--- a/js/common/view/ToolboxPanel.js    (revision 81d3aa0070077681f8a9bb037fe3c57ef4b06c31)
+++ b/js/common/view/ToolboxPanel.js    (date 1611954255724)
@@ -7,6 +7,8 @@
  */

 import Vector2 from '../../../../dot/js/Vector2.js';
+import Shape from '../../../../kite/js/Shape.js';
+import Node from '../../../../scenery/js/nodes/Node.js';
 import DragListener from '../../../../scenery/js/listeners/DragListener.js';
 import HBox from '../../../../scenery/js/nodes/HBox.js';
 import waveInterference from '../../waveInterference.js';
@@ -51,7 +53,12 @@

     // Node used to create the icon
     isStopwatchVisibleProperty.value = true;
-    const stopwatchNodeIcon = stopwatchNode.rasterized().mutate( { scale: 0.45 } );
+    const n = new Node({
+      children:[stopwatchNode]
+    });
+    n.clipArea=Shape.bounds(n.localBounds.eroded(10));
+    const stopwatchNodeIcon = n.rasterized().mutate( { scale: 0.45 } );
+    n.dispose();
     isStopwatchVisibleProperty.value = false;

     // The draggable icon, which has an overlay to make the buttons draggable instead of pressable

[samreid]

Setting the clip area of the rasterized node seems to work OK. I wonder if that would be a reasonable short/long term solution to add in rasterized by default? @jonathanolson what do you recommend?

This also seems ready to cherry-pick into the RC shas.

UPDATE: @KatieWoe can you please test in master? If it seems OK, I'll cherry-pick it to the release branch.

[KatieWoe]

Looks ok on master for Waves Intro

[samreid]

Thanks for testing, I'll mark for cherry picking into the release branch.

[jonathanolson]

Setting the clip area of the rasterized node seems to work OK. I wonder if that would be a reasonable short/long term solution to add in rasterized by default?

Seems like an ok temporary option, but wouldn't want to do it in general (the clipping would have performance characteristics that are undesirable in the general case).

[jonathanolson]

I think something that would hit the majority of use cases, IF wrap and useTargetBounds are true (the defaults), then we could make the wrapped image pickable:false and set mouse/touch areas on the wrapping Node.

It would also be possible to subclass Image with e.g. RasterizedImage, where it specifically overrides either selfBounds or containsSelfPoint, so that with hit-testing it only hits that main area, or other alternatives.

Thoughts? (Also @zepumph this is an ideal get-to-know-Scenery-internals issue)

[samreid]

That sounds like a good proposal to me.

[jonathanolson]

@zepumph would you be willing to discuss tradeoffs sometime, and go for an implementation together?

[zepumph]

@jonathanolson and I met to discuss this today. We didn't finish the conversation because we ran out of time. I took up most of the time asking questions about how scenery handles bounds as well as details about Picker.js and hit testing. I also learned about the strangeness behind Text.getSafeSelfBounds().

It isn't clear what would be best. @jonathanolson let me know if you want to discuss a bit more.

[jonathanolson]

@samreid I believe the above change to the resulting rasterized node should fix this. Can you try removing the workaround and verifying?

[samreid]

I removed the workaround, and tested with and without the scenery change, and confirmed the behavior is as desired. The scenery inspects still shows the larger bounds (even though they are not pickable), is that to be expected?

After discussing this last question, this issue can probably be closed.

[zepumph]

Yes this is expected. The reason is because of how Text.getSafeSelfBounds() works. This is basically a workaround estimate that is imperfect in this case for the long string, making the self bounds much larger than they actually are. What you are seeing here is that the Image itself includes all the transparent pixels on either side of the tool as you see above, but then we use the useTargetBounds option to manually set the localBounds (to correct for layout), and imageBounds to correct for hittesting (the part we just added). The transparent pixels still exist on the image.

[samreid]

If we know the bounds well enough to set the imageBounds, can those bounds used to be set the actual size of the raster? I'm trying to understand why the raster should include the transparent pixels on either side of the tool, if we know they should be clipped.

[zepumph]

if we know they should be clipped.

I think the point is that we don't know that they should be clipped? Since we don't know how well the text self bounds actually represents the text (because of browsers). I am running out of my ability to respond here. Perhaps I should tap in @jonathanolson.

[jonathanolson]

Rendered text can appear outside of the bounds that browsers give us. Only rasterizing those areas will likely clip off certain text, so generally we rasterize it with additional padding. The amount of padding needed is proportional to the size of the text, so we can't use a fixed amount.

[samreid]

Ah, that makes sense, thanks for helping me understand. I think this issue is ready to close.