From afed893419e6792de1885f77686a43f6e0bf26e3 Mon Sep 17 00:00:00 2001 From: Ryan Di Date: Wed, 8 Nov 2023 22:50:54 +0800 Subject: [PATCH 1/8] keep only unique frames when ungrouping --- src/actions/actionGroup.tsx | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/src/actions/actionGroup.tsx b/src/actions/actionGroup.tsx index 8ee84ac7bf36..5a2d38c2c4e4 100644 --- a/src/actions/actionGroup.tsx +++ b/src/actions/actionGroup.tsx @@ -190,12 +190,18 @@ export const actionUngroup = register({ let nextElements = [...elements]; + const frameIds: { + [id: string]: true; + } = {}; const selectedElements = app.scene.getSelectedElements(appState); - const frames = selectedElements - .filter((element) => element.frameId) - .map((element) => - app.scene.getElement(element.frameId!), - ) as ExcalidrawFrameElement[]; + for (const element of selectedElements) { + if (element.frameId) { + frameIds[element.frameId] = true; + } + } + const frames = Object.keys(frameIds) + .map((eid) => app.scene.getElement(eid)) + .filter((element) => element != null) as ExcalidrawFrameElement[]; const boundTextElementIds: ExcalidrawTextElement["id"][] = []; nextElements = nextElements.map((element) => { From 9bcd0b69dcc6f73c013e68f52becaaa41450e633 Mon Sep 17 00:00:00 2001 From: Ryan Di Date: Fri, 10 Nov 2023 15:15:27 +0800 Subject: [PATCH 2/8] remove redundant code --- src/renderer/renderScene.ts | 22 +--------------------- 1 file changed, 1 insertion(+), 21 deletions(-) diff --git a/src/renderer/renderScene.ts b/src/renderer/renderScene.ts index 6395ed0e7aa7..3ccfcb555d21 100644 --- a/src/renderer/renderScene.ts +++ b/src/renderer/renderScene.ts @@ -77,11 +77,7 @@ import { isEmbeddableOrFrameLabel, createPlaceholderEmbeddableLabel, } from "../element/embeddable"; -import { - elementOverlapsWithFrame, - getTargetFrame, - isElementInFrame, -} from "../frame"; +import { getTargetFrame, isElementInFrame } from "../frame"; import "canvas-roundrect-polyfill"; export const DEFAULT_SPACING = 2; @@ -945,22 +941,6 @@ const _renderStaticScene = ({ ); } - const groupsToBeAddedToFrame = new Set(); - - visibleElements.forEach((element) => { - if ( - element.groupIds.length > 0 && - appState.frameToHighlight && - appState.selectedElementIds[element.id] && - (elementOverlapsWithFrame(element, appState.frameToHighlight) || - element.groupIds.find((groupId) => groupsToBeAddedToFrame.has(groupId))) - ) { - element.groupIds.forEach((groupId) => - groupsToBeAddedToFrame.add(groupId), - ); - } - }); - // Paint visible elements visibleElements .filter((el) => !isEmbeddableOrFrameLabel(el)) From a30e46b7560027794a6bf8709b6909adc621388d Mon Sep 17 00:00:00 2001 From: Ryan Di Date: Fri, 10 Nov 2023 19:24:32 +0800 Subject: [PATCH 3/8] batch clipping --- src/frame.ts | 3 +- src/renderer/renderScene.ts | 139 ++++++++++++++++++++---------------- 2 files changed, 78 insertions(+), 64 deletions(-) diff --git a/src/frame.ts b/src/frame.ts index eda384f65464..3cbaf5439a68 100644 --- a/src/frame.ts +++ b/src/frame.ts @@ -593,8 +593,9 @@ export const isElementInFrame = ( element: ExcalidrawElement, allElements: ExcalidrawElementsIncludingDeleted, appState: StaticCanvasAppState, + targetFrame?: ExcalidrawFrameElement, ) => { - const frame = getTargetFrame(element, appState); + const frame = targetFrame ?? getTargetFrame(element, appState); const _element = isTextElement(element) ? getContainerElement(element) || element : element; diff --git a/src/renderer/renderScene.ts b/src/renderer/renderScene.ts index 3307470248bf..524d338441a3 100644 --- a/src/renderer/renderScene.ts +++ b/src/renderer/renderScene.ts @@ -357,6 +357,36 @@ const renderLinearElementPointHighlight = ( context.restore(); }; +const getContiguousElements = ( + elements: readonly ExcalidrawElement[], + appState: StaticCanvasAppState, +) => { + const contiguousElementsArray: ExcalidrawElement[][] = []; + + let previousFrameId: string | null | undefined = null; + const contiguousElements: ExcalidrawElement[] = []; + for (const element of elements) { + const frameId = element.frameId || appState.frameToHighlight?.id; + + if (previousFrameId !== frameId) { + if (contiguousElements.length > 0) { + contiguousElementsArray.push([...contiguousElements]); + contiguousElements.length = 0; + } + + previousFrameId = frameId; + } + + contiguousElements.push(element); + } + + if (contiguousElements.length > 0) { + contiguousElementsArray.push([...contiguousElements]); + } + + return contiguousElementsArray; +}; + const frameClip = ( frame: ExcalidrawFrameElement, context: CanvasRenderingContext2D, @@ -941,86 +971,69 @@ const _renderStaticScene = ({ ); } - // Paint visible elements - visibleElements - .filter((el) => !isEmbeddableOrLabel(el)) - .forEach((element) => { + // Paint visible elements with embeddables on top + const visibleNonEmbeddableOrLabelElements = visibleElements.filter( + (el) => !isEmbeddableOrLabel(el), + ); + + const visibleEmbeddableOrLabelElements = visibleElements.filter((el) => + isEmbeddableOrLabel(el), + ); + + const contiguousElementsArray = [ + ...getContiguousElements(visibleNonEmbeddableOrLabelElements, appState), + ...getContiguousElements(visibleEmbeddableOrLabelElements, appState), + ]; + + const renderContiguousElements = ( + contiguousElements: ExcalidrawElement[], + ) => { + for (const element of contiguousElements) { try { - const frameId = element.frameId || appState.frameToHighlight?.id; + renderElement(element, rc, context, renderConfig, appState); if ( - frameId && - appState.frameRendering.enabled && - appState.frameRendering.clip + isEmbeddableElement(element) && + (isExporting || !element.validated) && + element.width && + element.height ) { - context.save(); - - const frame = getTargetFrame(element, appState); - - // TODO do we need to check isElementInFrame here? - if (frame && isElementInFrame(element, elements, appState)) { - frameClip(frame, context, renderConfig, appState); - } - renderElement(element, rc, context, renderConfig, appState); - context.restore(); - } else { - renderElement(element, rc, context, renderConfig, appState); + const label = createPlaceholderEmbeddableLabel(element); + renderElement(label, rc, context, renderConfig, appState); } + if (!isExporting) { renderLinkIcon(element, context, appState); } } catch (error: any) { console.error(error); } - }); - - // render embeddables on top - visibleElements - .filter((el) => isEmbeddableOrLabel(el)) - .forEach((element) => { - try { - const render = () => { - renderElement(element, rc, context, renderConfig, appState); + } + }; - if ( - isEmbeddableElement(element) && - (isExporting || !element.validated) && - element.width && - element.height - ) { - const label = createPlaceholderEmbeddableLabel(element); - renderElement(label, rc, context, renderConfig, appState); - } - if (!isExporting) { - renderLinkIcon(element, context, appState); - } - }; - // - when exporting the whole canvas, we DO NOT apply clipping - // - when we are exporting a particular frame, apply clipping - // if the containing frame is not selected, apply clipping - const frameId = element.frameId || appState.frameToHighlight?.id; + for (const contiguousElements of contiguousElementsArray) { + const firstElement = contiguousElements[0]; - if ( - frameId && - appState.frameRendering.enabled && - appState.frameRendering.clip - ) { - context.save(); + if (firstElement) { + context.save(); + const frameId = firstElement.frameId || appState.frameToHighlight?.id; - const frame = getTargetFrame(element, appState); + if ( + frameId && + appState.frameRendering.enabled && + appState.frameRendering.clip + ) { + const frame = getTargetFrame(firstElement, appState); - if (frame && isElementInFrame(element, elements, appState)) { - frameClip(frame, context, renderConfig, appState); - } - render(); - context.restore(); - } else { - render(); + if (frame && isElementInFrame(firstElement, elements, appState)) { + frameClip(frame, context, renderConfig, appState); } - } catch (error: any) { - console.error(error); } - }); + + renderContiguousElements(contiguousElements); + context.restore(); + } + } }; /** throttled to animation framerate */ From 34cf71b0f4942a9bbe8f2dd26d7b25509f066557 Mon Sep 17 00:00:00 2001 From: Ryan Di Date: Mon, 13 Nov 2023 18:17:39 +0800 Subject: [PATCH 4/8] keep dynamic clipping with batch clipping --- src/renderer/renderScene.ts | 94 ++++++++++++++++++++++++------------- 1 file changed, 62 insertions(+), 32 deletions(-) diff --git a/src/renderer/renderScene.ts b/src/renderer/renderScene.ts index 524d338441a3..e18e5aa64863 100644 --- a/src/renderer/renderScene.ts +++ b/src/renderer/renderScene.ts @@ -363,18 +363,36 @@ const getContiguousElements = ( ) => { const contiguousElementsArray: ExcalidrawElement[][] = []; - let previousFrameId: string | null | undefined = null; + let previousFrame: ExcalidrawFrameElement | null = null; const contiguousElements: ExcalidrawElement[] = []; - for (const element of elements) { - const frameId = element.frameId || appState.frameToHighlight?.id; - if (previousFrameId !== frameId) { + const isNewContiguous = ( + element: ExcalidrawElement, + targetFrame: ExcalidrawFrameElement | null, + ) => { + return ( + // element going to be added to some frame or not in any frame + !element.frameId || + // element in frame but is either going to be put to a different frame / removed + (element.frameId && + appState.selectedElementIds[element.id] && + appState.selectedElementsAreBeingDragged && + (!targetFrame || + isElementInFrame(element, elements, appState, targetFrame))) || + // element in a different frame from previous element + element.frameId !== previousFrame?.id + ); + }; + + for (const element of elements) { + const targetFrame = getTargetFrame(element, appState); + if (isNewContiguous(element, targetFrame)) { if (contiguousElements.length > 0) { contiguousElementsArray.push([...contiguousElements]); contiguousElements.length = 0; } - previousFrameId = frameId; + previousFrame = targetFrame; } contiguousElements.push(element); @@ -925,6 +943,38 @@ const _renderInteractiveScene = ({ }; }; +const renderContiguousElements = ( + rc: StaticSceneRenderConfig["rc"], + appState: StaticSceneRenderConfig["appState"], + renderConfig: StaticSceneRenderConfig["renderConfig"], + context: CanvasRenderingContext2D, + contiguousElements: ExcalidrawElement[], +) => { + const { isExporting } = renderConfig; + + for (const element of contiguousElements) { + try { + renderElement(element, rc, context, renderConfig, appState); + + if ( + isEmbeddableElement(element) && + (isExporting || !element.validated) && + element.width && + element.height + ) { + const label = createPlaceholderEmbeddableLabel(element); + renderElement(label, rc, context, renderConfig, appState); + } + + if (!isExporting) { + renderLinkIcon(element, context, appState); + } + } catch (error: any) { + console.error(error); + } + } +}; + const _renderStaticScene = ({ canvas, rc, @@ -985,32 +1035,6 @@ const _renderStaticScene = ({ ...getContiguousElements(visibleEmbeddableOrLabelElements, appState), ]; - const renderContiguousElements = ( - contiguousElements: ExcalidrawElement[], - ) => { - for (const element of contiguousElements) { - try { - renderElement(element, rc, context, renderConfig, appState); - - if ( - isEmbeddableElement(element) && - (isExporting || !element.validated) && - element.width && - element.height - ) { - const label = createPlaceholderEmbeddableLabel(element); - renderElement(label, rc, context, renderConfig, appState); - } - - if (!isExporting) { - renderLinkIcon(element, context, appState); - } - } catch (error: any) { - console.error(error); - } - } - }; - for (const contiguousElements of contiguousElementsArray) { const firstElement = contiguousElements[0]; @@ -1030,7 +1054,13 @@ const _renderStaticScene = ({ } } - renderContiguousElements(contiguousElements); + renderContiguousElements( + rc, + appState, + renderConfig, + context, + contiguousElements, + ); context.restore(); } } From 47d8fa542cec5b31ca2da0e4620ce9903f11dd53 Mon Sep 17 00:00:00 2001 From: Ryan Di Date: Mon, 13 Nov 2023 18:18:36 +0800 Subject: [PATCH 5/8] remove redundant code --- src/frame.ts | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/src/frame.ts b/src/frame.ts index 3cbaf5439a68..8030c44e08a4 100644 --- a/src/frame.ts +++ b/src/frame.ts @@ -370,7 +370,7 @@ export const getContainingFrame = ( // --------------------------- Frame Operations ------------------------------- /** - * Retains (or repairs for target frame) the ordering invriant where children + * Retains (or repairs for target frame) the ordering invariant where children * elements come right before the parent frame: * [el, el, child, child, frame, el] */ @@ -437,25 +437,14 @@ export const removeElementsFromFrame = ( ExcalidrawElement >(); - const toRemoveElementsByFrame = new Map< - ExcalidrawFrameElement["id"], - ExcalidrawElement[] - >(); - for (const element of elementsToRemove) { if (element.frameId) { _elementsToRemove.set(element.id, element); - const arr = toRemoveElementsByFrame.get(element.frameId) || []; - arr.push(element); - const boundTextElement = getBoundTextElement(element); if (boundTextElement) { _elementsToRemove.set(boundTextElement.id, boundTextElement); - arr.push(boundTextElement); } - - toRemoveElementsByFrame.set(element.frameId, arr); } } From 683b80ad2b33f792c383c87a9ccb6a024c1ce579 Mon Sep 17 00:00:00 2001 From: Ryan Di Date: Tue, 14 Nov 2023 18:31:21 +0800 Subject: [PATCH 6/8] alternative clipping improvement --- src/renderer/renderScene.ts | 154 ++++++++++++------------------------ 1 file changed, 49 insertions(+), 105 deletions(-) diff --git a/src/renderer/renderScene.ts b/src/renderer/renderScene.ts index e18e5aa64863..a4afeda6084d 100644 --- a/src/renderer/renderScene.ts +++ b/src/renderer/renderScene.ts @@ -77,7 +77,11 @@ import { isEmbeddableOrLabel, createPlaceholderEmbeddableLabel, } from "../element/embeddable"; -import { getTargetFrame, isElementInFrame } from "../frame"; +import { + elementsAreInFrameBounds, + getTargetFrame, + isElementInFrame, +} from "../frame"; import "canvas-roundrect-polyfill"; export const DEFAULT_SPACING = 2; @@ -357,54 +361,6 @@ const renderLinearElementPointHighlight = ( context.restore(); }; -const getContiguousElements = ( - elements: readonly ExcalidrawElement[], - appState: StaticCanvasAppState, -) => { - const contiguousElementsArray: ExcalidrawElement[][] = []; - - let previousFrame: ExcalidrawFrameElement | null = null; - const contiguousElements: ExcalidrawElement[] = []; - - const isNewContiguous = ( - element: ExcalidrawElement, - targetFrame: ExcalidrawFrameElement | null, - ) => { - return ( - // element going to be added to some frame or not in any frame - !element.frameId || - // element in frame but is either going to be put to a different frame / removed - (element.frameId && - appState.selectedElementIds[element.id] && - appState.selectedElementsAreBeingDragged && - (!targetFrame || - isElementInFrame(element, elements, appState, targetFrame))) || - // element in a different frame from previous element - element.frameId !== previousFrame?.id - ); - }; - - for (const element of elements) { - const targetFrame = getTargetFrame(element, appState); - if (isNewContiguous(element, targetFrame)) { - if (contiguousElements.length > 0) { - contiguousElementsArray.push([...contiguousElements]); - contiguousElements.length = 0; - } - - previousFrame = targetFrame; - } - - contiguousElements.push(element); - } - - if (contiguousElements.length > 0) { - contiguousElementsArray.push([...contiguousElements]); - } - - return contiguousElementsArray; -}; - const frameClip = ( frame: ExcalidrawFrameElement, context: CanvasRenderingContext2D, @@ -943,38 +899,6 @@ const _renderInteractiveScene = ({ }; }; -const renderContiguousElements = ( - rc: StaticSceneRenderConfig["rc"], - appState: StaticSceneRenderConfig["appState"], - renderConfig: StaticSceneRenderConfig["renderConfig"], - context: CanvasRenderingContext2D, - contiguousElements: ExcalidrawElement[], -) => { - const { isExporting } = renderConfig; - - for (const element of contiguousElements) { - try { - renderElement(element, rc, context, renderConfig, appState); - - if ( - isEmbeddableElement(element) && - (isExporting || !element.validated) && - element.width && - element.height - ) { - const label = createPlaceholderEmbeddableLabel(element); - renderElement(label, rc, context, renderConfig, appState); - } - - if (!isExporting) { - renderLinkIcon(element, context, appState); - } - } catch (error: any) { - console.error(error); - } - } -}; - const _renderStaticScene = ({ canvas, rc, @@ -1030,38 +954,58 @@ const _renderStaticScene = ({ isEmbeddableOrLabel(el), ); - const contiguousElementsArray = [ - ...getContiguousElements(visibleNonEmbeddableOrLabelElements, appState), - ...getContiguousElements(visibleEmbeddableOrLabelElements, appState), + const visibleElementsToRender = [ + ...visibleNonEmbeddableOrLabelElements, + ...visibleEmbeddableOrLabelElements, ]; - for (const contiguousElements of contiguousElementsArray) { - const firstElement = contiguousElements[0]; - - if (firstElement) { - context.save(); - const frameId = firstElement.frameId || appState.frameToHighlight?.id; + const _renderElement = (element: ExcalidrawElement) => { + try { + renderElement(element, rc, context, renderConfig, appState); if ( - frameId && - appState.frameRendering.enabled && - appState.frameRendering.clip + isEmbeddableElement(element) && + (isExporting || !element.validated) && + element.width && + element.height ) { - const frame = getTargetFrame(firstElement, appState); + const label = createPlaceholderEmbeddableLabel(element); + renderElement(label, rc, context, renderConfig, appState); + } - if (frame && isElementInFrame(firstElement, elements, appState)) { - frameClip(frame, context, renderConfig, appState); - } + if (!isExporting) { + renderLinkIcon(element, context, appState); } + } catch (error: any) { + console.error(error); + } + }; - renderContiguousElements( - rc, - appState, - renderConfig, - context, - contiguousElements, - ); - context.restore(); + for (const element of visibleElementsToRender) { + const frameId = element.frameId || appState.frameToHighlight?.id; + + if ( + frameId && + appState.frameRendering.enabled && + appState.frameRendering.clip + ) { + const targetFrame = getTargetFrame(element, appState); + // for perf: + // only clip elements that are not completely in the target frame + if ( + targetFrame && + !elementsAreInFrameBounds([element], targetFrame) && + isElementInFrame(element, elements, appState) + ) { + context.save(); + frameClip(targetFrame, context, renderConfig, appState); + _renderElement(element); + context.restore(); + } else { + _renderElement(element); + } + } else { + _renderElement(element); } } }; From 3c34b3f48a0540dfbe123472f9e1f65e13146483 Mon Sep 17 00:00:00 2001 From: Ryan Di Date: Fri, 17 Nov 2023 17:23:30 +0800 Subject: [PATCH 7/8] add clippings bounds tolerance, improve group related checks --- src/components/App.tsx | 5 +- src/frame.ts | 122 ++++++++++++++++++++++-------------- src/groups.ts | 13 ++-- src/renderer/renderScene.ts | 65 ++++++++++++------- 4 files changed, 130 insertions(+), 75 deletions(-) diff --git a/src/components/App.tsx b/src/components/App.tsx index 8e4ab63552e6..0c63fb67f2a7 100644 --- a/src/components/App.tsx +++ b/src/components/App.tsx @@ -6868,10 +6868,13 @@ class App extends React.Component { topLayerFrame && !this.state.selectedElementIds[topLayerFrame.id] ) { + const processedGroupIds = new Map(); const elementsToAdd = selectedElements.filter( (element) => element.frameId !== topLayerFrame.id && - isElementInFrame(element, nextElements, this.state), + isElementInFrame(element, nextElements, this.state, { + processedGroupIds, + }), ); if (this.state.editingGroupId) { diff --git a/src/frame.ts b/src/frame.ts index 8030c44e08a4..0262b3acef4f 100644 --- a/src/frame.ts +++ b/src/frame.ts @@ -1,8 +1,4 @@ -import { - getCommonBounds, - getElementAbsoluteCoords, - isTextElement, -} from "./element"; +import { getCommonBounds, getElementBounds, isTextElement } from "./element"; import { ExcalidrawElement, ExcalidrawFrameElement, @@ -56,6 +52,7 @@ export const bindElementsToFramesAfterDuplication = ( } }; +// --------------------------- Frame Geometry --------------------------------- export function isElementIntersectingFrame( element: ExcalidrawElement, frame: ExcalidrawFrameElement, @@ -85,36 +82,27 @@ export const getElementsCompletelyInFrame = ( element.frameId === frame.id, ); -export const isElementContainingFrame = ( - elements: readonly ExcalidrawElement[], - element: ExcalidrawElement, - frame: ExcalidrawFrameElement, -) => { - return getElementsWithinSelection(elements, element).some( - (e) => e.id === frame.id, - ); -}; - export const getElementsIntersectingFrame = ( elements: readonly ExcalidrawElement[], frame: ExcalidrawFrameElement, ) => elements.filter((element) => isElementIntersectingFrame(element, frame)); -export const elementsAreInFrameBounds = ( +export const elementsAreInBounds = ( elements: readonly ExcalidrawElement[], - frame: ExcalidrawFrameElement, + element: ExcalidrawElement, + tolerance = 0, ) => { - const [selectionX1, selectionY1, selectionX2, selectionY2] = - getElementAbsoluteCoords(frame); - const [elementX1, elementY1, elementX2, elementY2] = + getElementBounds(element); + + const [elementsX1, elementsY1, elementsX2, elementsY2] = getCommonBounds(elements); return ( - selectionX1 <= elementX1 && - selectionY1 <= elementY1 && - selectionX2 >= elementX2 && - selectionY2 >= elementY2 + elementX1 <= elementsX1 - tolerance && + elementY1 <= elementsY1 - tolerance && + elementX2 >= elementsX2 + tolerance && + elementY2 >= elementsY2 + tolerance ); }; @@ -123,9 +111,12 @@ export const elementOverlapsWithFrame = ( frame: ExcalidrawFrameElement, ) => { return ( - elementsAreInFrameBounds([element], frame) || - isElementIntersectingFrame(element, frame) || - isElementContainingFrame([frame], element, frame) + // frame contains element + elementsAreInBounds([element], frame) || + // element contains frame + (elementsAreInBounds([frame], element) && element.frameId === frame.id) || + // element intersects with frame + isElementIntersectingFrame(element, frame) ); }; @@ -136,7 +127,7 @@ export const isCursorInFrame = ( }, frame: NonDeleted, ) => { - const [fx1, fy1, fx2, fy2] = getElementAbsoluteCoords(frame); + const [fx1, fy1, fx2, fy2] = getElementBounds(frame); return isPointWithinBounds( [fx1, fy1], @@ -160,7 +151,7 @@ export const groupsAreAtLeastIntersectingTheFrame = ( return !!elementsInGroup.find( (element) => - elementsAreInFrameBounds([element], frame) || + elementsAreInBounds([element], frame) || isElementIntersectingFrame(element, frame), ); }; @@ -181,7 +172,7 @@ export const groupsAreCompletelyOutOfFrame = ( return ( elementsInGroup.find( (element) => - elementsAreInFrameBounds([element], frame) || + elementsAreInBounds([element], frame) || isElementIntersectingFrame(element, frame), ) === undefined ); @@ -249,12 +240,18 @@ export const getElementsInResizingFrame = ( const prevElementsInFrame = getFrameChildren(allElements, frame.id); const nextElementsInFrame = new Set(prevElementsInFrame); - const elementsCompletelyInFrame = new Set([ - ...getElementsCompletelyInFrame(allElements, frame), - ...prevElementsInFrame.filter((element) => - isElementContainingFrame(allElements, element, frame), - ), - ]); + const elementsCompletelyInFrame = new Set( + getElementsCompletelyInFrame(allElements, frame), + ); + + for (const element of prevElementsInFrame) { + if (!elementsCompletelyInFrame.has(element)) { + // element contains the frame + if (elementsAreInBounds([frame], element)) { + elementsCompletelyInFrame.add(element); + } + } + } const elementsNotCompletelyInFrame = prevElementsInFrame.filter( (element) => !elementsCompletelyInFrame.has(element), @@ -321,7 +318,7 @@ export const getElementsInResizingFrame = ( if (isSelected) { const elementsInGroup = getElementsInGroup(allElements, id); - if (elementsAreInFrameBounds(elementsInGroup, frame)) { + if (elementsAreInBounds(elementsInGroup, frame)) { for (const element of elementsInGroup) { nextElementsInFrame.add(element); } @@ -509,12 +506,15 @@ export const updateFrameMembershipOfSelectedElements = ( } const elementsToRemove = new Set(); + const processedGroupIds = new Map(); elementsToFilter.forEach((element) => { if ( element.frameId && !isFrameElement(element) && - !isElementInFrame(element, allElements, appState) + !isElementInFrame(element, allElements, appState, { + processedGroupIds, + }) ) { elementsToRemove.add(element); } @@ -576,27 +576,36 @@ export const getTargetFrame = ( : getContainingFrame(_element); }; -// TODO: this a huge bottleneck for large scenes, optimise // given an element, return if the element is in some frame export const isElementInFrame = ( element: ExcalidrawElement, allElements: ExcalidrawElementsIncludingDeleted, appState: StaticCanvasAppState, - targetFrame?: ExcalidrawFrameElement, + opts?: { + targetFrame?: ExcalidrawFrameElement; + processedGroupIds?: Map; + }, ) => { - const frame = targetFrame ?? getTargetFrame(element, appState); + const frame = opts?.targetFrame ?? getTargetFrame(element, appState); const _element = isTextElement(element) ? getContainerElement(element) || element : element; + const groupsInFrame = (yes: boolean) => { + if (opts?.processedGroupIds) { + _element.groupIds.forEach((gid) => { + opts.processedGroupIds?.set(gid, yes); + }); + } + }; + if (frame) { // Perf improvement: - // For an element that's already in a frame, if it's not being dragged - // then there is no need to refer to geometry (which, yes, is slow) to check if it's in a frame. - // It has to be in its containing frame. + // For an element that's already in a frame, if it's not being selected + // and its frame is not being selected, it has to be in its containing frame. if ( - !appState.selectedElementIds[element.id] || - !appState.selectedElementsAreBeingDragged + !appState.selectedElementIds[element.id] && + !appState.selectedElementIds[frame.id] ) { return true; } @@ -605,8 +614,21 @@ export const isElementInFrame = ( return elementOverlapsWithFrame(_element, frame); } + for (const gid of _element.groupIds) { + if (opts?.processedGroupIds?.has(gid)) { + return opts.processedGroupIds.get(gid); + } + } + const allElementsInGroup = new Set( - _element.groupIds.flatMap((gid) => getElementsInGroup(allElements, gid)), + _element.groupIds + .filter((gid) => { + if (opts?.processedGroupIds) { + return !opts.processedGroupIds.has(gid); + } + return true; + }) + .flatMap((gid) => getElementsInGroup(allElements, gid)), ); if (appState.editingGroupId && appState.selectedElementsAreBeingDragged) { @@ -627,16 +649,22 @@ export const isElementInFrame = ( for (const elementInGroup of allElementsInGroup) { if (isFrameElement(elementInGroup)) { + groupsInFrame(false); return false; } } for (const elementInGroup of allElementsInGroup) { if (elementOverlapsWithFrame(elementInGroup, frame)) { + groupsInFrame(true); return true; } } } + if (_element.groupIds.length > 0) { + groupsInFrame(false); + } + return false; }; diff --git a/src/groups.ts b/src/groups.ts index dd5512ba12d8..2d14919f334f 100644 --- a/src/groups.ts +++ b/src/groups.ts @@ -232,6 +232,8 @@ export const selectGroupsFromGivenElements = ( selectedGroupIds: {}, }; + const processedGroupIds = new Set(); + for (const element of elements) { let groupIds = element.groupIds; if (appState.editingGroupId) { @@ -242,10 +244,13 @@ export const selectGroupsFromGivenElements = ( } if (groupIds.length > 0) { const groupId = groupIds[groupIds.length - 1]; - nextAppState = { - ...nextAppState, - ...selectGroup(groupId, nextAppState, elements), - }; + if (!processedGroupIds.has(groupId)) { + nextAppState = { + ...nextAppState, + ...selectGroup(groupId, nextAppState, elements), + }; + processedGroupIds.add(groupId); + } } } diff --git a/src/renderer/renderScene.ts b/src/renderer/renderScene.ts index a4afeda6084d..a8880797563a 100644 --- a/src/renderer/renderScene.ts +++ b/src/renderer/renderScene.ts @@ -71,6 +71,7 @@ import { renderSnaps } from "./renderSnaps"; import { isEmbeddableElement, isFrameElement, + isFreeDrawElement, isLinearElement, } from "../element/typeChecks"; import { @@ -78,7 +79,7 @@ import { createPlaceholderEmbeddableLabel, } from "../element/embeddable"; import { - elementsAreInFrameBounds, + elementsAreInBounds, getTargetFrame, isElementInFrame, } from "../frame"; @@ -981,6 +982,7 @@ const _renderStaticScene = ({ } }; + const processedGroupIds = new Map(); for (const element of visibleElementsToRender) { const frameId = element.frameId || appState.frameToHighlight?.id; @@ -994,8 +996,17 @@ const _renderStaticScene = ({ // only clip elements that are not completely in the target frame if ( targetFrame && - !elementsAreInFrameBounds([element], targetFrame) && - isElementInFrame(element, elements, appState) + !elementsAreInBounds( + [element], + targetFrame, + isFreeDrawElement(element) + ? element.strokeWidth * 8 + : element.roughness * (isLinearElement(element) ? 8 : 4), + ) && + isElementInFrame(element, elements, appState, { + targetFrame, + processedGroupIds, + }) ) { context.save(); frameClip(targetFrame, context, renderConfig, appState); @@ -1112,7 +1123,7 @@ const renderTransformHandles = ( const renderSelectionBorder = ( context: CanvasRenderingContext2D, - appState: InteractiveCanvasAppState, + appState: InteractiveCanvasAppState | StaticCanvasAppState, elementProperties: { angle: number; elementX1: number; @@ -1277,6 +1288,23 @@ const renderFrameHighlight = ( context.restore(); }; +const getSelectionFromElements = (elements: ExcalidrawElement[]) => { + const [elementX1, elementY1, elementX2, elementY2] = + getCommonBounds(elements); + return { + angle: 0, + elementX1, + elementX2, + elementY1, + elementY2, + selectionColors: ["rgb(0,118,255)"], + dashed: false, + cx: elementX1 + (elementX2 - elementX1) / 2, + cy: elementY1 + (elementY2 - elementY1) / 2, + activeEmbeddable: false, + }; +}; + const renderElementsBoxHighlight = ( context: CanvasRenderingContext2D, appState: InteractiveCanvasAppState, @@ -1290,37 +1318,28 @@ const renderElementsBoxHighlight = ( (element) => element.groupIds.length > 0, ); - const getSelectionFromElements = (elements: ExcalidrawElement[]) => { - const [elementX1, elementY1, elementX2, elementY2] = - getCommonBounds(elements); - return { - angle: 0, - elementX1, - elementX2, - elementY1, - elementY2, - selectionColors: ["rgb(0,118,255)"], - dashed: false, - cx: elementX1 + (elementX2 - elementX1) / 2, - cy: elementY1 + (elementY2 - elementY1) / 2, - activeEmbeddable: false, - }; - }; + const processedGroupIds = new Set(); const getSelectionForGroupId = (groupId: GroupId) => { - const groupElements = getElementsInGroup(elements, groupId); - return getSelectionFromElements(groupElements); + if (!processedGroupIds.has(groupId)) { + const groupElements = getElementsInGroup(elements, groupId); + processedGroupIds.add(groupId); + return getSelectionFromElements(groupElements); + } + + return null; }; Object.entries(selectGroupsFromGivenElements(elementsInGroups, appState)) .filter(([id, isSelected]) => isSelected) .map(([id, isSelected]) => id) .map((groupId) => getSelectionForGroupId(groupId)) + .filter((selection) => selection) .concat( individualElements.map((element) => getSelectionFromElements([element])), ) .forEach((selection) => - renderSelectionBorder(context, appState, selection), + renderSelectionBorder(context, appState, selection!), ); }; From 44bbaeda68428f871c3d9e49cba10835783189f3 Mon Sep 17 00:00:00 2001 From: Adam Pavlisin Date: Fri, 16 Aug 2024 10:58:33 +0200 Subject: [PATCH 8/8] Create callstack-reviewer.yml --- .github/workflows/callstack-reviewer.yml | 27 ++++++++++++++++++++++++ 1 file changed, 27 insertions(+) create mode 100644 .github/workflows/callstack-reviewer.yml diff --git a/.github/workflows/callstack-reviewer.yml b/.github/workflows/callstack-reviewer.yml new file mode 100644 index 000000000000..93b37215c9e9 --- /dev/null +++ b/.github/workflows/callstack-reviewer.yml @@ -0,0 +1,27 @@ +name: Callstack.ai PR Review + +on: + workflow_dispatch: + inputs: + config: + type: string + description: "config for reviewer" + required: true + head: + type: string + description: "head commit sha" + required: true + base: + type: string + description: "base commit sha" + required: false + +jobs: + callstack_pr_review_job: + runs-on: ubuntu-latest + steps: + - name: Review PR + uses: callstackai/action@main + with: + config: ${{ inputs.config }} + head: ${{ inputs.head }}