From ad22513bc7baae76a1ad17ff01d5999636359b3d Mon Sep 17 00:00:00 2001 From: Joseph Cumines Date: Fri, 19 Jun 2026 11:38:38 +1000 Subject: [PATCH] fix(tools): address CodeRabbit review on analyze_overlaps - Export OverlapScope, AnalyzeOverlapsSummary, and OverlapIntersection from the analyze barrel so consumers don't deep-import. - Expand the local rectangle by stroke overflow before transforming through the world matrix, so stroked rotated nodes get their true rotated footprint instead of an underestimated canvas-space expansion. - Add clipPolygon to geometry and preserve the clipped polygon through the full clip chain in computeNodeBounds, so a later outer clip can't reintroduce corners an inner clip already removed. - Cap returned overlaps to an empty list when limit is 0 or negative; summary totals still reflect the complete result set. - Trim whitespace from scope and severity inputs so " major " resolves. - Add geometry and overlap tests covering each fix. --- CHANGELOG.md | 5 + packages/core/src/geometry.ts | 64 +++++++---- packages/core/src/tools/analyze.ts | 3 + .../src/tools/analyze/overlaps/helpers.ts | 54 +++++---- .../core/src/tools/analyze/overlaps/index.ts | 4 +- .../core/src/tools/analyze/overlaps/params.ts | 6 +- tests/engine/geometry/clip-polygon.test.ts | 107 ++++++++++++++++++ .../tools/analyze/overlaps/bounds.test.ts | 56 +++++++++ .../tools/analyze/overlaps/index.test.ts | 45 ++++++++ 9 files changed, 298 insertions(+), 46 deletions(-) create mode 100644 tests/engine/geometry/clip-polygon.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index bab5146e3..635532f5e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,11 @@ - Preserve rotated Figma transform origins for imported vector nodes. - Render complex text fills through vector glyph outlines so imported Figma text can use the normal fill pipeline for gradients, images, patterns, and other non-solid paints. - Fix file-backed CLI commands (`convert`, `eval --output`, `export`) to use Node `fs/promises` instead of Bun runtime APIs, so the published CLI works when installed and run under Node. +- Fix `analyze_overlaps` stroke-overflow bounds for rotated nodes by expanding the local rectangle before transforming through the world matrix, so stroked rotated nodes are measured with their true rotated footprint. +- Fix `analyze_overlaps` clipping across multiple rotated ancestors by preserving the clipped polygon through the full clip chain instead of collapsing to an AABB between clips, which could reintroduce corners removed by an inner clip. +- Fix `analyze_overlaps` `limit` of `0` (or a negative value) so it caps the returned overlaps to an empty list instead of returning the full set; summary totals still reflect the complete result. +- Trim whitespace from `analyze_overlaps` `scope` and `severity` inputs so values like `" major "` resolve instead of falling back to defaults. +- Export `OverlapScope`, `AnalyzeOverlapsSummary`, and `OverlapIntersection` from the `@open-pencil/core/tools/analyze` barrel so consumers do not need to deep-import the overlaps module. ## 0.13.2 — 2026-05-30 diff --git a/packages/core/src/geometry.ts b/packages/core/src/geometry.ts index d13e44f96..7355d90a0 100644 --- a/packages/core/src/geometry.ts +++ b/packages/core/src/geometry.ts @@ -438,22 +438,20 @@ function clipHalfPlane(polygon: Vector[], a: Vector, b: Vector, wantPositive: bo } /** - * Clip an axis-aligned VisualBounds rectangle against a convex polygon - * (e.g. the 4 canvas-space corners of a rotated clipping ancestor). + * Clip a subject polygon against a convex polygon (e.g. the 4 canvas-space + * corners of a rotated clipping ancestor). * * Uses Sutherland–Hodgman polygon clipping with centroid-based interior * detection, making it robust to either winding order of the clip polygon. - * Returns the AABB of the intersection, or null if the bounds are fully - * outside the clip polygon. + * Returns the clipped polygon, or null if the subject is fully outside the + * clip polygon. When `clipCorners` has fewer than 3 points the subject is + * returned unchanged (no clipping). * - * For a non-rotated clip (axis-aligned corners) the result is identical - * to `intersectVisualBounds`. + * Preserving the polygon (rather than collapsing to an AABB) lets callers + * chain multiple clips without reintroducing corners removed by an inner clip. */ -export function clipBoundsToPolygon( - bounds: VisualBounds, - clipCorners: Vector[] -): VisualBounds | null { - if (clipCorners.length < 3) return bounds +export function clipPolygon(subject: Vector[], clipCorners: Vector[]): Vector[] | null { + if (clipCorners.length < 3) return subject let cx = 0 let cy = 0 @@ -464,28 +462,50 @@ export function clipBoundsToPolygon( cx /= clipCorners.length cy /= clipCorners.length - let subject: Vector[] = [ + let polygon: Vector[] = subject + + for (let i = 0; i < clipCorners.length; i++) { + if (polygon.length === 0) return null + const a = clipCorners[i] + const b = clipCorners[(i + 1) % clipCorners.length] + const centroidCross = crossProduct(a, b, { x: cx, y: cy }) + polygon = clipHalfPlane(polygon, a, b, centroidCross >= 0) + } + + return polygon.length === 0 ? null : polygon +} + +/** + * Clip an axis-aligned VisualBounds rectangle against a convex polygon + * (e.g. the 4 canvas-space corners of a rotated clipping ancestor). + * + * Returns the AABB of the intersection, or null if the bounds are fully + * outside the clip polygon. Delegates to {@link clipPolygon}. + * + * For a non-rotated clip (axis-aligned corners) the result is identical + * to `intersectVisualBounds`. + */ +export function clipBoundsToPolygon( + bounds: VisualBounds, + clipCorners: Vector[] +): VisualBounds | null { + if (clipCorners.length < 3) return bounds + + const subject: Vector[] = [ { x: bounds.minX, y: bounds.minY }, { x: bounds.maxX, y: bounds.minY }, { x: bounds.maxX, y: bounds.maxY }, { x: bounds.minX, y: bounds.maxY } ] - for (let i = 0; i < clipCorners.length; i++) { - if (subject.length === 0) return null - const a = clipCorners[i] - const b = clipCorners[(i + 1) % clipCorners.length] - const centroidCross = crossProduct(a, b, { x: cx, y: cy }) - subject = clipHalfPlane(subject, a, b, centroidCross >= 0) - } - - if (subject.length === 0) return null + const polygon = clipPolygon(subject, clipCorners) + if (!polygon) return null let minX = Infinity let minY = Infinity let maxX = -Infinity let maxY = -Infinity - for (const p of subject) { + for (const p of polygon) { if (p.x < minX) minX = p.x if (p.y < minY) minY = p.y if (p.x > maxX) maxX = p.x diff --git a/packages/core/src/tools/analyze.ts b/packages/core/src/tools/analyze.ts index 0b2d13571..f1d414f8d 100644 --- a/packages/core/src/tools/analyze.ts +++ b/packages/core/src/tools/analyze.ts @@ -7,9 +7,12 @@ export { analyzeOverlaps, computeOverlaps } from './analyze/overlaps' export type { AnalyzeOverlapsArgs, AnalyzeOverlapsResult, + AnalyzeOverlapsSummary, OverlapCategory, + OverlapIntersection, OverlapItem, OverlapNodeSummary, + OverlapScope, OverlapSeverity } from './analyze/overlaps' export { analyzeSpacing } from './analyze/spacing' diff --git a/packages/core/src/tools/analyze/overlaps/helpers.ts b/packages/core/src/tools/analyze/overlaps/helpers.ts index 637ecf841..717349714 100644 --- a/packages/core/src/tools/analyze/overlaps/helpers.ts +++ b/packages/core/src/tools/analyze/overlaps/helpers.ts @@ -1,7 +1,7 @@ import { getWorldMatrix } from '#core/canvas/coordinate' import Matrix from '#core/canvas/matrix' import { - clipBoundsToPolygon, + clipPolygon, effectOverflow, geometryBlobBounds, intersectVisualBounds, @@ -274,15 +274,20 @@ function aabbFromCorners(corners: Vector[]): VisualBounds { */ function computeNodeVisualBounds(node: SceneNode, graph: SceneGraph): VisualBounds { const matrix = getWorldMatrix(node, graph) + // Expand the local rectangle by the stroke overflow *before* transforming + // through the world matrix. Expanding the already-rotated AABB by `stroke` + // underestimates the stroked bounds for rotated nodes (the true stroked AABB + // grows by `stroke * (|cos θ| + |sin θ|)`, not `stroke`). + const stroke = strokeOverflow(node.strokes) const baseCorners = Matrix.mapPoints(matrix, [ - 0, - 0, - node.width, - 0, - node.width, - node.height, - 0, - node.height + -stroke, + -stroke, + node.width + stroke, + -stroke, + node.width + stroke, + node.height + stroke, + -stroke, + node.height + stroke ]) let bounds = aabbFromCorners([ { x: baseCorners[0], y: baseCorners[1] }, @@ -291,14 +296,8 @@ function computeNodeVisualBounds(node: SceneNode, graph: SceneGraph): VisualBoun { x: baseCorners[6], y: baseCorners[7] } ]) - const stroke = strokeOverflow(node.strokes) - if (stroke > 0) { - bounds.minX -= stroke - bounds.minY -= stroke - bounds.maxX += stroke - bounds.maxY += stroke - } - + // Effects (drop shadow, blur) radiate in screen space, so expanding the + // canvas-space AABB by the directional overflow is correct regardless of rotation. const effects = effectOverflow(node.effects) bounds.minX -= effects.left bounds.minY -= effects.top @@ -369,15 +368,28 @@ export function computeNodeBounds( node: SceneNode, graph: SceneGraph ): { bounds: VisualBounds; area: number } { - let bounds = computeNodeVisualBounds(node, graph) + const visual = computeNodeVisualBounds(node, graph) const clips = collectClipChain(graph, node) + if (clips.length === 0) { + return { bounds: visual, area: visualBoundsArea(visual) } + } + // Seed the subject polygon from the visual AABB, then clip against each + // ancestor preserving the polygon. Collapsing to an AABB between clips would + // reintroduce corners already removed by an inner clip, so hidden regions + // could look visible again to a later outer clip. + let polygon: Vector[] | null = [ + { x: visual.minX, y: visual.minY }, + { x: visual.maxX, y: visual.minY }, + { x: visual.maxX, y: visual.maxY }, + { x: visual.minX, y: visual.maxY } + ] for (const clip of clips) { - const clipped = clipBoundsToPolygon(bounds, clip) - if (!clipped) { + polygon = clipPolygon(polygon, clip) + if (!polygon) { return { bounds: EMPTY_BOUNDS, area: 0 } } - bounds = clipped } + const bounds = aabbFromCorners(polygon) return { bounds, area: visualBoundsArea(bounds) } } diff --git a/packages/core/src/tools/analyze/overlaps/index.ts b/packages/core/src/tools/analyze/overlaps/index.ts index 6aef894e3..3316c3e2e 100644 --- a/packages/core/src/tools/analyze/overlaps/index.ts +++ b/packages/core/src/tools/analyze/overlaps/index.ts @@ -303,7 +303,9 @@ export function computeOverlaps( ) const limit = Math.max(0, Number.isFinite(Number(args.limit)) ? Number(args.limit) : 100) - const trimmed = limit > 0 ? sorted.slice(0, limit) : sorted + // A limit of 0 (or a negative value clamped to 0) caps the returned overlaps + // to an empty list. The summary totals still reflect the full `sorted` set. + const trimmed = sorted.slice(0, limit) const byCategory = emptyByCategory() const bySeverity = emptyBySeverity() diff --git a/packages/core/src/tools/analyze/overlaps/params.ts b/packages/core/src/tools/analyze/overlaps/params.ts index f1844c5c1..ba5048703 100644 --- a/packages/core/src/tools/analyze/overlaps/params.ts +++ b/packages/core/src/tools/analyze/overlaps/params.ts @@ -23,7 +23,8 @@ export const VALID_OVERLAP_SEVERITIES: readonly OverlapSeverity[] = [ export function parseOverlapScope(raw: string | undefined): OverlapScope | undefined { if (!raw) return undefined - const normalized = raw.toLowerCase() + const normalized = raw.trim().toLowerCase() + if (!normalized) return undefined return VALID_OVERLAP_SCOPES.find((scope) => scope === normalized) } @@ -42,6 +43,7 @@ export function parseOverlapCategories(raw: string | undefined): OverlapCategory export function parseOverlapSeverity(raw: string | undefined): OverlapSeverity | undefined { if (!raw) return undefined - const normalized = raw.toLowerCase() + const normalized = raw.trim().toLowerCase() + if (!normalized) return undefined return VALID_OVERLAP_SEVERITIES.find((severity) => severity === normalized) } diff --git a/tests/engine/geometry/clip-polygon.test.ts b/tests/engine/geometry/clip-polygon.test.ts new file mode 100644 index 000000000..b9edfde83 --- /dev/null +++ b/tests/engine/geometry/clip-polygon.test.ts @@ -0,0 +1,107 @@ +import { describe, expect, test } from 'bun:test' + +import type { Vector } from '@open-pencil/core' + +import { clipBoundsToPolygon, clipPolygon } from '#core/geometry' + +describe('clipPolygon', () => { + test('returns the subject unchanged when the clip has fewer than 3 corners', () => { + const subject: Vector[] = [ + { x: 0, y: 0 }, + { x: 10, y: 0 }, + { x: 10, y: 10 }, + { x: 0, y: 10 } + ] + expect(clipPolygon(subject, [])).toBe(subject) + expect( + clipPolygon(subject, [ + { x: 0, y: 0 }, + { x: 1, y: 1 } + ]) + ).toBe(subject) + }) + + test('returns null when the subject is fully outside the clip', () => { + const subject: Vector[] = [ + { x: 100, y: 100 }, + { x: 110, y: 100 }, + { x: 110, y: 110 }, + { x: 100, y: 110 } + ] + const clip: Vector[] = [ + { x: 0, y: 0 }, + { x: 10, y: 0 }, + { x: 10, y: 10 }, + { x: 0, y: 10 } + ] + expect(clipPolygon(subject, clip)).toBeNull() + }) + + test('preserves the clipped polygon across a chain so a later clip excludes corners the inner clip removed', () => { + // Subject: the full [0,100]² square. + const subject: Vector[] = [ + { x: 0, y: 0 }, + { x: 100, y: 0 }, + { x: 100, y: 100 }, + { x: 0, y: 100 } + ] + + // Inner clip D1: the diamond inscribed in the square (corners at the edge + // midpoints). It removes the four square corners, including (0,0). + const d1: Vector[] = [ + { x: 50, y: 0 }, + { x: 100, y: 50 }, + { x: 50, y: 100 }, + { x: 0, y: 50 } + ] + + // Outer clip D2: a small square [0,30]² covering the (0,0) corner that D1 + // already removed. If the chain collapsed D1's result to its AABB ([0,100]², + // which re-adds the corner) before applying D2, the corner would survive. + // Preserving the polygon keeps it excluded: the only surviving region is the + // triangle where the diamond's x+y=50 edge meets [0,30]², i.e. AABB + // [20,30]×[20,30]. + const d2: Vector[] = [ + { x: 0, y: 0 }, + { x: 30, y: 0 }, + { x: 30, y: 30 }, + { x: 0, y: 30 } + ] + + const afterD1 = clipPolygon(subject, d1) + expect(afterD1).not.toBeNull() + const clipped = afterD1 ? clipPolygon(afterD1, d2) : null + expect(clipped).not.toBeNull() + expect(clipped?.length).toBeGreaterThanOrEqual(3) + + let minX = Infinity + let minY = Infinity + let maxX = -Infinity + let maxY = -Infinity + for (const p of clipped ?? []) { + if (p.x < minX) minX = p.x + if (p.y < minY) minY = p.y + if (p.x > maxX) maxX = p.x + if (p.y > maxY) maxY = p.y + } + + // The corner (0,0) must stay excluded: the result does not reach below 20. + expect(minX).toBeGreaterThanOrEqual(20) + expect(minY).toBeGreaterThanOrEqual(20) + expect(maxX).toBeLessThanOrEqual(30) + expect(maxY).toBeLessThanOrEqual(30) + }) + + test('clipBoundsToPolygon agrees with clipPolygon for a single axis-aligned clip', () => { + const bounds = { minX: 0, minY: 0, maxX: 100, maxY: 100 } + const clip: Vector[] = [ + { x: 25, y: 25 }, + { x: 75, y: 25 }, + { x: 75, y: 75 }, + { x: 25, y: 75 } + ] + + const aabb = clipBoundsToPolygon(bounds, clip) + expect(aabb).toEqual({ minX: 25, minY: 25, maxX: 75, maxY: 75 }) + }) +}) diff --git a/tests/engine/tools/analyze/overlaps/bounds.test.ts b/tests/engine/tools/analyze/overlaps/bounds.test.ts index 5430f6003..c08cc9381 100644 --- a/tests/engine/tools/analyze/overlaps/bounds.test.ts +++ b/tests/engine/tools/analyze/overlaps/bounds.test.ts @@ -178,4 +178,60 @@ describe('analyze overlaps visible bounds', () => { const result = computeOverlaps(graph, { category: 'sibling-overlap' }) expect(result.summary.overlapCount).toBe(0) }) + + test('a stroked rotated node expands bounds along the rotated axes', () => { + const graph = new SceneGraph() + const page = pageId(graph) + + // 10x10 rectangle at (45,45) rotated 45° around its center (50,50). Its + // unstroked canvas AABB is [42.93, 57.07]² (half-diagonal ≈ 7.07). + // An OUTSIDE stroke of weight 20 expands the LOCAL box by 20 on each side; + // after 45° rotation the canvas-space expansion is 20*(|cos45|+|sin45|) ≈ + // 28.28 per side, so the stroked AABB is [14.64, 85.36]². The OLD code + // expanded the already-rotated AABB by 20 → only [22.93, 77.07]². + const stroked = graph.createNode('RECTANGLE', page, { + name: 'RotatedStroked', + x: 45, + y: 45, + width: 10, + height: 10, + rotation: 45, + strokes: [ + { + color: { r: 0, g: 0, b: 0, a: 1 }, + weight: 20, + opacity: 1, + visible: true, + align: 'OUTSIDE' + } + ] + }) + + // Target at canvas (18, 48, 2, 2): inside the rotation-aware stroked AABB + // (18 > 14.64) but outside both the unstroked AABB (18 < 42.93) and the + // OLD naive expansion (18 < 22.93). Only the rotation-aware stroke reaches it. + rect(graph, 'GapTarget', page, 18, 48, 2, 2) + + const result = computeOverlaps(graph, { category: 'sibling-overlap' }) + expect(result.summary.overlapCount).toBeGreaterThan(0) + expect( + result.overlaps.some((o) => o.nodeA.id === stroked.id || o.nodeB.id === stroked.id) + ).toBe(true) + + // Without the stroke, the same target must NOT overlap — proving the reach + // into the gap is caused by the stroked bounds, not the raw node box. + const plain = new SceneGraph() + const plainPage = pageId(plain) + graph.createNode('RECTANGLE', plainPage, { + name: 'RotatedPlain', + x: 45, + y: 45, + width: 10, + height: 10, + rotation: 45 + }) + rect(plain, 'GapTarget', plainPage, 18, 48, 2, 2) + const plainResult = computeOverlaps(plain, { category: 'sibling-overlap' }) + expect(plainResult.summary.overlapCount).toBe(0) + }) }) diff --git a/tests/engine/tools/analyze/overlaps/index.test.ts b/tests/engine/tools/analyze/overlaps/index.test.ts index 14acfb329..825de49fa 100644 --- a/tests/engine/tools/analyze/overlaps/index.test.ts +++ b/tests/engine/tools/analyze/overlaps/index.test.ts @@ -315,6 +315,23 @@ describe('analyze overlaps', () => { expect(result.overlaps.length).toBeGreaterThan(0) }) + test('scope and severity inputs are trimmed of surrounding whitespace', () => { + const graph = new SceneGraph() + const page = pageId(graph) + rect(graph, 'A', page, 0, 0, 100, 100) + rect(graph, 'B', page, 50, 50, 100, 100) + + const scoped = computeOverlaps(graph, { scope: ' same-parent ' as OverlapScope }) + expect(scoped.overlaps.length).toBeGreaterThan(0) + + const severityCapped = computeOverlaps(graph, { + scope: ' same-parent ' as OverlapScope, + severity: ' minor ' as OverlapSeverity + }) + // Both overlaps are minor sibling-overlaps, so trimming severity keeps them. + expect(severityCapped.overlaps.length).toBe(scoped.overlaps.length) + }) + test('ToolDef defaults to current page and does not report overlaps on other pages', () => { const graph = new SceneGraph() const page2 = graph.addPage('Page 2') @@ -417,4 +434,32 @@ describe('analyze overlaps', () => { const totalBySeverity = Object.values(result.summary.bySeverity).reduce((a, b) => a + b, 0) expect(totalBySeverity).toBe(result.summary.overlapCount) }) + + test('limit of zero returns no overlaps while the summary still reports the full set', () => { + const graph = new SceneGraph() + const page = pageId(graph) + const parent = frame(graph, 'Frame', page, 0, 0, 200, 200) + + for (let i = 0; i < 4; i++) { + rect(graph, `R${i}`, parent.id, 0, 0, 100, 100) + } + + const result = computeOverlaps(graph, { scope: 'inside-parent', limit: 0 }) + expect(result.overlaps).toHaveLength(0) + expect(result.summary.overlapCount).toBeGreaterThan(0) + }) + + test('a negative limit is clamped to zero and returns no overlaps', () => { + const graph = new SceneGraph() + const page = pageId(graph) + const parent = frame(graph, 'Frame', page, 0, 0, 200, 200) + + for (let i = 0; i < 4; i++) { + rect(graph, `R${i}`, parent.id, 0, 0, 100, 100) + } + + const result = computeOverlaps(graph, { scope: 'inside-parent', limit: -3 }) + expect(result.overlaps).toHaveLength(0) + expect(result.summary.overlapCount).toBeGreaterThan(0) + }) })