diff --git a/packages/element/src/binding.ts b/packages/element/src/binding.ts index 9d97801f2e..fdfbb08235 100644 --- a/packages/element/src/binding.ts +++ b/packages/element/src/binding.ts @@ -37,7 +37,7 @@ import { getCenterForBounds, getElementBounds, } from "./bounds"; -import { intersectElementWithLineSegment } from "./collision"; +import { intersectElementWithLineSegment, isPointInElement } from "./collision"; import { distanceToElement } from "./distance"; import { headingForPointFromElement, @@ -127,6 +127,9 @@ export const bindOrUnbindLinearElement = ( endBindingElement: ExcalidrawBindableElement | null | "keep", scene: Scene, ): void => { + const bothEndBoundToTheSameElement = + linearElement.startBinding?.elementId === + linearElement.endBinding?.elementId && !!linearElement.startBinding; const elementsMap = scene.getNonDeletedElementsMap(); const boundToElementIds: Set = new Set(); const unboundFromElementIds: Set = new Set(); @@ -151,18 +154,20 @@ export const bindOrUnbindLinearElement = ( elementsMap, ); - const onlyUnbound = Array.from(unboundFromElementIds).filter( - (id) => !boundToElementIds.has(id), - ); + if (!bothEndBoundToTheSameElement) { + const onlyUnbound = Array.from(unboundFromElementIds).filter( + (id) => !boundToElementIds.has(id), + ); - getNonDeletedElements(scene, onlyUnbound).forEach((element) => { - scene.mutateElement(element, { - boundElements: element.boundElements?.filter( - (element) => - element.type !== "arrow" || element.id !== linearElement.id, - ), + getNonDeletedElements(scene, onlyUnbound).forEach((element) => { + scene.mutateElement(element, { + boundElements: element.boundElements?.filter( + (element) => + element.type !== "arrow" || element.id !== linearElement.id, + ), + }); }); - }); + } }; const bindOrUnbindLinearElementEdge = ( @@ -203,6 +208,7 @@ const bindOrUnbindLinearElementEdge = ( linearElement, bindableElement, startOrEnd, + elementsMap, ) : startOrEnd === "start" || otherEdgeBindableElement.id !== bindableElement.id) @@ -459,6 +465,7 @@ export const maybeBindLinearElement = ( linearElement, hoveredElement, "end", + elementsMap, ) ) { bindLinearElement(linearElement, hoveredElement, "end", scene); @@ -487,29 +494,64 @@ export const bindLinearElement = ( return; } - let binding: PointBinding | FixedPointBinding = { - elementId: hoveredElement.id, - ...normalizePointBinding( - calculateFocusAndGap( - linearElement, - hoveredElement, - startOrEnd, - scene.getNonDeletedElementsMap(), - ), - hoveredElement, - ), - }; + const elementsMap = scene.getNonDeletedElementsMap(); + let binding: PointBinding | FixedPointBinding; if (isElbowArrow(linearElement)) { binding = { - ...binding, + elementId: hoveredElement.id, + ...normalizePointBinding( + calculateFocusAndGap( + linearElement, + hoveredElement, + startOrEnd, + elementsMap, + ), + hoveredElement, + ), ...calculateFixedPointForElbowArrowBinding( linearElement, hoveredElement, startOrEnd, - scene.getNonDeletedElementsMap(), + elementsMap, ), }; + } else { + // For non-elbow arrows, check if the endpoint is inside the shape + const edgePoint = LinearElementEditor.getPointAtIndexGlobalCoordinates( + linearElement, + startOrEnd === "start" ? 0 : -1, + elementsMap, + ); + + if (isPointInElement(edgePoint, hoveredElement, elementsMap)) { + // Use FixedPoint binding when the arrow endpoint is inside the shape + binding = { + elementId: hoveredElement.id, + focus: 0, + gap: 0, + ...calculateFixedPointForNonElbowArrowBinding( + linearElement, + hoveredElement, + startOrEnd, + elementsMap, + ), + }; + } else { + // Use traditional focus/gap binding when the endpoint is outside the shape + binding = { + elementId: hoveredElement.id, + ...normalizePointBinding( + calculateFocusAndGap( + linearElement, + hoveredElement, + startOrEnd, + elementsMap, + ), + hoveredElement, + ), + }; + } } scene.mutateElement(linearElement, { @@ -532,14 +574,36 @@ const isLinearElementSimpleAndAlreadyBoundOnOppositeEdge = ( linearElement: NonDeleted, bindableElement: ExcalidrawBindableElement, startOrEnd: "start" | "end", + elementsMap: ElementsMap, ): boolean => { const otherBinding = linearElement[startOrEnd === "start" ? "endBinding" : "startBinding"]; - return isLinearElementSimpleAndAlreadyBound( - linearElement, - otherBinding?.elementId, - bindableElement, - ); + + // Only prevent binding if opposite end is bound to the same element + if ( + otherBinding?.elementId !== bindableElement.id || + !isLinearElementSimple(linearElement) + ) { + return false; + } + + // For non-elbow arrows, allow FixedPoint binding even when both ends bind to the same element + if (!isElbowArrow(linearElement)) { + const currentEndPoint = + LinearElementEditor.getPointAtIndexGlobalCoordinates( + linearElement, + startOrEnd === "start" ? 0 : -1, + elementsMap, + ); + + // If current end would use FixedPoint binding, allow it + if (isPointInElement(currentEndPoint, bindableElement, elementsMap)) { + return false; + } + } + + // Prevent traditional focus/gap binding when both ends would bind to the same element + return true; }; export const isLinearElementSimpleAndAlreadyBound = ( @@ -1254,15 +1318,22 @@ const updateBoundPoint = ( const direction = startOrEnd === "startBinding" ? -1 : 1; const edgePointIndex = direction === -1 ? 0 : linearElement.points.length - 1; - if (isElbowArrow(linearElement) && isFixedPointBinding(binding)) { + if (isFixedPointBinding(binding)) { const fixedPoint = normalizeFixedPoint(binding.fixedPoint) ?? - calculateFixedPointForElbowArrowBinding( - linearElement, - bindableElement, - startOrEnd === "startBinding" ? "start" : "end", - elementsMap, - ).fixedPoint; + (isElbowArrow(linearElement) + ? calculateFixedPointForElbowArrowBinding( + linearElement, + bindableElement, + startOrEnd === "startBinding" ? "start" : "end", + elementsMap, + ).fixedPoint + : calculateFixedPointForNonElbowArrowBinding( + linearElement, + bindableElement, + startOrEnd === "startBinding" ? "start" : "end", + elementsMap, + ).fixedPoint); const globalMidPoint = elementCenterPoint(bindableElement, elementsMap); const global = pointFrom( bindableElement.x + fixedPoint[0] * bindableElement.width, @@ -1401,6 +1472,42 @@ export const calculateFixedPointForElbowArrowBinding = ( }; }; +export const calculateFixedPointForNonElbowArrowBinding = ( + linearElement: NonDeleted, + hoveredElement: ExcalidrawBindableElement, + startOrEnd: "start" | "end", + elementsMap: ElementsMap, +): { fixedPoint: FixedPoint } => { + const edgePoint = LinearElementEditor.getPointAtIndexGlobalCoordinates( + linearElement, + startOrEnd === "start" ? 0 : -1, + elementsMap, + ); + + // Convert the global point to element-local coordinates + const elementCenter = pointFrom( + hoveredElement.x + hoveredElement.width / 2, + hoveredElement.y + hoveredElement.height / 2, + ); + + // Rotate the point to account for element rotation + const nonRotatedPoint = pointRotateRads( + edgePoint, + elementCenter, + -hoveredElement.angle as Radians, + ); + + // Calculate the ratio relative to the element's bounds + const fixedPointX = + (nonRotatedPoint[0] - hoveredElement.x) / hoveredElement.width; + const fixedPointY = + (nonRotatedPoint[1] - hoveredElement.y) / hoveredElement.height; + + return { + fixedPoint: normalizeFixedPoint([fixedPointX, fixedPointY]), + }; +}; + const maybeCalculateNewGapWhenScaling = ( changedElement: ExcalidrawBindableElement, currentBinding: PointBinding | null | undefined, diff --git a/packages/excalidraw/data/__snapshots__/transform.test.ts.snap b/packages/excalidraw/data/__snapshots__/transform.test.ts.snap index f00a51817d..bd9c4f9a18 100644 --- a/packages/excalidraw/data/__snapshots__/transform.test.ts.snap +++ b/packages/excalidraw/data/__snapshots__/transform.test.ts.snap @@ -88,8 +88,12 @@ exports[`Test Transform > Test arrow bindings > should bind arrows to existing s "endArrowhead": "arrow", "endBinding": { "elementId": "ellipse-1", - "focus": -0.007519379844961235, - "gap": 11.562288374879595, + "fixedPoint": [ + 0.04, + 0.4633333333333333, + ], + "focus": 0, + "gap": 0, }, "fillStyle": "solid", "frameId": null, @@ -174,8 +178,12 @@ exports[`Test Transform > Test arrow bindings > should bind arrows to existing s "startArrowhead": null, "startBinding": { "elementId": "diamond-1", + "fixedPoint": [ + 0.9357142857142857, + 0.5001, + ], "focus": 0, - "gap": 4.535423522449215, + "gap": 0, }, "strokeColor": "#e67700", "strokeStyle": "solid", @@ -1539,8 +1547,12 @@ exports[`Test Transform > should transform the elements correctly when linear el "endArrowhead": "arrow", "endBinding": { "elementId": "B", + "fixedPoint": [ + 0.46387050630528887, + 0.48466257668711654, + ], "focus": 0, - "gap": 32, + "gap": 0, }, "fillStyle": "solid", "frameId": null, diff --git a/packages/excalidraw/data/restore.ts b/packages/excalidraw/data/restore.ts index a609c0a0eb..bc4736b729 100644 --- a/packages/excalidraw/data/restore.ts +++ b/packages/excalidraw/data/restore.ts @@ -28,9 +28,14 @@ import { LinearElementEditor } from "@excalidraw/element"; import { bumpVersion } from "@excalidraw/element"; import { getContainerElement } from "@excalidraw/element"; import { detectLineHeight } from "@excalidraw/element"; +import { + isPointInElement, + calculateFixedPointForNonElbowArrowBinding, +} from "@excalidraw/element"; import { isArrowBoundToElement, isArrowElement, + isBindableElement, isElbowArrow, isFixedPointBinding, isLinearElement, @@ -519,6 +524,87 @@ const repairFrameMembership = ( } }; +/** + * Migrates old PointBinding to FixedPointBinding for non-elbow arrows + * when arrow endpoints are inside bindable shapes. + * + * NOTE mutates element. + */ +const migratePointBindingToFixedPoint = ( + element: Mutable, + elementsMap: Map>, +) => { + if (!isArrowElement(element) || isElbowArrow(element)) { + return; + } + + let shouldUpdateElement = false; + let newStartBinding: FixedPointBinding | PointBinding | null = + element.startBinding; + let newEndBinding: FixedPointBinding | PointBinding | null = + element.endBinding; + + // Check start binding + if (element.startBinding && !isFixedPointBinding(element.startBinding)) { + const boundElement = elementsMap.get(element.startBinding.elementId); + if (boundElement && isBindableElement(boundElement)) { + const edgePoint = LinearElementEditor.getPointAtIndexGlobalCoordinates( + element, + 0, + elementsMap, + ); + if (isPointInElement(edgePoint, boundElement, elementsMap)) { + const { fixedPoint } = calculateFixedPointForNonElbowArrowBinding( + element, + boundElement, + "start", + elementsMap, + ); + newStartBinding = { + elementId: element.startBinding.elementId, + focus: 0, + gap: 0, + fixedPoint, + }; + shouldUpdateElement = true; + } + } + } + + // Check end binding + if (element.endBinding && !isFixedPointBinding(element.endBinding)) { + const boundElement = elementsMap.get(element.endBinding.elementId); + if (boundElement && isBindableElement(boundElement)) { + const edgePoint = LinearElementEditor.getPointAtIndexGlobalCoordinates( + element, + -1, + elementsMap, + ); + if (isPointInElement(edgePoint, boundElement, elementsMap)) { + const { fixedPoint } = calculateFixedPointForNonElbowArrowBinding( + element, + boundElement, + "end", + elementsMap, + ); + newEndBinding = { + elementId: element.endBinding.elementId, + focus: 0, + gap: 0, + fixedPoint, + }; + shouldUpdateElement = true; + } + } + } + + if (shouldUpdateElement) { + (element as Mutable).startBinding = + newStartBinding; + (element as Mutable).endBinding = newEndBinding; + } +}; + export const restoreElements = ( elements: ImportedDataState["elements"], /** NOTE doesn't serve for reconciliation */ @@ -598,6 +684,9 @@ export const restoreElements = ( (element as Mutable).endBinding = null; } } + + // Migrate old PointBinding to FixedPointBinding for non-elbow arrows + migratePointBindingToFixedPoint(element, restoredElementsMap); } // NOTE (mtolmacs): Temporary fix for extremely large arrows diff --git a/packages/excalidraw/tests/__snapshots__/history.test.tsx.snap b/packages/excalidraw/tests/__snapshots__/history.test.tsx.snap index 077897575f..4b8ada075f 100644 --- a/packages/excalidraw/tests/__snapshots__/history.test.tsx.snap +++ b/packages/excalidraw/tests/__snapshots__/history.test.tsx.snap @@ -195,7 +195,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "fillStyle": "solid", "frameId": null, "groupIds": [], - "height": "99.19972", + "height": 150, "id": "id4", "index": "a2", "isDeleted": false, @@ -209,8 +209,8 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl 0, ], [ - "98.40611", - "99.19972", + "124.00500", + 150, ], ], "roughness": 1, @@ -225,7 +225,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "type": "arrow", "updated": 1, "version": 35, - "width": "98.40611", + "width": "124.00500", "x": 1, "y": 0, } @@ -332,15 +332,15 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "focus": 0, "gap": 1, }, - "height": "68.58402", + "height": "104.34908", "points": [ [ 0, 0, ], [ - 98, - "68.58402", + "124.00500", + "104.34908", ], ], "startBinding": { @@ -356,7 +356,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "focus": "-0.02000", "gap": 1, }, - "height": "0.00656", + "height": "0.00849", "points": [ [ 0, @@ -364,7 +364,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl ], [ "98.00000", - "-0.00656", + "-0.00849", ], ], "startBinding": { @@ -415,15 +415,15 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl }, "id4": { "deleted": { - "height": "99.19972", + "height": 150, "points": [ [ 0, 0, ], [ - "98.40611", - "99.19972", + "124.00500", + 150, ], ], "startBinding": null, @@ -431,15 +431,15 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "y": 0, }, "inserted": { - "height": "68.58402", + "height": "104.34908", "points": [ [ 0, 0, ], [ - 98, - "68.58402", + "124.00500", + "104.34908", ], ], "startBinding": { @@ -448,7 +448,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "gap": 1, }, "version": 33, - "y": "35.82151", + "y": "45.65092", }, }, }, @@ -1228,7 +1228,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "fillStyle": "solid", "frameId": null, "groupIds": [], - "height": "1.36342", + "height": "49.99000", "id": "id4", "index": "Zz", "isDeleted": false, @@ -1242,8 +1242,8 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl 0, ], [ - 98, - "1.36342", + "150.01000", + "49.99000", ], ], "roughness": 1, @@ -1264,9 +1264,9 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "type": "arrow", "updated": 1, "version": 11, - "width": 98, - "x": 1, - "y": 0, + "width": "150.01000", + "x": 0, + "y": "0.01000", } `; @@ -1591,7 +1591,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "fillStyle": "solid", "frameId": null, "groupIds": [], - "height": "1.36342", + "height": "49.99000", "id": "id5", "index": "a0", "isDeleted": false, @@ -1605,8 +1605,8 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl 0, ], [ - 98, - "1.36342", + "249.99000", + "-49.99000", ], ], "roughness": 1, @@ -1627,9 +1627,9 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "type": "arrow", "updated": 1, "version": 11, - "width": 98, - "x": 1, - "y": 0, + "width": "249.99000", + "x": "-49.99000", + "y": 50, } `; @@ -1741,7 +1741,7 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "fillStyle": "solid", "frameId": null, "groupIds": [], - "height": "1.36342", + "height": "49.99000", "index": "a0", "isDeleted": false, "lastCommittedPoint": null, @@ -1754,8 +1754,8 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl 0, ], [ - 98, - "1.36342", + "249.99000", + "-49.99000", ], ], "roughness": 1, @@ -1775,9 +1775,9 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl "strokeWidth": 2, "type": "arrow", "version": 11, - "width": 98, - "x": 1, - "y": 0, + "width": "249.99000", + "x": "-49.99000", + "y": 50, }, "inserted": { "isDeleted": true,