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 }} 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 eda384f65464..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); } @@ -370,7 +367,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 +434,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); } } @@ -520,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); } @@ -587,26 +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, + opts?: { + targetFrame?: ExcalidrawFrameElement; + processedGroupIds?: Map; + }, ) => { - const frame = 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; } @@ -615,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) { @@ -637,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 2a183777bb48..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 { - elementOverlapsWithFrame, + elementsAreInBounds, getTargetFrame, isElementInFrame, } from "../frame"; @@ -945,102 +946,79 @@ 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 with embeddables on top + const visibleNonEmbeddableOrLabelElements = visibleElements.filter( + (el) => !isEmbeddableOrLabel(el), + ); - // Paint visible elements - visibleElements - .filter((el) => !isEmbeddableOrLabel(el)) - .forEach((element) => { - try { - const frameId = element.frameId || appState.frameToHighlight?.id; + const visibleEmbeddableOrLabelElements = visibleElements.filter((el) => + isEmbeddableOrLabel(el), + ); - if ( - frameId && - appState.frameRendering.enabled && - appState.frameRendering.clip - ) { - context.save(); + const visibleElementsToRender = [ + ...visibleNonEmbeddableOrLabelElements, + ...visibleEmbeddableOrLabelElements, + ]; - const frame = getTargetFrame(element, appState); + const _renderElement = (element: ExcalidrawElement) => { + 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); + } - // 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); - } - if (!isExporting) { - renderLinkIcon(element, context, appState); - } - } catch (error: any) { - console.error(error); + 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); + const processedGroupIds = new Map(); + for (const element of visibleElementsToRender) { + const frameId = element.frameId || appState.frameToHighlight?.id; - 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; - - if ( - frameId && - appState.frameRendering.enabled && - appState.frameRendering.clip - ) { - context.save(); - - const frame = getTargetFrame(element, appState); - - if (frame && isElementInFrame(element, elements, appState)) { - frameClip(frame, context, renderConfig, appState); - } - render(); - context.restore(); - } else { - render(); - } - } catch (error: any) { - console.error(error); + 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 && + !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); + _renderElement(element); + context.restore(); + } else { + _renderElement(element); } - }); + } else { + _renderElement(element); + } + } }; /** throttled to animation framerate */ @@ -1145,7 +1123,7 @@ const renderTransformHandles = ( const renderSelectionBorder = ( context: CanvasRenderingContext2D, - appState: InteractiveCanvasAppState, + appState: InteractiveCanvasAppState | StaticCanvasAppState, elementProperties: { angle: number; elementX1: number; @@ -1310,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, @@ -1323,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!), ); };