fix rotated filled box-selection & refactor

packages/element/src/bounds.ts

@@ -27,6 +27,7 @@ import type {
   GlobalPoint,
   LineSegment,
   LocalPoint,
+  NonRotated,
   Radians,
 } from "@excalidraw/math";
 
@@ -996,12 +997,16 @@ const getLinearElementRotatedBounds = (
   return coords;
 };
 
-export const getElementBounds = (
+export const getElementBounds = <T extends boolean = false>(
   element: ExcalidrawElement,
   elementsMap: ElementsMap,
-  nonRotated: boolean = false,
+  nonRotated: T = false as T,
-): Bounds => {
+): T extends true ? NonRotated<Bounds> : Bounds => {
-  return ElementBounds.getBounds(element, elementsMap, nonRotated);
+  return ElementBounds.getBounds(
+    element,
+    elementsMap,
+    nonRotated,
+  ) as T extends true ? NonRotated<Bounds> : Bounds;
 };
 
 export const getCommonBounds = (

packages/element/src/collision.ts

@@ -22,6 +22,7 @@ import type {
   Curve,
   GlobalPoint,
   LineSegment,
+  NonRotated,
   Radians,
 } from "@excalidraw/math";
 
@@ -189,7 +190,17 @@ export const hitElementItself = ({
   return result;
 };
 
-const getBoundsCorners = (bounds: Bounds, angle: Radians = 0 as Radians) => {
+export function getBoundsCorners(
+  bounds: Bounds,
+): readonly [GlobalPoint, GlobalPoint, GlobalPoint, GlobalPoint];
+export function getBoundsCorners(
+  bounds: NonRotated<Bounds>,
+  angle: Radians,
+): readonly [GlobalPoint, GlobalPoint, GlobalPoint, GlobalPoint];
+export function getBoundsCorners(
+  bounds: Bounds,
+  angle: Radians = 0 as Radians,
+) {
   const [x1, y1, x2, y2] = bounds;
   const center = getCenterForBounds(bounds);
   const corners = [
@@ -209,9 +220,9 @@ const getBoundsCorners = (bounds: Bounds, angle: Radians = 0 as Radians) => {
     GlobalPoint,
     GlobalPoint,
   ];
-};
+}
 
-const getBoundsEdges = (
+export const getBoundsEdges = (
   corners: readonly [GlobalPoint, GlobalPoint, GlobalPoint, GlobalPoint],
 ) =>
   [
@@ -223,7 +234,7 @@ const getBoundsEdges = (
 
 const isPointInRotatedBounds = (
   point: GlobalPoint,
-  bounds: Bounds,
+  bounds: NonRotated<Bounds>,
   angle: Radians,
   tolerance = 0,
 ) => {

packages/element/src/selection.ts

@@ -19,13 +19,17 @@ import {
 } from "./bounds";
 import {
   doBoundsIntersectElementBoundingBox,
+  getBoundsCorners,
+  getBoundsEdges,
   intersectElementWithLineSegment,
+  isPointInElement,
   shouldTestInside,
 } from "./collision";
 import { isElementInViewport } from "./sizeHelpers";
 import {
   isBoundToContainer,
   isFrameLikeElement,
+  isFreeDrawElement,
   isLinearElement,
   isTextElement,
 } from "./typeChecks";
@@ -46,17 +50,15 @@ import type {
   NonDeletedExcalidrawElement,
 } from "./types";
 
-// Broad-phase only for overlap mode. This decides whether plain AABB overlap
+// Broad-phase only for overlap mode. Rotated closed shapes should not select
-// should be refined via the element's rotated local bounds. Elements that fail
+// from the empty corners of their axis-aligned bounds. Linear elements and
-// `shouldTestInside()` still go through the outline-specific path below, so
+// freedraw already rely on the outline-specific path below, so exclude them.
-// this is intentionally not the whole overlap-selection policy.
 const shouldUseRotatedOverlapBroadPhase = (
   element: NonDeletedExcalidrawElement,
 ) =>
   element.angle !== 0 &&
-  (isTextElement(element) ||
+  !isLinearElement(element) &&
-    element.type === "freedraw" ||
+  !isFreeDrawElement(element);
-    element.type === "image");
 
 const clipLineSegmentToBounds = (
   segment: LineSegment<GlobalPoint>,
@@ -112,6 +114,11 @@ const isPointWithinAabb = (point: GlobalPoint, bounds: Bounds) =>
   point[1] >= bounds[1] &&
   point[1] <= bounds[3];
 
+const shouldUsePreciseFilledOverlap = (element: NonDeletedExcalidrawElement) =>
+  element.type === "ellipse" ||
+  element.type === "diamond" ||
+  (element.type === "rectangle" && !!element.roundness);
+
 const shouldSkipElementFromSelection = (element: NonDeletedExcalidrawElement) =>
   element.locked || element.type === "selection" || isBoundToContainer(element);
 
@@ -149,38 +156,6 @@ const finalizeElementsInSelection = (
   });
 };
 
-const getSelectionEdges = (
-  selectionBounds: Bounds,
-): readonly LineSegment<GlobalPoint>[] => {
-  const selectionTopLeft = pointFrom<GlobalPoint>(
-    selectionBounds[0],
-    selectionBounds[1],
-  );
-  const selectionBottomRight = pointFrom<GlobalPoint>(
-    selectionBounds[2],
-    selectionBounds[3],
-  );
-
-  return [
-    lineSegment(
-      selectionTopLeft,
-      pointFrom<GlobalPoint>(selectionBounds[2], selectionBounds[1]),
-    ),
-    lineSegment(
-      pointFrom<GlobalPoint>(selectionBounds[2], selectionBounds[1]),
-      selectionBottomRight,
-    ),
-    lineSegment(
-      selectionBottomRight,
-      pointFrom<GlobalPoint>(selectionBounds[0], selectionBounds[3]),
-    ),
-    lineSegment(
-      pointFrom<GlobalPoint>(selectionBounds[0], selectionBounds[3]),
-      selectionTopLeft,
-    ),
-  ];
-};
-
 const getVisibleElementOutlineSegments = (
   element: NonDeletedExcalidrawElement,
   frameBounds: Bounds | null,
@@ -220,6 +195,60 @@ const doesSelectionContainElementOutline = (
       isPointWithinAabb(outlineSegment[1], selectionBounds),
   );
 
+const doesSelectionContainElementInterior = (
+  element: NonDeletedExcalidrawElement,
+  frameBounds: Bounds | null,
+  selectionCorners: readonly GlobalPoint[],
+  elementsMap: ElementsMap,
+) =>
+  selectionCorners.some(
+    (selectionCorner) =>
+      (!frameBounds || isPointWithinAabb(selectionCorner, frameBounds)) &&
+      isPointInElement(selectionCorner, element, elementsMap),
+  );
+
+const doesSelectionOverlapFilledElement = (
+  element: NonDeletedExcalidrawElement,
+  frameBounds: Bounds | null,
+  selectionBounds: Bounds,
+  selectionCorners: readonly GlobalPoint[],
+  selectionEdges: readonly LineSegment<GlobalPoint>[],
+  elementsMap: ElementsMap,
+) => {
+  if (
+    doesSelectionContainElementInterior(
+      element,
+      frameBounds,
+      selectionCorners,
+      elementsMap,
+    )
+  ) {
+    return true;
+  }
+
+  if (
+    doesSelectionIntersectElementOutline(
+      element,
+      frameBounds,
+      selectionEdges,
+      elementsMap,
+    )
+  ) {
+    return true;
+  }
+
+  const outlineSegments = getVisibleElementOutlineSegments(
+    element,
+    frameBounds,
+    elementsMap,
+  );
+
+  return (
+    outlineSegments.length > 0 &&
+    doesSelectionContainElementOutline(outlineSegments, selectionBounds)
+  );
+};
+
 /**
  * Frames and their containing elements are not to be selected at the same time.
  * Given an array of selected elements, if there are frames and their containing elements
@@ -306,7 +335,8 @@ export const getElementsWithinSelection = (
     );
   }
 
-  const selectionEdges = getSelectionEdges(selectionBounds);
+  const selectionCorners = getBoundsCorners(selectionBounds);
+  const selectionEdges = getBoundsEdges(selectionCorners);
   const elementsInSelection: NonDeletedExcalidrawElement[] = [];
 
   for (const element of elements) {
@@ -353,7 +383,19 @@ export const getElementsWithinSelection = (
     const shouldSelectFromInside = shouldTestInside(element);
 
     if (shouldSelectFromInside) {
-      if (isSelectionOverlappingElement) {
+      if (
+        isSelectionOverlappingElement &&
+        (!shouldUsePreciseFilledOverlap(element) ||
+          isSelectionContainingElement ||
+          doesSelectionOverlapFilledElement(
+            element,
+            frameBounds,
+            selectionBounds,
+            selectionCorners,
+            selectionEdges,
+            elementsMap,
+          ))
+      ) {
         elementsInSelection.push(element);
       }
       continue;

packages/excalidraw/tests/selection.test.tsx

@@ -3,7 +3,7 @@ import { vi } from "vitest";
 
 import { KEYS, ROUNDNESS, reseed } from "@excalidraw/common";
 import { getElementBounds, getElementLineSegments } from "@excalidraw/element";
-import { pointFrom, type LocalPoint } from "@excalidraw/math";
+import { pointFrom, pointRotateRads, type LocalPoint } from "@excalidraw/math";
 
 import { SHAPES } from "../components/shapes";
 
@@ -207,6 +207,42 @@ describe("box-selection overlap mode", () => {
     mouse.up();
   };
 
+  const boxSelectTopLeftAabbCorner = (
+    element: ReturnType<typeof API.createElement>,
+  ) => {
+    const sceneElement = API.getElement(element);
+    const elementsMap = h.scene.getNonDeletedElementsMap();
+    const [x1, y1] = getElementBounds(sceneElement, elementsMap);
+
+    boxSelect(x1 + 2, y1 + 2, x1 + 12, y1 + 12);
+  };
+
+  const boxSelectTopRightAabbCorner = (
+    element: ReturnType<typeof API.createElement>,
+  ) => {
+    const sceneElement = API.getElement(element);
+    const elementsMap = h.scene.getNonDeletedElementsMap();
+    const [, y1, x2] = getElementBounds(sceneElement, elementsMap);
+
+    boxSelect(x2 - 12, y1 + 2, x2 - 2, y1 + 12);
+  };
+
+  const boxSelectTopLeftRotatedLocalBoundsCorner = (
+    element: ReturnType<typeof API.createElement>,
+  ) => {
+    const sceneElement = API.getElement(element);
+    const elementsMap = h.scene.getNonDeletedElementsMap();
+    const [x1, y1, x2, y2] = getElementBounds(sceneElement, elementsMap, true);
+    const center = pointFrom((x1 + x2) / 2, (y1 + y2) / 2);
+    const [cornerX, cornerY] = pointRotateRads(
+      pointFrom(x1, y1),
+      center,
+      sceneElement.angle,
+    );
+
+    boxSelect(cornerX - 4, cornerY - 4, cornerX + 4, cornerY + 4);
+  };
+
   beforeEach(async () => {
     await render(
       <Excalidraw
@@ -327,6 +363,99 @@ describe("box-selection overlap mode", () => {
     assertSelectedElements([rect.id]);
   });
 
+  it("should not select a filled rotated rectangle when the selection box only overlaps its axis-aligned bounds", () => {
+    const rect = API.createElement({
+      type: "rectangle",
+      x: 0,
+      y: 0,
+      width: 100,
+      height: 100,
+      angle: Math.PI / 4,
+      backgroundColor: "red",
+      fillStyle: "solid",
+    });
+
+    API.setElements([rect]);
+
+    boxSelectTopLeftAabbCorner(rect);
+
+    assertSelectedElements([]);
+  });
+
+  it("should not select a filled ellipse when the selection box only overlaps its bounds corner", () => {
+    const ellipse = API.createElement({
+      type: "ellipse",
+      x: 0,
+      y: 0,
+      width: 100,
+      height: 100,
+      backgroundColor: "red",
+      fillStyle: "solid",
+    });
+
+    API.setElements([ellipse]);
+
+    boxSelectTopRightAabbCorner(ellipse);
+
+    assertSelectedElements([]);
+  });
+
+  it("should not select a filled diamond when the selection box only overlaps its bounds corner", () => {
+    const diamond = API.createElement({
+      type: "diamond",
+      x: 0,
+      y: 0,
+      width: 100,
+      height: 100,
+      backgroundColor: "red",
+      fillStyle: "solid",
+    });
+
+    API.setElements([diamond]);
+
+    boxSelectTopRightAabbCorner(diamond);
+
+    assertSelectedElements([]);
+  });
+
+  it("should not select a filled rotated ellipse when the selection box only overlaps its axis-aligned bounds", () => {
+    const ellipse = API.createElement({
+      type: "ellipse",
+      x: 0,
+      y: 0,
+      width: 100,
+      height: 100,
+      angle: Math.PI / 4,
+      backgroundColor: "red",
+      fillStyle: "solid",
+    });
+
+    API.setElements([ellipse]);
+
+    boxSelectTopLeftRotatedLocalBoundsCorner(ellipse);
+
+    assertSelectedElements([]);
+  });
+
+  it("should not select a filled rotated diamond when the selection box only overlaps its rotated local bounds", () => {
+    const diamond = API.createElement({
+      type: "diamond",
+      x: 0,
+      y: 0,
+      width: 100,
+      height: 100,
+      angle: Math.PI / 4,
+      backgroundColor: "red",
+      fillStyle: "solid",
+    });
+
+    API.setElements([diamond]);
+
+    boxSelectTopLeftRotatedLocalBoundsCorner(diamond);
+
+    assertSelectedElements([]);
+  });
+
   it("should not select rotated text when the selection box only overlaps its axis-aligned bounds", () => {
     const text = API.createElement({
       type: "text",

packages/math/src/types.ts

@@ -23,6 +23,12 @@ export type Degrees = number & { _brand: "excalimath_degree" };
  */
 export type InclusiveRange = [number, number] & { _brand: "excalimath_degree" };
 
+/**
+ * Can be used for any type of point-likes that are non-rotated, such as
+ * non-AABB Bounds, or non-rotated Point.
+ */
+export type NonRotated<T> = T & { _brand_nonrotated: "excalimath_nonrotated" };
+
 //
 // Point
 //