From d587b8a3de6ce8b4ae1bd7578210977ab99fddb7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=A1rk=20Tolm=C3=A1cs?= Date: Mon, 10 Mar 2025 16:25:33 +0100 Subject: [PATCH] fix: Do not rebind undragged elbow arrow endpoint (#9191) --- packages/excalidraw/element/binding.ts | 29 +++-- packages/excalidraw/element/elbowArrow.ts | 109 +++++++++++-------- packages/excalidraw/element/flowchart.ts | 3 +- packages/excalidraw/element/mutateElement.ts | 4 +- packages/excalidraw/tests/resize.test.tsx | 5 +- 5 files changed, 90 insertions(+), 60 deletions(-) diff --git a/packages/excalidraw/element/binding.ts b/packages/excalidraw/element/binding.ts index 8de9254fa..e716b860f 100644 --- a/packages/excalidraw/element/binding.ts +++ b/packages/excalidraw/element/binding.ts @@ -73,6 +73,8 @@ import { vectorCross, pointsEqual, lineSegmentIntersectionPoints, + round, + PRECISION, } from "@excalidraw/math"; import { intersectElementWithLineSegment } from "./collision"; import { distanceToBindableElement } from "./distance"; @@ -272,14 +274,16 @@ const getBindingStrategyForDraggingArrowEndpoints = ( zoom, ) : null // If binding is disabled and start is dragged, break all binds - : // We have to update the focus and gap of the binding, so let's rebind + : !isElbowArrow(selectedElement) + ? // We have to update the focus and gap of the binding, so let's rebind getElligibleElementForBindingElement( selectedElement, "start", elementsMap, elements, zoom, - ); + ) + : "keep"; const end = endDragged ? isBindingEnabled ? getElligibleElementForBindingElement( @@ -290,14 +294,16 @@ const getBindingStrategyForDraggingArrowEndpoints = ( zoom, ) : null // If binding is disabled and end is dragged, break all binds - : // We have to update the focus and gap of the binding, so let's rebind + : !isElbowArrow(selectedElement) + ? // We have to update the focus and gap of the binding, so let's rebind getElligibleElementForBindingElement( selectedElement, "end", elementsMap, elements, zoom, - ); + ) + : "keep"; return [start, end]; }; @@ -309,6 +315,11 @@ const getBindingStrategyForDraggingArrowOrJoints = ( isBindingEnabled: boolean, zoom?: AppState["zoom"], ): (NonDeleted | null | "keep")[] => { + // Elbow arrows don't bind when dragged as a whole + if (isElbowArrow(selectedElement)) { + return ["keep", "keep"]; + } + const [startIsClose, endIsClose] = getOriginalBindingsIfStillCloseToArrowEnds( selectedElement, elementsMap, @@ -945,13 +956,17 @@ export const bindPointToSnapToElementOutline = ( const currentDistance = pointDistance(p, center); const fullDistance = Math.max( pointDistance(intersection ?? p, center), - 1e-5, + PRECISION, ); - const ratio = currentDistance / fullDistance; + const ratio = round(currentDistance / fullDistance, 6); switch (true) { case ratio > 0.9: - if (currentDistance - fullDistance > FIXED_BINDING_DISTANCE) { + if ( + currentDistance - fullDistance > FIXED_BINDING_DISTANCE || + // Too close to determine vector from intersection to p + pointDistanceSq(p, intersection) < PRECISION + ) { return p; } diff --git a/packages/excalidraw/element/elbowArrow.ts b/packages/excalidraw/element/elbowArrow.ts index 48ddeee2c..ce8c3d5a6 100644 --- a/packages/excalidraw/element/elbowArrow.ts +++ b/packages/excalidraw/element/elbowArrow.ts @@ -15,7 +15,7 @@ import { import BinaryHeap from "../binaryheap"; import { getSizeFromPoints } from "../points"; import { aabbForElement, pointInsideBounds } from "../shapes"; -import { invariant, isAnyTrue, toBrandedType, tupleToCoors } from "../utils"; +import { invariant, isAnyTrue, tupleToCoors } from "../utils"; import type { AppState } from "../types"; import { bindPointToSnapToElementOutline, @@ -52,6 +52,7 @@ import type { ExcalidrawBindableElement, FixedPointBinding, FixedSegment, + NonDeletedExcalidrawElement, } from "./types"; import { distanceToBindableElement } from "./distance"; @@ -101,7 +102,7 @@ export const BASE_PADDING = 40; const handleSegmentRenormalization = ( arrow: ExcalidrawElbowArrowElement, - elementsMap: NonDeletedSceneElementsMap | SceneElementsMap, + elementsMap: NonDeletedSceneElementsMap, ) => { const nextFixedSegments: FixedSegment[] | null = arrow.fixedSegments ? arrow.fixedSegments.slice() @@ -234,6 +235,16 @@ const handleSegmentRenormalization = ( nextPoints.map((p) => pointFrom(p[0] - arrow.x, p[1] - arrow.y), ), + arrow.startBinding && + getBindableElementForId( + arrow.startBinding.elementId, + elementsMap, + ), + arrow.endBinding && + getBindableElementForId( + arrow.endBinding.elementId, + elementsMap, + ), ), ) ?? [], ), @@ -271,7 +282,7 @@ const handleSegmentRenormalization = ( const handleSegmentRelease = ( arrow: ExcalidrawElbowArrowElement, fixedSegments: readonly FixedSegment[], - elementsMap: NonDeletedSceneElementsMap | SceneElementsMap, + elementsMap: NonDeletedSceneElementsMap, ) => { const newFixedSegmentIndices = fixedSegments.map((segment) => segment.index); const oldFixedSegmentIndices = @@ -295,6 +306,8 @@ const handleSegmentRelease = ( // We need to render a sub-arrow path to restore deleted segments const x = arrow.x + (prevSegment ? prevSegment.end[0] : 0); const y = arrow.y + (prevSegment ? prevSegment.end[1] : 0); + const startBinding = prevSegment ? null : arrow.startBinding; + const endBinding = nextSegment ? null : arrow.endBinding; const { startHeading, endHeading, @@ -307,10 +320,11 @@ const handleSegmentRelease = ( { x, y, - startBinding: prevSegment ? null : arrow.startBinding, - endBinding: nextSegment ? null : arrow.endBinding, + startBinding, + endBinding, startArrowhead: null, endArrowhead: null, + points: arrow.points, }, elementsMap, [ @@ -324,6 +338,9 @@ const handleSegmentRelease = ( y, ), ], + startBinding && + getBindableElementForId(startBinding.elementId, elementsMap), + endBinding && getBindableElementForId(endBinding.elementId, elementsMap), { isDragging: false }, ); @@ -870,7 +887,7 @@ const MAX_POS = 1e6; */ export const updateElbowArrowPoints = ( arrow: Readonly, - elementsMap: NonDeletedSceneElementsMap | SceneElementsMap, + elementsMap: NonDeletedSceneElementsMap, updates: { points?: readonly LocalPoint[]; fixedSegments?: FixedSegment[] | null; @@ -986,8 +1003,11 @@ export const updateElbowArrowPoints = ( typeof updates.endBinding !== "undefined" ? updates.endBinding : arrow.endBinding; - const startElement = startBinding && elementsMap.get(startBinding.elementId); - const endElement = endBinding && elementsMap.get(endBinding.elementId); + const startElement = + startBinding && + getBindableElementForId(startBinding.elementId, elementsMap); + const endElement = + endBinding && getBindableElementForId(endBinding.elementId, elementsMap); if ( (elementsMap.size === 0 && validateElbowPoints(updatedPoints)) || startElement?.id !== startBinding?.elementId || @@ -1019,9 +1039,12 @@ export const updateElbowArrowPoints = ( endBinding, startArrowhead: arrow.startArrowhead, endArrowhead: arrow.endArrowhead, + points: arrow.points, }, elementsMap, updatedPoints, + startElement, + endElement, options, ); @@ -1155,9 +1178,12 @@ const getElbowArrowData = ( endBinding: FixedPointBinding | null; startArrowhead: Arrowhead | null; endArrowhead: Arrowhead | null; + points: readonly LocalPoint[]; }, - elementsMap: NonDeletedSceneElementsMap | SceneElementsMap, + elementsMap: NonDeletedSceneElementsMap, nextPoints: readonly LocalPoint[], + startElement: ExcalidrawBindableElement | null, + endElement: ExcalidrawBindableElement | null, options?: { isDragging?: boolean; zoom?: AppState["zoom"]; @@ -1171,20 +1197,27 @@ const getElbowArrowData = ( LocalPoint, GlobalPoint >(nextPoints[nextPoints.length - 1], vector(arrow.x, arrow.y)); - const startElement = - arrow.startBinding && - getBindableElementForId(arrow.startBinding.elementId, elementsMap); - const endElement = - arrow.endBinding && - getBindableElementForId(arrow.endBinding.elementId, elementsMap); - const [hoveredStartElement, hoveredEndElement] = options?.isDragging - ? getHoveredElements( + + let hoveredStartElement = startElement; + let hoveredEndElement = endElement; + if (options?.isDragging) { + const elements = Array.from(elementsMap.values()); + hoveredStartElement = + getHoveredElement( origStartGlobalPoint, + elementsMap, + elements, + options?.zoom, + ) || startElement; + hoveredEndElement = + getHoveredElement( origEndGlobalPoint, elementsMap, + elements, options?.zoom, - ) - : [startElement, endElement]; + ) || endElement; + } + const startGlobalPoint = getGlobalPoint( { ...arrow, @@ -2214,36 +2247,20 @@ const getBindPointHeading = ( origPoint, ); -const getHoveredElements = ( - origStartGlobalPoint: GlobalPoint, - origEndGlobalPoint: GlobalPoint, - elementsMap: NonDeletedSceneElementsMap | SceneElementsMap, +const getHoveredElement = ( + origPoint: GlobalPoint, + elementsMap: NonDeletedSceneElementsMap, + elements: readonly NonDeletedExcalidrawElement[], zoom?: AppState["zoom"], ) => { - // TODO: Might be a performance bottleneck and the Map type - // remembers the insertion order anyway... - const nonDeletedSceneElementsMap = toBrandedType( - new Map([...elementsMap].filter((el) => !el[1].isDeleted)), + return getHoveredElementForBinding( + tupleToCoors(origPoint), + elements, + elementsMap, + zoom, + true, + true, ); - const elements = Array.from(elementsMap.values()); - return [ - getHoveredElementForBinding( - tupleToCoors(origStartGlobalPoint), - elements, - nonDeletedSceneElementsMap, - zoom, - true, - true, - ), - getHoveredElementForBinding( - tupleToCoors(origEndGlobalPoint), - elements, - nonDeletedSceneElementsMap, - zoom, - true, - true, - ), - ]; }; const gridAddressesEqual = (a: GridAddress, b: GridAddress): boolean => diff --git a/packages/excalidraw/element/flowchart.ts b/packages/excalidraw/element/flowchart.ts index 02ee1ff29..09f006dd0 100644 --- a/packages/excalidraw/element/flowchart.ts +++ b/packages/excalidraw/element/flowchart.ts @@ -10,7 +10,6 @@ import { import { bindLinearElement } from "./binding"; import { LinearElementEditor } from "./linearElementEditor"; import { newArrowElement, newElement } from "./newElement"; -import type { SceneElementsMap } from "./types"; import { type ElementsMap, type ExcalidrawBindableElement, @@ -472,7 +471,7 @@ const createBindingArrow = ( const update = updateElbowArrowPoints( bindingArrow, - toBrandedType( + toBrandedType( new Map([ ...elementsMap.entries(), [startBindingElement.id, startBindingElement], diff --git a/packages/excalidraw/element/mutateElement.ts b/packages/excalidraw/element/mutateElement.ts index b64366be9..cfff5c826 100644 --- a/packages/excalidraw/element/mutateElement.ts +++ b/packages/excalidraw/element/mutateElement.ts @@ -1,4 +1,4 @@ -import type { ExcalidrawElement, SceneElementsMap } from "./types"; +import type { ExcalidrawElement, NonDeletedSceneElementsMap } from "./types"; import Scene from "../scene/Scene"; import { getSizeFromPoints } from "../points"; import { randomInteger } from "../random"; @@ -44,7 +44,7 @@ export const mutateElement = >( typeof startBinding !== "undefined" || typeof endBinding !== "undefined") // manual binding to element ) { - const elementsMap = toBrandedType( + const elementsMap = toBrandedType( Scene.getScene(element)?.getNonDeletedElementsMap() ?? new Map(), ); diff --git a/packages/excalidraw/tests/resize.test.tsx b/packages/excalidraw/tests/resize.test.tsx index a12d31048..3ebd8cf81 100644 --- a/packages/excalidraw/tests/resize.test.tsx +++ b/packages/excalidraw/tests/resize.test.tsx @@ -533,9 +533,8 @@ describe("arrow element", () => { expect(arrow.startBinding?.fixedPoint?.[1]).toBeCloseTo(0.75); UI.resize([rectangle, arrow], "nw", [300, 350]); - - expect(arrow.startBinding?.fixedPoint?.[0]).toBeCloseTo(-0.13); - expect(arrow.startBinding?.fixedPoint?.[1]).toBeCloseTo(0.11); + expect(arrow.startBinding?.fixedPoint?.[0]).toBeCloseTo(0); + expect(arrow.startBinding?.fixedPoint?.[1]).toBeCloseTo(0.25); }); });