add clippings bounds tolerance, improve group related checks

src/components/App.tsx

@@ -6868,10 +6868,13 @@ class App extends React.Component<AppProps, AppState> {
               topLayerFrame &&
               !this.state.selectedElementIds[topLayerFrame.id]
             ) {
+              const processedGroupIds = new Map<string, boolean>();
               const elementsToAdd = selectedElements.filter(
                 (element) =>
                   element.frameId !== topLayerFrame.id &&
-                  isElementInFrame(element, nextElements, this.state),
+                  isElementInFrame(element, nextElements, this.state, {
+                    processedGroupIds,
+                  }),
               );
 
               if (this.state.editingGroupId) {

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<ExcalidrawFrameElement>,
 ) => {
-  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<ExcalidrawElement>(prevElementsInFrame);
 
-  const elementsCompletelyInFrame = new Set([
-    ...getElementsCompletelyInFrame(allElements, frame),
-    ...prevElementsInFrame.filter((element) =>
-      isElementContainingFrame(allElements, element, frame),
-    ),
-  ]);
+  const elementsCompletelyInFrame = new Set<ExcalidrawElement>(
+    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<ExcalidrawElement>();
+  const processedGroupIds = new Map<string, boolean>();
 
   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<string, boolean>;
+  },
 ) => {
-  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;
 };

src/groups.ts

@@ -232,6 +232,8 @@ export const selectGroupsFromGivenElements = (
     selectedGroupIds: {},
   };
 
+  const processedGroupIds = new Set<string>();
+
   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];
+      if (!processedGroupIds.has(groupId)) {
         nextAppState = {
           ...nextAppState,
           ...selectGroup(groupId, nextAppState, elements),
         };
+        processedGroupIds.add(groupId);
+      }
     }
   }
 

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,61 +946,22 @@ const _renderStaticScene = ({
     );
   }
 
-  const groupsToBeAddedToFrame = new Set<string>();
-
-  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) => {
+  const visibleEmbeddableOrLabelElements = visibleElements.filter((el) =>
+    isEmbeddableOrLabel(el),
+  );
+
+  const visibleElementsToRender = [
+    ...visibleNonEmbeddableOrLabelElements,
+    ...visibleEmbeddableOrLabelElements,
+  ];
+
+  const _renderElement = (element: ExcalidrawElement) => {
     try {
-        const frameId = element.frameId || appState.frameToHighlight?.id;
-
-        if (
-          frameId &&
-          appState.frameRendering.enabled &&
-          appState.frameRendering.clip
-        ) {
-          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);
-        }
-        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 (
@@ -1011,36 +973,52 @@ const _renderStaticScene = ({
         const label = createPlaceholderEmbeddableLabel(element);
         renderElement(label, rc, context, renderConfig, appState);
       }
+
       if (!isExporting) {
         renderLinkIcon(element, context, appState);
       }
+    } catch (error: any) {
+      console.error(error);
+    }
   };
-        // - 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 processedGroupIds = new Map<string, boolean>();
+  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 &&
+        !elementsAreInBounds(
+          [element],
+          targetFrame,
+          isFreeDrawElement(element)
+            ? element.strokeWidth * 8
+            : element.roughness * (isLinearElement(element) ? 8 : 4),
+        ) &&
+        isElementInFrame(element, elements, appState, {
+          targetFrame,
+          processedGroupIds,
+        })
       ) {
         context.save();
-
-          const frame = getTargetFrame(element, appState);
-
-          if (frame && isElementInFrame(element, elements, appState)) {
-            frameClip(frame, context, renderConfig, appState);
-          }
-          render();
+        frameClip(targetFrame, context, renderConfig, appState);
+        _renderElement(element);
         context.restore();
       } else {
-          render();
+        _renderElement(element);
+      }
+    } else {
+      _renderElement(element);
     }
-      } catch (error: any) {
-        console.error(error);
   }
-    });
 };
 
 /** 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,20 +1288,7 @@ const renderFrameHighlight = (
   context.restore();
 };
 
-const renderElementsBoxHighlight = (
-  context: CanvasRenderingContext2D,
-  appState: InteractiveCanvasAppState,
-  elements: NonDeleted<ExcalidrawElement>[],
-) => {
-  const individualElements = elements.filter(
-    (element) => element.groupIds.length === 0,
-  );
-
-  const elementsInGroups = elements.filter(
-    (element) => element.groupIds.length > 0,
-  );
-
-  const getSelectionFromElements = (elements: ExcalidrawElement[]) => {
+const getSelectionFromElements = (elements: ExcalidrawElement[]) => {
   const [elementX1, elementY1, elementX2, elementY2] =
     getCommonBounds(elements);
   return {
@@ -1338,22 +1303,43 @@ const renderElementsBoxHighlight = (
     cy: elementY1 + (elementY2 - elementY1) / 2,
     activeEmbeddable: false,
   };
-  };
+};
+
+const renderElementsBoxHighlight = (
+  context: CanvasRenderingContext2D,
+  appState: InteractiveCanvasAppState,
+  elements: NonDeleted<ExcalidrawElement>[],
+) => {
+  const individualElements = elements.filter(
+    (element) => element.groupIds.length === 0,
+  );
+
+  const elementsInGroups = elements.filter(
+    (element) => element.groupIds.length > 0,
+  );
+
+  const processedGroupIds = new Set<string>();
 
   const getSelectionForGroupId = (groupId: GroupId) => {
+    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!),
     );
 };