fix: Binding hit test

Signed-off-by: Mark Tolmacs <mark@lazycat.hu>

packages/element/src/arrows/focus.ts

@@ -20,7 +20,7 @@ import {
   isElbowArrow,
 } from "../typeChecks";
 import { LinearElementEditor } from "../linearElementEditor";
-import { getHoveredElementForFocusPoint, hitElementItself } from "../collision";
+import { getHoveredElementForBinding, hitElementItself } from "../collision";
 import { moveArrowAboveBindable } from "../zindex";
 
 import type {
@@ -234,9 +234,9 @@ export const handleFocusPointDrag = (
     pointerCoords.y - offsetY,
   );
   const bindingField = isStartBinding ? "startBinding" : "endBinding";
-  const hit = getHoveredElementForFocusPoint(
+  const hit = getHoveredElementForBinding(
-    point,
     arrow,
+    point,
     scene.getNonDeletedElements(),
     elementsMap,
     maxBindingDistance_simple(appState.zoom),

packages/element/src/binding.ts

@@ -270,6 +270,7 @@ const bindingStrategyForElbowArrowEndpointDragging = (
     elementsMap,
   );
   const hit = getHoveredElementForBinding(
+    arrow,
     globalPoint,
     elements,
     elementsMap,
@@ -321,7 +322,7 @@ const bindingStrategyForNewSimpleArrowEndpointDragging = (
     draggingPoints.get(startDragged ? startIdx : endIdx)!.point,
     elementsMap,
   );
-  const hit = getHoveredElementForBinding(point, elements, elementsMap);
+  const hit = getHoveredElementForBinding(arrow, point, elements, elementsMap);
 
   // With new arrows this handles the binding at arrow creation
   if (startDragged) {
@@ -366,7 +367,12 @@ const bindingStrategyForNewSimpleArrowEndpointDragging = (
     // Check and handle nested shapes
     if (hit && arrow.startBinding) {
       const startBinding = arrow.startBinding;
-      const allHits = getAllHoveredElementAtPoint(point, elements, elementsMap);
+      const allHits = getAllHoveredElementAtPoint(
+        arrow,
+        point,
+        elements,
+        elementsMap,
+      );
 
       if (allHits.find((el) => el.id === startBinding.elementId)) {
         const otherElement = elementsMap.get(
@@ -468,9 +474,9 @@ const bindingStrategyForSimpleArrowEndpointDragging_complex = (
   let other: BindingStrategy = { mode: undefined };
 
   const isMultiPoint = arrow.points.length > 2;
-  const hit = getHoveredElementForBinding(point, elements, elementsMap);
+  const hit = getHoveredElementForBinding(arrow, point, elements, elementsMap);
   const isOverlapping = oppositeBinding
-    ? getAllHoveredElementAtPoint(point, elements, elementsMap).some(
+    ? getAllHoveredElementAtPoint(arrow, point, elements, elementsMap).some(
         (el) => el.id === oppositeBinding.elementId,
       )
     : false;
@@ -695,6 +701,7 @@ const getBindingStrategyForDraggingBindingElementEndpoints_simple = (
     elementsMap,
   );
   const hit = getHoveredElementForBinding(
+    arrow,
     globalPoint,
     elements,
     elementsMap,

packages/element/src/collision.ts

@@ -65,7 +65,6 @@ import { hasBackground } from "./comparisons";
 
 import type {
   ElementsMap,
-  ExcalidrawArrowElement,
   ExcalidrawBindableElement,
   ExcalidrawDiamondElement,
   ExcalidrawElement,
@@ -254,25 +253,20 @@ export const hitElementBoundText = (
   return isPointInElement(point, boundTextElement, elementsMap);
 };
 
-const bindingBorderTest = (
+const borderDistance = (
   element: NonDeleted<ExcalidrawBindableElement>,
-  [x, y]: Readonly<GlobalPoint>,
+  point: GlobalPoint,
-  elementsMap: NonDeletedSceneElementsMap,
+  elementsMap: ElementsMap,
   tolerance: number = 0,
-): boolean => {
+) => {
-  const p = pointFrom<GlobalPoint>(x, y);
-  const shouldTestInside =
-    // disable fullshape snapping for frame elements so we
-    // can bind to frame children
-    !isFrameLikeElement(element);
-
   // PERF: Run a cheap test to see if the binding element
   // is even close to the element
+  const [x, y] = point;
   const t = Math.max(1, tolerance);
   const bounds = [x - t, y - t, x + t, y + t] as Bounds;
   const elementBounds = getElementBounds(element, elementsMap);
   if (!doBoundsIntersect(bounds, elementBounds)) {
-    return false;
+    return -Infinity;
   }
 
   // If the element is inside a frame, we should clip the element
@@ -283,26 +277,70 @@ const bindingBorderTest = (
         enclosingFrame,
         elementsMap,
       );
-      if (!pointInsideBounds(p, enclosingFrameBounds)) {
+      if (!pointInsideBounds(point, enclosingFrameBounds)) {
-        return false;
+        return -Infinity;
       }
     }
   }
 
-  // Do the intersection test against the element since it's close enough
+  const distance = distanceToElement(element, elementsMap, point);
-  const intersections = intersectElementWithLineSegment(
+  if (isPointInElement(point, element, elementsMap)) {
-    element,
+    return distance;
-    elementsMap,
+  }
-    lineSegment(elementCenterPoint(element, elementsMap), p),
-  );
-  const distance = distanceToElement(element, elementsMap, p);
 
-  return shouldTestInside
+  return distance > tolerance ? -Infinity : -distance;
-    ? intersections.length === 0 || distance <= tolerance
-    : intersections.length > 0 && distance <= t;
 };
 
+// const bindingBorderTest = (
+//   element: NonDeleted<ExcalidrawBindableElement>,
+//   [x, y]: Readonly<GlobalPoint>,
+//   elementsMap: NonDeletedSceneElementsMap,
+//   tolerance: number = 0,
+// ): boolean => {
+//   const p = pointFrom<GlobalPoint>(x, y);
+//   const shouldTestInside =
+//     // disable fullshape snapping for frame elements so we
+//     // can bind to frame children
+//     !isFrameLikeElement(element);
+
+//   // PERF: Run a cheap test to see if the binding element
+//   // is even close to the element
+//   const t = Math.max(1, tolerance);
+//   const bounds = [x - t, y - t, x + t, y + t] as Bounds;
+//   const elementBounds = getElementBounds(element, elementsMap);
+//   if (!doBoundsIntersect(bounds, elementBounds)) {
+//     return false;
+//   }
+
+//   // If the element is inside a frame, we should clip the element
+//   if (element.frameId) {
+//     const enclosingFrame = elementsMap.get(element.frameId);
+//     if (enclosingFrame && isFrameLikeElement(enclosingFrame)) {
+//       const enclosingFrameBounds = getElementBounds(
+//         enclosingFrame,
+//         elementsMap,
+//       );
+//       if (!pointInsideBounds(p, enclosingFrameBounds)) {
+//         return false;
+//       }
+//     }
+//   }
+
+//   // Do the intersection test against the element since it's close enough
+//   const intersections = intersectElementWithLineSegment(
+//     element,
+//     elementsMap,
+//     lineSegment(elementCenterPoint(element, elementsMap), p),
+//   );
+//   const distance = distanceToElement(element, elementsMap, p);
+
+//   return shouldTestInside
+//     ? intersections.length === 0 || distance <= tolerance
+//     : intersections.length > 0 && distance <= t;
+// };
+
 export const getAllHoveredElementAtPoint = (
+  arrow: { elbowed: boolean },
   point: Readonly<GlobalPoint>,
   elements: readonly Ordered<NonDeletedExcalidrawElement>[],
   elementsMap: NonDeletedSceneElementsMap,
@@ -322,7 +360,13 @@ export const getAllHoveredElementAtPoint = (
 
     if (
       isBindableElement(element, false) &&
-      bindingBorderTest(element, point, elementsMap, tolerance)
+      hitElementItself({
+        element,
+        point,
+        elementsMap,
+        threshold: tolerance ?? getBindingGap(element, arrow),
+        overrideShouldTestInside: true,
+      })
     ) {
       candidateElements.push(element);
 
@@ -339,82 +383,42 @@ export const getAllHoveredElementAtPoint = (
 };
 
 export const getHoveredElementForBinding = (
+  arrow: { elbowed: boolean },
   point: Readonly<GlobalPoint>,
   elements: readonly Ordered<NonDeletedExcalidrawElement>[],
   elementsMap: NonDeletedSceneElementsMap,
   tolerance?: number,
 ): NonDeleted<ExcalidrawBindableElement> | null => {
-  const candidateElements = getAllHoveredElementAtPoint(
+  const candidateElements: {
-    point,
+    element: NonDeleted<ExcalidrawBindableElement>;
-    elements,
+    distance: number;
-    elementsMap,
+  }[] = [];
-    tolerance,
-  );
-
-  if (!candidateElements || candidateElements.length === 0) {
-    return null;
-  }
-
-  if (candidateElements.length === 1) {
-    return candidateElements[0];
-  }
-
-  // Prefer smaller shapes
-  return candidateElements
-    .sort(
-      (a, b) => b.width ** 2 + b.height ** 2 - (a.width ** 2 + a.height ** 2),
-    )
-    .pop() as NonDeleted<ExcalidrawBindableElement>;
-};
-
-export const getHoveredElementForFocusPoint = (
-  point: GlobalPoint,
-  arrow: ExcalidrawArrowElement,
-  elements: readonly Ordered<NonDeletedExcalidrawElement>[],
-  elementsMap: NonDeletedSceneElementsMap,
-  tolerance?: number,
-): ExcalidrawBindableElement | null => {
-  const candidateElements: NonDeleted<ExcalidrawBindableElement>[] = [];
-  // We need to to hit testing from front (end of the array) to back (beginning of the array)
-  // because array is ordered from lower z-index to highest and we want element z-index
-  // with higher z-index
   for (let index = elements.length - 1; index >= 0; --index) {
     const element = elements[index];
 
-    invariant(
+    if (!isBindableElement(element, false)) {
-      !element.isDeleted,
+      continue;
-      "Elements in the function parameter for getAllElementsAtPositionForBinding() should not contain deleted elements",
+    }
-    );
 
-    if (
+    const distance = borderDistance(element, point, elementsMap, tolerance);
-      isBindableElement(element, false) &&
+    const bindingGap = getBindingGap(element, arrow);
-      bindingBorderTest(element, point, elementsMap, tolerance)
+
-    ) {
+    if (distance > -(tolerance ?? bindingGap)) {
-      candidateElements.push(element);
+      candidateElements.push({ element, distance });
     }
   }
 
-  if (!candidateElements || candidateElements.length === 0) {
+  if (candidateElements.length === 0) {
     return null;
   }
 
   if (candidateElements.length === 1) {
-    return candidateElements[0];
+    return candidateElements[0].element;
   }
 
-  const distanceFilteredCandidateElements = candidateElements
+  return candidateElements
-    // Resolve by distance
+    .sort((a, b) => a.distance - b.distance)
-    .filter(
+    .map((c) => c.element)[0];
-      (el) =>
-        distanceToElement(el, elementsMap, point) <= getBindingGap(el, arrow) ||
-        isPointInElement(point, el, elementsMap),
-    );
-
-  if (distanceFilteredCandidateElements.length === 0) {
-    return null;
-  }
-
-  return distanceFilteredCandidateElements[0] as NonDeleted<ExcalidrawBindableElement>;
 };
 
 /**

packages/element/src/elbowArrow.ts

@@ -318,6 +318,7 @@ const handleSegmentRelease = (
     ...rest
   } = getElbowArrowData(
     {
+      ...arrow,
       x,
       y,
       startBinding,
@@ -1041,6 +1042,7 @@ export const updateElbowArrowPoints = (
     ...rest
   } = getElbowArrowData(
     {
+      ...arrow,
       x: arrow.x,
       y: arrow.y,
       startBinding,
@@ -1190,15 +1192,7 @@ export const updateElbowArrowPoints = (
  * - hoveredEndElement: The element being hovered over at the end point.
  */
 const getElbowArrowData = (
-  arrow: {
+  arrow: ExcalidrawElbowArrowElement,
-    x: number;
-    y: number;
-    startBinding: FixedPointBinding | null;
-    endBinding: FixedPointBinding | null;
-    startArrowhead: Arrowhead | null;
-    endArrowhead: Arrowhead | null;
-    points: readonly LocalPoint[];
-  },
   elementsMap: NonDeletedSceneElementsMap,
   nextPoints: readonly LocalPoint[],
   options?: {
@@ -1223,6 +1217,7 @@ const getElbowArrowData = (
     const elements = Array.from(elementsMap.values());
     hoveredStartElement =
       getHoveredElement(
+        arrow,
         origStartGlobalPoint,
         elementsMap,
         elements,
@@ -1230,6 +1225,7 @@ const getElbowArrowData = (
       ) || null;
     hoveredEndElement =
       getHoveredElement(
+        arrow,
         origEndGlobalPoint,
         elementsMap,
         elements,
@@ -2279,12 +2275,14 @@ const getBindPointHeading = (
   );
 
 const getHoveredElement = (
+  arrow: ExcalidrawElbowArrowElement,
   origPoint: GlobalPoint,
   elementsMap: NonDeletedSceneElementsMap,
   elements: readonly Ordered<NonDeletedExcalidrawElement>[],
   zoom?: AppState["zoom"],
 ) => {
   return getHoveredElementForBinding(
+    arrow,
     origPoint,
     elements,
     elementsMap,

packages/element/src/zindex.ts

@@ -160,7 +160,7 @@ export const moveArrowAboveBindable = (
 ): readonly OrderedExcalidrawElement[] => {
   const hoveredElement = hit
     ? hit
-    : getHoveredElementForBinding(point, elements, elementsMap);
+    : getHoveredElementForBinding(arrow, point, elements, elementsMap);
 
   if (!hoveredElement) {
     return elements;

packages/excalidraw/components/App.tsx

@@ -940,6 +940,9 @@ class App extends React.Component<AppProps, AppState> {
       );
       const elementsMap = this.scene.getNonDeletedElementsMap();
       const hoveredElement = getHoveredElementForBinding(
+        elementsMap.get(
+          this.state.selectedLinearElement.elementId,
+        ) as ExcalidrawArrowElement,
         pointFrom<GlobalPoint>(
           this.lastPointerMoveCoords.x,
           this.lastPointerMoveCoords.y,
@@ -1076,6 +1079,7 @@ class App extends React.Component<AppProps, AppState> {
 
       const { x, y } = this.lastPointerMoveCoords;
       const hoveredElement = getHoveredElementForBinding(
+        arrow,
         pointFrom<GlobalPoint>(x, y),
         this.scene.getNonDeletedElements(),
         this.scene.getNonDeletedElementsMap(),
@@ -5381,12 +5385,6 @@ class App extends React.Component<AppProps, AppState> {
           this.state,
         );
 
-        const hoveredElement = getHoveredElementForBinding(
-          pointFrom<GlobalPoint>(scenePointer.x, scenePointer.y),
-          this.scene.getNonDeletedElements(),
-          this.scene.getNonDeletedElementsMap(),
-        );
-
         if (this.state.selectedLinearElement) {
           const element = LinearElementEditor.getElement(
             this.state.selectedLinearElement.elementId,
@@ -5394,6 +5392,13 @@ class App extends React.Component<AppProps, AppState> {
           );
 
           if (isBindingElement(element)) {
+            const hoveredElement = getHoveredElementForBinding(
+              element,
+              pointFrom<GlobalPoint>(scenePointer.x, scenePointer.y),
+              this.scene.getNonDeletedElements(),
+              this.scene.getNonDeletedElementsMap(),
+            );
+
             this.handleDelayedBindModeChange(element, hoveredElement);
           }
         }
@@ -7080,7 +7085,8 @@ class App extends React.Component<AppProps, AppState> {
         );
         const elementsMap = this.scene.getNonDeletedElementsMap();
         const hoveredElement = getHoveredElementForBinding(
-          globalPoint,
+          { elbowed: this.state.currentItemArrowType === ARROW_TYPE.elbow },
+          pointFrom<GlobalPoint>(scenePointerX, scenePointerY),
           this.scene.getNonDeletedElements(),
           elementsMap,
           maxBindingDistance_simple(this.state.zoom),
@@ -7119,6 +7125,7 @@ class App extends React.Component<AppProps, AppState> {
           isArrowElement(this.state.newElement) &&
           isBindingEnabled(this.state) &&
           getHoveredElementForBinding(
+            this.state.newElement,
             pointFrom<GlobalPoint>(scenePointerX, scenePointerY),
             this.scene.getNonDeletedElements(),
             this.scene.getNonDeletedElementsMap(),
@@ -7237,6 +7244,7 @@ class App extends React.Component<AppProps, AppState> {
 
         if (isSimpleArrow(multiElement)) {
           const hoveredElement = getHoveredElementForBinding(
+            multiElement,
             pointFrom<GlobalPoint>(scenePointerX, scenePointerY),
             this.scene.getNonDeletedElements(),
             elementsMap,
@@ -7269,6 +7277,7 @@ class App extends React.Component<AppProps, AppState> {
 
     if (this.state.activeTool.type === "arrow") {
       const hit = getHoveredElementForBinding(
+        { elbowed: this.state.currentItemArrowType === ARROW_TYPE.elbow },
         pointFrom<GlobalPoint>(scenePointerX, scenePointerY),
         this.scene.getNonDeletedElements(),
         this.scene.getNonDeletedElementsMap(),
@@ -9271,6 +9280,7 @@ class App extends React.Component<AppProps, AppState> {
       const hoveredElementForBinding =
         isBindingEnabled(this.state) &&
         getHoveredElementForBinding(
+          { elbowed: isElbowArrow(multiElement) },
           pointFrom<GlobalPoint>(
             this.lastPointerMoveCoords?.x ??
               rx + multiElement.points[multiElement.points.length - 1][0],
@@ -9393,6 +9403,7 @@ class App extends React.Component<AppProps, AppState> {
       const elementsMap = this.scene.getNonDeletedElementsMap();
       const boundElement = isBindingEnabled(this.state)
         ? getHoveredElementForBinding(
+            { elbowed: this.state.currentItemArrowType === ARROW_TYPE.elbow },
             point,
             this.scene.getNonDeletedElements(),
             elementsMap,
@@ -9876,6 +9887,7 @@ class App extends React.Component<AppProps, AppState> {
 
           if (isBindingElement(element)) {
             const hoveredElement = getHoveredElementForBinding(
+              element,
               pointFrom<GlobalPoint>(pointerCoords.x, pointerCoords.y),
               this.scene.getNonDeletedElements(),
               elementsMap,