From d1fa9005b9d1e205a1951266aadacc7cc737bf46 Mon Sep 17 00:00:00 2001 From: dwelle <5153846+dwelle@users.noreply.github.com> Date: Thu, 8 May 2025 22:59:44 +0200 Subject: [PATCH] fix polygon behavior when adding/removing/moving points within line editor --- packages/element/src/linearElementEditor.ts | 156 +++++++++++--------- packages/element/src/types.ts | 6 +- packages/excalidraw/data/restore.ts | 6 +- packages/excalidraw/data/transform.ts | 2 +- 4 files changed, 97 insertions(+), 73 deletions(-) diff --git a/packages/element/src/linearElementEditor.ts b/packages/element/src/linearElementEditor.ts index 87621259d..33dc7d3bc 100644 --- a/packages/element/src/linearElementEditor.ts +++ b/packages/element/src/linearElementEditor.ts @@ -88,11 +88,41 @@ import type { PointsPositionUpdates, } from "./types"; +/** + * Normalizes line points so that the start point is at [0,0]. This is + * expected in various parts of the codebase. + * + * Also returns the offsets - [0,0] if no normalization needed. + * + * @private + */ +const getNormalizedPoints = ({ + points, +}: { + points: ExcalidrawLinearElement["points"]; +}): { + points: LocalPoint[]; + offsetX: number; + offsetY: number; +} => { + const offsetX = points[0][0]; + const offsetY = points[0][1]; + + return { + points: points.map((p) => { + return pointFrom(p[0] - offsetX, p[1] - offsetY); + }), + offsetX, + offsetY, + }; +}; + const editorMidPointsCache: { version: number | null; points: (GlobalPoint | null)[]; zoom: number | null; } = { version: null, points: [], zoom: null }; + export class LinearElementEditor { public readonly elementId: ExcalidrawElement["id"] & { _brand: "excalidrawLinearElementId"; @@ -135,7 +165,11 @@ export class LinearElementEditor { }; if (!pointsEqual(element.points[0], pointFrom(0, 0))) { console.error("Linear element is not normalized", Error().stack); - LinearElementEditor.normalizePoints(element, elementsMap); + mutateElement( + element, + elementsMap, + LinearElementEditor.getNormalizeElementPointsAndCoords(element), + ); } this.selectedPointsIndices = null; this.lastUncommittedPoint = null; @@ -1033,7 +1067,7 @@ export class LinearElementEditor { ]), ); } else { - LinearElementEditor.addPoints(element, app.scene, [{ point: newPoint }]); + LinearElementEditor.addPoints(element, app.scene, [newPoint]); } return { ...appState.editingLinearElement, @@ -1176,40 +1210,23 @@ export class LinearElementEditor { /** * Normalizes line points so that the start point is at [0,0]. This is - * expected in various parts of the codebase. Also returns new x/y to account - * for the potential normalization. + * expected in various parts of the codebase. + * + * Also returns normalized x and y coords to account for the normalization + * of the points. */ - static getNormalizedPoints(element: ExcalidrawLinearElement): { - points: LocalPoint[]; - x: number; - y: number; - } { - const { points } = element; - - const offsetX = points[0][0]; - const offsetY = points[0][1]; + static getNormalizeElementPointsAndCoords(element: ExcalidrawLinearElement) { + const { points, offsetX, offsetY } = getNormalizedPoints(element); return { - points: points.map((p) => { - return pointFrom(p[0] - offsetX, p[1] - offsetY); - }), + points, x: element.x + offsetX, y: element.y + offsetY, }; } + // element-mutating methods // --------------------------------------------------------------------------- - static normalizePoints( - element: NonDeleted, - elementsMap: ElementsMap, - ) { - mutateElement( - element, - elementsMap, - LinearElementEditor.getNormalizedPoints(element), - ); - } - static duplicateSelectedPoints(appState: AppState, scene: Scene): AppState { invariant( appState.editingLinearElement, @@ -1291,51 +1308,43 @@ export class LinearElementEditor { app: AppClassProperties, pointIndices: readonly number[], ) { - // Handle loop lock behavior + const isUncommittedPoint = + app.state.editingLinearElement?.lastUncommittedPoint === + element.points[element.points.length - 1]; + + // break polygon if deleting start/end point if (isLineElement(element) && element.loopLock) { if ( pointIndices.includes(0) || (pointIndices.includes(element.points.length - 1) && // don't disable polygon if cleaning up uncommitted point - app.state.editingLinearElement?.lastUncommittedPoint !== - element.points[element.points.length - 1]) + !isUncommittedPoint) ) { app.scene.mutateElement(element, { loopLock: false }); } } - let offsetX = 0; - let offsetY = 0; + const nextPoints = element.points.filter((_, idx) => { + return !pointIndices.includes(idx); + }); - const isDeletingOriginPoint = pointIndices.includes(0); - - // if deleting first point, make the next to be [0,0] and recalculate - // positions of the rest with respect to it - if (isDeletingOriginPoint) { - const firstNonDeletedPoint = element.points.find((point, idx) => { - return !pointIndices.includes(idx); - }); - if (firstNonDeletedPoint) { - offsetX = firstNonDeletedPoint[0]; - offsetY = firstNonDeletedPoint[1]; - } + if (isUncommittedPoint && isLineElement(element) && element.loopLock) { + nextPoints[0] = pointFrom( + nextPoints[nextPoints.length - 1][0], + nextPoints[nextPoints.length - 1][1], + ); } - const nextPoints = element.points.reduce((acc: LocalPoint[], p, idx) => { - if (!pointIndices.includes(idx)) { - acc.push( - !acc.length - ? pointFrom(0, 0) - : pointFrom(p[0] - offsetX, p[1] - offsetY), - ); - } - return acc; - }, []); + const { + points: normalizedPoints, + offsetX, + offsetY, + } = getNormalizedPoints({ points: nextPoints }); LinearElementEditor._updatePoints( element, app.scene, - nextPoints, + normalizedPoints, offsetX, offsetY, ); @@ -1344,16 +1353,27 @@ export class LinearElementEditor { static addPoints( element: NonDeleted, scene: Scene, - targetPoints: { point: LocalPoint }[], + addedPoints: LocalPoint[], ) { - const offsetX = 0; - const offsetY = 0; + const nextPoints = [...element.points, ...addedPoints]; + + if (isLineElement(element) && element.loopLock) { + nextPoints[0] = pointFrom( + nextPoints[nextPoints.length - 1][0], + nextPoints[nextPoints.length - 1][1], + ); + } + + const { + points: normalizedPoints, + offsetX, + offsetY, + } = getNormalizedPoints({ points: nextPoints }); - const nextPoints = [...element.points, ...targetPoints.map((x) => x.point)]; LinearElementEditor._updatePoints( element, scene, - nextPoints, + normalizedPoints, offsetX, offsetY, ); @@ -1396,12 +1416,11 @@ export class LinearElementEditor { // all the other points in the opposite direction by delta to // offset it. We do the same with actual element.x/y position, so // this hacks are completely transparent to the user. - const [deltaX, deltaY] = + + const updatedOriginPoint = pointUpdates.get(0)?.point ?? pointFrom(0, 0); - const [offsetX, offsetY] = pointFrom( - deltaX - points[0][0], - deltaY - points[0][1], - ); + + const [offsetX, offsetY] = updatedOriginPoint; const nextPoints = isElbowArrow(element) ? [ @@ -1571,6 +1590,7 @@ export class LinearElementEditor { isDragging: options?.isDragging ?? false, }); } else { + // TODO do we need to get precise coords here just to calc centers? const nextCoords = getElementPointsCoords(element, nextPoints); const prevCoords = getElementPointsCoords(element, element.points); const nextCenterX = (nextCoords[0] + nextCoords[2]) / 2; @@ -1579,7 +1599,7 @@ export class LinearElementEditor { const prevCenterY = (prevCoords[1] + prevCoords[3]) / 2; const dX = prevCenterX - nextCenterX; const dY = prevCenterY - nextCenterY; - const rotated = pointRotateRads( + const rotatedOffset = pointRotateRads( pointFrom(offsetX, offsetY), pointFrom(dX, dY), element.angle, @@ -1587,8 +1607,8 @@ export class LinearElementEditor { scene.mutateElement(element, { ...otherUpdates, points: nextPoints, - x: element.x + rotated[0], - y: element.y + rotated[1], + x: element.x + rotatedOffset[0], + y: element.y + rotatedOffset[1], }); } } diff --git a/packages/element/src/types.ts b/packages/element/src/types.ts index 5fab88729..d444eab53 100644 --- a/packages/element/src/types.ts +++ b/packages/element/src/types.ts @@ -296,8 +296,10 @@ export type FixedPointBinding = Merge< } >; +type Index = number; + export type PointsPositionUpdates = Map< - number, + Index, { point: LocalPoint; isDragging?: boolean } >; @@ -335,7 +337,7 @@ export type ExcalidrawLineElement = ExcalidrawLinearElement & export type FixedSegment = { start: LocalPoint; end: LocalPoint; - index: number; + index: Index; }; export type ExcalidrawArrowElement = ExcalidrawLinearElement & diff --git a/packages/excalidraw/data/restore.ts b/packages/excalidraw/data/restore.ts index c30e9541c..2f90c0c76 100644 --- a/packages/excalidraw/data/restore.ts +++ b/packages/excalidraw/data/restore.ts @@ -324,7 +324,8 @@ const restoreElement = ( : element.points; if (points[0][0] !== 0 || points[0][1] !== 0) { - ({ points, x, y } = LinearElementEditor.getNormalizedPoints(element)); + ({ points, x, y } = + LinearElementEditor.getNormalizeElementPointsAndCoords(element)); } return restoreElementWithProperties(element, { @@ -355,7 +356,8 @@ const restoreElement = ( : element.points; if (points[0][0] !== 0 || points[0][1] !== 0) { - ({ points, x, y } = LinearElementEditor.getNormalizedPoints(element)); + ({ points, x, y } = + LinearElementEditor.getNormalizeElementPointsAndCoords(element)); } const base = { diff --git a/packages/excalidraw/data/transform.ts b/packages/excalidraw/data/transform.ts index 787f2489d..d594614fb 100644 --- a/packages/excalidraw/data/transform.ts +++ b/packages/excalidraw/data/transform.ts @@ -469,7 +469,7 @@ const bindLinearElementToElement = ( Object.assign( linearElement, - LinearElementEditor.getNormalizedPoints({ + LinearElementEditor.getNormalizeElementPointsAndCoords({ ...linearElement, points: newPoints, }),