Dropping circuit elements into the toolbox should have some tolerance around the center of the object

[matthew-blackman]

When investigating https://github.com/phetsims/circuit-construction-kit-common/issues/872, we found that it can be difficult to drop circuit elements back into the toolbox when they are zoomed in. To fix this and unblock large zoom levels, we suggest adding a tolerance so that the center of the item can be within a certain distance to the toolbox to be dropped back in.

[matthew-blackman]

@samreid ready for review, in particular would like to confirm that this behavior gives a consistent UX across all zoom levels and window sizes. The circuit element hitbox size is being defined in the constant RETURN_ITEM_BOUNDS_TOLERANCE, but can be made scale-dependent if necessary.

[samreid]

A few recommendations from the review:

• I think the arithmetic in these lines:

  const hitBoxMinX = globalMidpoint.x - CCKCConstants.RETURN_ITEM_BOUNDS_TOLERANCE;
  const hitBoxMinY = globalMidpoint.y - CCKCConstants.RETURN_ITEM_BOUNDS_TOLERANCE;
  const hitBoxMaxX = globalMidpoint.x + CCKCConstants.RETURN_ITEM_BOUNDS_TOLERANCE;
  const hitBoxMaxY = globalMidpoint.y + CCKCConstants.RETURN_ITEM_BOUNDS_TOLERANCE;
  
  const hitBox = new Bounds2( hitBoxMinX, hitBoxMinY, hitBoxMaxX, hitBoxMaxY );
  const overToolbox = toolbox.globalBounds.intersectsBounds( hitBox );

  may be simplified by using Bounds2.point and dilation.

• Next, I used this patch to detect if something is over the toolbox:

Subject: [PATCH] a
---
Index: js/view/CCKCScreenView.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/view/CCKCScreenView.ts b/js/view/CCKCScreenView.ts
--- a/js/view/CCKCScreenView.ts (revision 83fa1d35cb772a5392aaadfa53e2b6fc5ae2bd2c)
+++ b/js/view/CCKCScreenView.ts (date 1674258596996)
@@ -427,6 +427,16 @@
       }
     } );

