Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ProbeNode: rotation results in incorrect default pointer areas #488

Closed
KatieWoe opened this issue Apr 11, 2019 · 37 comments
Closed

ProbeNode: rotation results in incorrect default pointer areas #488

KatieWoe opened this issue Apr 11, 2019 · 37 comments

Comments

@KatieWoe
Copy link

Documented in phetsims/beers-law-lab#236. Also seen in the Concentration sim. When looking at the pointer areas of the concentration tool, the blue one that indicates mouse interaction droops down. On my iphone it seemed to occur in landscape mode, but upon checking other platforms it seems to be the norm there. This pointer area doesn't seem to be actually be interactable and the tool itself can be moved normally. As such, there is no pedagogical impact and this is likely a minor issue.

Found on the published versions of the sims.

@pixelzoom
Copy link
Contributor

pixelzoom commented Apr 11, 2019

Screenshot from phetsims/beers-law-lab#236. Blue is the mouseArea.

@pixelzoom
Copy link
Contributor

pixelzoom commented Apr 11, 2019

This is bug in SCENERY_PHET/ProbeNode. There's a problem with the default pointer areas when the probe is rotated. The problem is seen in BLL for mouseArea only because we're overriding the default touchArea.

Default pointer areas are set like this in ProbeNode:

221    // Allow the client to override mouse and touch area, but fall back to the outline
222    var outline = createShape().close();
223    options.mouseArea = options.mouseArea || outline;
224    options.touchArea = options.touchArea || outline;

scenery-phet demo shows the default orientation of the probe:

screenshot_1128

If I rotate the scenery-phet demo ProbeNode by -Math.PI/2 (the orientation in BLL), the problem is visible for both the touchArea and mouseArea:

screenshot_1129

@pixelzoom pixelzoom changed the title Blue pointer area of concentration tool droops down Concentration probe mouseArea is wrong Apr 11, 2019
@pixelzoom pixelzoom changed the title Concentration probe mouseArea is wrong ProbeNode: rotation results in incorrect default pointer areas Apr 11, 2019
@pixelzoom
Copy link
Contributor

Transferring this issue from beers-law-lab to scenery-phet, since it's not specific to beers-law-lab.

@pixelzoom pixelzoom transferred this issue from phetsims/beers-law-lab Apr 11, 2019
@pixelzoom
Copy link
Contributor

I don't see anything obviously wrong with ProbeNode. @jonathanolson is a Node's transform applied to its pointer areas?

@jonathanolson
Copy link
Contributor

I don't see anything obviously wrong with ProbeNode. @jonathanolson is a Node's transform applied to its pointer areas?

The touch/mouse area are in the local coordinate frame, so generally no

@samreid
Copy link
Member

samreid commented Apr 11, 2019

I tested #488 (comment) using this patch:

Index: js/demo/ComponentsScreenView.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/demo/ComponentsScreenView.js	(revision c91007a5bbebb999ce4c0be0a3bf366a52d29571)
+++ js/demo/ComponentsScreenView.js	(date 1555003558000)
@@ -12,9 +12,10 @@
   'use strict';
 
   // modules
-  var BooleanProperty = require( 'AXON/BooleanProperty' );
+  var SimpleDragHandler = require( 'SCENERY/input/SimpleDragHandler' );
   var ArrowKeyNode = require( 'SCENERY_PHET/keyboard/ArrowKeyNode' );
   var ArrowNode = require( 'SCENERY_PHET/ArrowNode' );
+  var BooleanProperty = require( 'AXON/BooleanProperty' );
   var BracketNode = require( 'SCENERY_PHET/BracketNode' );
   var CapsLockKeyNode = require( 'SCENERY_PHET/keyboard/CapsLockKeyNode' );
   var Checkbox = require( 'SUN/Checkbox' );
@@ -39,10 +40,10 @@
   var HandleNode = require( 'SCENERY_PHET/HandleNode' );
   var HBox = require( 'SCENERY/nodes/HBox' );
   var HeaterCoolerNode = require( 'SCENERY_PHET/HeaterCoolerNode' );