+    // Use set interval to run a loop so that we can detect if any circuit element node can drop in the toolbox. If it does, write a message to the console
+    // so that we can see if this is happening in the wild.
+    setInterval( () => {
+      Object.values( this.circuitLayerNode.circuitElementNodeMap ).forEach( circuitElementNode => {
+        if ( this.canNodeDropInToolbox( circuitElementNode ) ) {
+          console.log( 'circuit element node can drop in toolbox' );
+        }
+      } );
+    } );
+
     // Re-render after setting state
     Tandem.PHET_IO_ENABLED && phet.phetio.phetioEngine.phetioStateEngine.stateSetEmitter.addListener( () => {
       this.step( 1 / 60 );

When I did this, I saw that horizontal test seemed good, but that the vertical test would allow items to fall into the toolbox when they are below it. For instance, in this picture, the battery is just 1px away from dropping into the toolbox:

[matthew-blackman]

Thanks for the ideas @samreid. Now using Bounds2.point and dilation for hitbox. The dropping behaviour looks good in my testing, with the vertical and horizontal bounds acting as expected.

[samreid]

Excellent, thanks! The arithmetic looks really nice. I noticed it is still possible to delete a circuit element by dropping it pretty far beneath the toolbox. Can this please be made more precise, possibly by using dilateXY?

[matthew-blackman]

That behavior looks okay to me as the intent looks like it would still be to return the wire rather than place it under the toolbox, although we could play with the hitbox size to get it closer. The issue with dilating x and y separately is that the bounds are independent of the component orientation and size. This means that if the component was taller we would run into the similar issues with horizontal offset.

One option could be to give the hitbox bounds the same x/y proportions and rotation as the element graphic, but this seems like overkill to me. I think a good option to pursue is to adjust the bounds size to be a good compromise for various circuit element dimensions. What do you think @samreid?

[samreid]

That is a good point, and I do not want to overcomplicate this. I did make 2 observations: first, in PhET-iO you can hide the schematic/lifelike radio buttons and see circuit elements get dropped when then that may not be the intention:

Secondly, I noticed with the hit box as it is right now, light bulbs can get pretty far across the toolbox without dropping in:

How about a minor change like this?

Subject: [PATCH] Add try/catch and assertions to Client.setLocaleFromQueryString, see https://github.com/phetsims/phet-io/issues/1901
---
Index: js/view/CCKCScreenView.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/view/CCKCScreenView.ts b/js/view/CCKCScreenView.ts
--- a/js/view/CCKCScreenView.ts (revision f65746167350325257802e5ececf9094bb0b059c)
+++ b/js/view/CCKCScreenView.ts (date 1674569517227)
@@ -521,7 +521,7 @@

     // Detect whether the midpoint between the vertices overlaps the toolbox
     const globalMidpoint = circuitElementNode.localToGlobalPoint( circuitElement.getMidpoint() );
-    const hitBox = Bounds2.point( globalMidpoint ).dilate( CCKCConstants.RETURN_ITEM_BOUNDS_TOLERANCE );
+    const hitBox = Bounds2.point( globalMidpoint ).dilateXY( 25, 2 );

     const overToolbox = toolbox.globalBounds.intersectsBounds( hitBox );

Index: js/CCKCConstants.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/CCKCConstants.ts b/js/CCKCConstants.ts
--- a/js/CCKCConstants.ts   (revision f65746167350325257802e5ececf9094bb0b059c)
+++ b/js/CCKCConstants.ts   (date 1674569472417)
@@ -27,9 +27,6 @@
   // Available scale factors for the sim stage
   ZOOM_SCALES: [ 0.5, 1, 1.6 ],

-  // When trying to drop an item back in the toolbox, this is the width the hit box
-  RETURN_ITEM_BOUNDS_TOLERANCE: 25,
-
   // Maximum size for Width or height of icons in the circuit element toolbox or sensor toolbox
   TOOLBOX_ICON_HEIGHT: 31,
   TOOLBOX_ICON_WIDTH: 60,

You can apply patches into WebStorm via Git => Patch => apply patch from clipboard.

[samreid]

In discussion with @arouinfar and @matthew-blackman, we agreed we want to use the centroid of the CircuitElementNode, not the midpoint between the vertices. Possibly with a smaller dilation (like 10).

Double check that things work well at different zoom levels. (should not change how easy it is to drop something in the toolbox).

@arouinfar also asked: can we add another predicate that makes it so if the mouse is over the toolbox, it also drops in?

@samreid said: Let's keep this from getting too complicated, but we can check on that.

UPDATE (start here):

• Check the carousel bounds--is it buggy (too tall)?
• we agreed we want to use the centroid of the CircuitElementNode, not the midpoint between the vertices.
• Possibly with a smaller dilation (like 10).

[matthew-blackman]

This is looking very good to me now. The carousel bounds issue seems to be fixed, and the dropping logic is now using the constant RETURN_ITEM_HITBOX_PERCENT to calculate the percent of the component bounds to use for the dropping hitbox. See image below (component hitboxes shown in red, carousel in green).

@arouinfar feel free to adjust the RETURN_ITEM_HITBOX_PERCENT so that dropping feels right. Right now it is set to 0.2, meaning that the dropping hitbox will be 20% of the object's width and 20% of its height.

@samreid adding you as well to review implementation details.

[arouinfar]

@matthew-blackman @samreid I tested on master and the return behavior feels good to me.

[samreid]

This looks great, nice work. Perhaps a code comment explaining this arithmetic? Is it because the erosion is from all edges?

const erosionPercent = 0.5 * ( 1 - CCKCConstants.RETURN_ITEM_HITBOX_PERCENT );

Also, it says:

  // When trying to drop an item back in the toolbox, this is the percent of its width and height used for the hitbox
  RETURN_ITEM_HITBOX_PERCENT: 0.2,

But I'm thinking it is 20% and not 0.2%, so perhaps rename and document as RETURN_ITEM_HITBOX_AMOUNT or RETURN_ITEM_HITBOX_PROPORTION or something?

[matthew-blackman]

I will rename RETURN_ITEM_HITBOX_PERCENT to RETURN_ITEM_HITBOX_RATIO and add documentation to the erosionPercent calculation.

[matthew-blackman]

@arouinfar @samreid and I agree that the sensor dropping should obey the same hitbox logic as the circuit elements. We like the 20% hitbox behavior and would like to propagate this to the sensor tool nodes. @samreid pointed out that this can be done in CCKCScreenView.ts, and should use the same erosion logic as the circuit elements. We propose adding a function to CCKUtils to do the hitbox erosion, so that there is a central source of logic for hitbox calculations.

[matthew-blackman]

Dropping logic has been centralized and applied to all sensors. @arouinfar please confirm that this is a good UX. @samreid please confirm that this is sufficiently refactored / documented. Once both have reviewed, this should be ready to close.

[arouinfar]

@matthew-blackman this feels so much better now, and I think it's a definite improvement.

[samreid]

Review complete, looks great. I just renamed one thing. Closing.