-  var KeyboardHelpSection = require( 'SCENERY_PHET/keyboard/help/KeyboardHelpSection' );
   var HSlider = require( 'SUN/HSlider' );
   var HStrut = require( 'SCENERY/nodes/HStrut' );
   var inherit = require( 'PHET_CORE/inherit' );
+  var KeyboardHelpSection = require( 'SCENERY_PHET/keyboard/help/KeyboardHelpSection' );
   var Keypad = require( 'SCENERY_PHET/keypad/Keypad' );
   var KitSelectionNode = require( 'SCENERY_PHET/KitSelectionNode' );
   var LaserPointerNode = require( 'SCENERY_PHET/LaserPointerNode' );
@@ -529,7 +530,7 @@
       ],
       function() {
         probeNodeLayer.removeAllChildren();
-        probeNodeLayer.addChild( new ProbeNode( {
+        const probeNode = new ProbeNode( {
 
           // ProbeNode options
           color: colorProperty.value,
@@ -543,8 +544,14 @@
 
           // layout options
           x: layoutBounds.centerX,
-          y: layoutBounds.centerY
+          y: layoutBounds.centerY,
+          rotation: -Math.PI / 2,
+          cursor: 'pointer'
+        } );
+        probeNode.addInputListener( new SimpleDragHandler( {
+          start: function() {}
         } ) );
+        probeNodeLayer.addChild( probeNode );
       } );
 
     // Show a cross hairs in the middle of the screen so that we can verify that the sensor's origin is correct.

And found that the hand turned into a cursor over the probe itself, not over the purple/blue area.

@pixelzoom
Copy link
Contributor

pixelzoom commented Apr 12, 2019

I'm going to take a stab at debugging this in scenery playground. Some notes to myself:

  • http://phetsims.github.io/scenery/tests/playground.html
  • scene is the top-level node
  • namespacing is different, e.g. window.dot, window.kite, window.scenery, etc.
  • to show pointer areas, display.setPointerAreaDisplayVisible( true )
  • display.updateDisplay() is not automatically called; call it explicitly, or press the "Update" button

Example:

> display.setPointerAreaDisplayVisible( true )
> var rect = new scenery.Rectangle( 0, 0, 100, 100, { fill: 'red', left: 100, top: 100 } )
> scene.addChild( rect )
> rect.touchArea = rect.localBounds.dilatedXY( 10, 10 )
> display.updateDisplay()
> rect.rotation = Math.PI/4
> display.updateDisplay()

pixelzoom added a commit that referenced this issue Apr 12, 2019
@pixelzoom
Copy link
Contributor

pixelzoom commented Apr 12, 2019

Here's where the pointer areas are set in ProbeNode:

    var outline = createShape().close();
    options.mouseArea = options.mouseArea || outline;
    options.touchArea = options.touchArea || outline;

If I add 2 calls to outline.copy(), the problem goes away:

    var outline = createShape().close();
    options.mouseArea = options.mouseArea || outline.copy();
    options.touchArea = options.touchArea || outline.copy();

So I thought that perhaps each pointer area needed a unique shape. But I tried this, and it does not resolve the problem:

    options.mouseArea = options.mouseArea || createShape().close();
    options.touchArea = options.touchArea || createShape().close();

So I suspect that there's something funky about the Shape created by createShape().close(), and that the funkiness is resolved by calling copy.

@jonathanolson could you take a peek?

@jonathanolson jonathanolson self-assigned this Apr 12, 2019
@jonathanolson
Copy link
Contributor

I'll take a look.

@pixelzoom
Copy link
Contributor

@jonathanolson In case it's significant, note @samreid's findings in #488 (comment). For mouseArea, the cursor changes over the expected area. So it may just be that the pointer area shapes are drawn incorrectly.

Also note that part of the shape is drawn correctly, and part (the partial ellipse?) is not.

@KatieWoe
Copy link
Author

KatieWoe commented Apr 12, 2019

I saw that pH Scale Basics has an odd red pointer area that starts just off screen, connected to the Probe Node, that can be manipulated when on screen. Seems like it may be related.
Edit: Also appears in non-basic pH Scale
Image from iOS

@pixelzoom
Copy link
Contributor

@ariel-phet I had planned to create an RC (new release branch) for pH-Scale and pH Scale: Basics this coming Monday. But since this problem is occurring in those sims, I'm going to hold off. Please prioritize this issue, and let me know how you want me to proceed with RC.

@pixelzoom
Copy link
Contributor

pixelzoom commented Apr 13, 2019

A couple of things to note about the pH Scale case that @KatieWoe identified in #488 (comment):

  • pH Scale rotation is Math.PI/2, while the BLL rotation was -Math.PI/2
  • pH Scale shows a problem with touchArea specified by the client, while BLL shows a problem with ProbeNode's default touchArea

And a couple more clues:

  • in BLL, the problem is transient; it isn't visible until you move the probe around, and it comes and goes. In pH Scale, the problem is persistent.
  • The problem is present in the published versions of both sims.

@pixelzoom
Copy link
Contributor

This problem seems to be present in any sim that uses a rotated ProbeNode. In addition to beers-law-lab and ph-scale, that includes energy-skate-park (@samreid @jessegreenberg), see screenshot below.

@KatieWoe if you find other sims that are affected, please note in this issue, rather than sim-specific issues.

screenshot_1135

@ariel-phet
Copy link

Although this issue is odd, (and is worthy of investigation), it does not appear to be causing any noticeable issues (and it has existed for a long time without complaint in the published sims). It would be good to fix in the long run, but I do not see it as a blocking issue.

@pixelzoom you are free to proceed with RC's as you see fit.

@ariel-phet ariel-phet removed their assignment Apr 15, 2019
@ariel-phet
Copy link

@jonathanolson this is fairly easy for me to reproduce. Tried on both my windows laptop touchscreen and through the app on my phone. Basically just get the sim in this configuration:

phscale

Then put your finger in near the "sag" in the wire in the button left, and you can move the probe around.

Again, I don't see this as a major issue. Though easy to reproduce it is not a likely use case scenario, but it would be good to figure out why this other touch area appears.

@pixelzoom
Copy link
Contributor

4/25/19 dev meeting:

@ariel-phet requested that this issue be deferred and assigned to him.

@pixelzoom
Copy link
Contributor

pixelzoom commented Apr 25, 2019

@jonathanolson @ariel-phet FYI, the bogus touchArea (red rectangle) shown in #488 (comment) was a sim-specific bug. It has been fixed, see phetsims/ph-scale#77.

So the remaining problem here is the default pointer areas in ProbeNode, which does not affect interactivity. And the cause was identified by @jonathanolson in #488 (comment).

@ariel-phet
Copy link

@KatieWoe since this bug just shows something odd in a internally used tool, I am inclined to close this issue as wontfix and just have it be a known issue.

Feel free to close if you agree.

@KatieWoe KatieWoe closed this as completed May 6, 2019
@pixelzoom
Copy link
Contributor

Reopening. While I agree that it's OK to ignore the problem as it manifests in ProbeNode, I don't think it's a good idea to ignore the underlying problem in kite. Kite is a foundational repo, and that problem could easily manifest in other places. So I'd like to see an issue that identifies the problem in kite, and refers to this issue, before closing this issue.

@pixelzoom pixelzoom reopened this May 6, 2019
@pixelzoom
Copy link
Contributor

@jonathanolson identified the general problem in #488 (comment):

It looks like Shape.transformed is not behaving correctly for a certain class of transforms (depending on the exact scale/translation) ....

Recommended to have him create an issue for that, including any additional information that he knows about. In face-to-face discussion, I recall him providing more details that I don't see documented in in this issue.

@jonathanolson
Copy link
Contributor

Added details, image and a link in phetsims/kite#77. Closing here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants