fix(offset): make planar wire/polygon offset sign winding-robust by andymai · Pull Request #741 · andymai/brepkit

andymai · 2026-05-29T15:37:51Z

Summary

Negative offset distance now always shrinks a planar loop and positive always grows it, regardless of the loop's traversal winding or the face normal's sign.

Previously the perpendicular convention (edge_dir x face_normal) only pointed outward when the loop wound CCW as seen from the +normal side. A clockwise-projected loop — e.g. a box bottom face whose surface normal points -Z — inverted the sign, so an inward offset of -2 on a 20x20 face expanded it to 24x24 (area 576) instead of shrinking it to 16x16 (area 256).

Fix

Both the 2D polygon path (offset_polygon_2d) and the face-based path (offset_wire) now compute the source loop's signed area relative to the offset plane normal and normalize the effective offset sign before applying the fixed perpendicular convention. A degenerate (zero-area) loop is rejected explicitly.

Tests

offset_wire::offset_cw_bottom_face_inward — CW-wound, -Z-normal 20x20 face, inward offset shrinks the loop.
polygon_offset::offset_cw_square_inward — CW polygon, inward offset shrinks.
All existing offset tests continue to pass (CCW outward/inward, arc, chamfer, legacy-match).

Negative offset distance now always shrinks a planar loop and positive always grows it, regardless of the loop's traversal winding or the face normal's sign. Previously a clockwise-projected loop (e.g. a box bottom face with a -Z normal) inverted the sign, expanding a 20x20 square to 24x24 (area 576) instead of shrinking it to 16x16 (area 256). Both the 2D polygon path (offset_polygon_2d) and the face-based path (offset_wire) now detect the source loop's signed area and normalize the effective offset sign before applying the fixed perpendicular convention.

github-actions · 2026-05-29T15:40:16Z

WASM Binary Size

File	main	PR	Delta
`brepkit_wasm.wasm`	4.70 MB	4.70 MB	+.7 KB (+0%)

⚠️ Size increased slightly.

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

greptile-apps · 2026-05-29T15:43:11Z

Greptile Summary

This PR fixes a winding-order bug in planar offset operations: both offset_polygon_2d and offset_wire_with_join now compute the loop's signed area relative to the offset plane normal and flip the effective distance when the winding is clockwise-relative-to-normal, ensuring negative distance always shrinks and positive always grows the loop.

polygon_offset.rs: Adds signed_area_2x (shoelace formula) before the offset loop and flips distance for CW polygons (area2 < 0). New test covers the CW square case correctly.
offset_wire.rs: Adds the 3D generalisation (N · Σ Vi×Vj) with the same flip logic. The added regression test (make_cw_bottom_face, CW-in-XY + N=−Z) has area2 > 0 and never enters the new branch, so it does not cover the actual bug path.

Confidence Score: 4/5

The core fix is geometrically correct and safe to merge; the main gap is that the 3D regression test doesn't cover the scenario it was designed for.

The area-sign logic in both files is mathematically sound, and the 2D regression test correctly exercises the fixed path. The 3D test constructs a CW-in-XY face with N=−Z, which gives area2 > 0 — the flip branch is never taken and the test would pass unchanged against the pre-fix code. The failure mode of CW-in-XY + N=+Z (or CCW-in-XY + N=−Z) therefore has no automated coverage.

crates/operations/src/offset_wire.rs — the regression test needs a face where the winding and normal sign disagree (area2 < 0) to genuinely guard against future regressions.

Important Files Changed

Filename	Overview
crates/math/src/polygon_offset.rs	Adds winding-robust sign correction via `signed_area_2x` (shoelace) and flips the effective offset distance for CW polygons; also adds a degenerate-area guard. The tolerance check compares an area² quantity against a linear tolerance (dimensionally inconsistent), and the public doc comment still says "(assuming CCW winding)". The new CW regression test is correct and exercises the fixed code path.
crates/operations/src/offset_wire.rs	Adds the same sign-correction logic using the 3D generalisation (N · Σ Vi×Vj). Logic is correct, but the new regression test `offset_cw_bottom_face_inward` (CW-in-XY + N=−Z) yields area2 > 0 and never enters the flipped-distance branch, so it passes with or without the fix; the actual bug path (area2 < 0) lacks test coverage.

Comments Outside Diff (1)

crates/math/src/polygon_offset.rs, line 16-17 (link)

The doc comment's (assuming CCW winding) qualifier is now stale — the fix makes the function winding-agnostic, so callers reading this will be misled.

Prompt To Fix With AI

This is a comment left during a code review.
Path: crates/math/src/polygon_offset.rs
Line: 16-17

Comment:
The doc comment's `(assuming CCW winding)` qualifier is now stale — the fix makes the function winding-agnostic, so callers reading this will be misled.



How can I resolve this? If you propose a fix, please make it concise.

Prompt To Fix All With AI

Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 3
crates/operations/src/offset_wire.rs:492-569
**3D regression test doesn't exercise the fixed code path**

`make_cw_bottom_face` constructs a CW-in-XY loop with `N = -Z`. The area2 calculation is `dot(-Z, Σ Vi×Vj)`. For a CW-in-XY polygon the z-component of `Σ Vi×Vj` is negative (−800 for this 20×20 square), so `area2 = (-1)*(−800) = 800 > 0`. The new flip branch (`area2 < 0.0`) is never taken, and `edge_dir × N` already produces correct outward normals for this combination — the test would pass without the fix.

The actual bug case is `area2 < 0`, which occurs for CW-in-XY + N=+Z (or CCW-in-XY + N=−Z). A genuine regression test needs a face where traversal winding and face normal sign disagree, e.g. a CW-wound 20×20 loop with `normal: Vec3::new(0.0, 0.0, 1.0)` — that combination triggers the flip and was the scenario that produced area 576 before the fix.

### Issue 2 of 3
crates/math/src/polygon_offset.rs:16-17
The doc comment's `(assuming CCW winding)` qualifier is now stale — the fix makes the function winding-agnostic, so callers reading this will be misled.

```suggestion
/// Positive `distance` offsets outward, negative offsets inward,
/// regardless of the polygon's winding order. Applies a miter limit of 2× the offset
```

### Issue 3 of 3
crates/math/src/polygon_offset.rs:47-50
**Dimensionally inconsistent degenerate-area guard**

`area2` is twice the signed area (units of length²), but it is compared against `tolerance`, which is a linear distance (e.g. `1e-10`). For a sliver polygon whose edge lengths are around `sqrt(tolerance) ≈ 3e-5`, `area2` can be close to `tolerance` and the polygon would be incorrectly rejected. The analogous check in `offset_wire.rs` (line 124) has the same issue against `tol.linear = 1e-7`. A length²-scaled threshold such as `tolerance * tolerance` or `tolerance * perimeter` would be dimensionally consistent.

_{Reviews (1): Last reviewed commit: "fix(offset): make wire/polygon offset si..." | Re-trigger Greptile}

greptile-apps · 2026-05-29T15:43:15Z

+    /// Helper: make a 20x20 face whose loop winds clockwise in the (x, y)
+    /// projection and whose surface normal points -Z, mirroring the bottom
+    /// face of a box. Loop order: (20,0) -> (0,0) -> (0,20) -> (20,20).
+    fn make_cw_bottom_face(topo: &mut Topology) -> brepkit_topology::face::FaceId {
+        let tol_val = 1e-7;
+        let v0 = topo.add_vertex(Vertex::new(Point3::new(20.0, 0.0, 0.0), tol_val));
+        let v1 = topo.add_vertex(Vertex::new(Point3::new(0.0, 0.0, 0.0), tol_val));
+        let v2 = topo.add_vertex(Vertex::new(Point3::new(0.0, 20.0, 0.0), tol_val));
+        let v3 = topo.add_vertex(Vertex::new(Point3::new(20.0, 20.0, 0.0), tol_val));
+
+        let e0 = topo.add_edge(Edge::new(v0, v1, EdgeCurve::Line));
+        let e1 = topo.add_edge(Edge::new(v1, v2, EdgeCurve::Line));
+        let e2 = topo.add_edge(Edge::new(v2, v3, EdgeCurve::Line));
+        let e3 = topo.add_edge(Edge::new(v3, v0, EdgeCurve::Line));
+
+        let wire = Wire::new(
+            vec![
+                OrientedEdge::new(e0, true),
+                OrientedEdge::new(e1, true),
+                OrientedEdge::new(e2, true),
+                OrientedEdge::new(e3, true),
+            ],
+            true,
+        )
+        .unwrap();
+        let wid = topo.add_wire(wire);
+
+        topo.add_face(Face::new(
+            wid,
+            vec![],
+            FaceSurface::Plane {
+                normal: Vec3::new(0.0, 0.0, -1.0),
+                d: 0.0,
+            },
+        ))
+    }
+
+    fn wire_signed_area(topo: &Topology, wid: brepkit_topology::wire::WireId) -> f64 {
+        let wire = topo.wire(wid).unwrap();
+        let pts: Vec<Point3> = wire
+            .edges()
+            .iter()
+            .map(|oe| {
+                let edge = topo.edge(oe.edge()).unwrap();
+                topo.vertex(edge.start()).unwrap().point()
+            })
+            .collect();
+        let n = pts.len();
+        let mut acc = 0.0;
+        for i in 0..n {
+            let j = (i + 1) % n;
+            acc += pts[i].x() * pts[j].y() - pts[j].x() * pts[i].y();
+        }
+        acc * 0.5
+    }
+
+    #[test]
+    fn offset_cw_bottom_face_inward() {
+        let mut topo = Topology::new();
+        let face = make_cw_bottom_face(&mut topo);
+
+        let inward = offset_wire(&mut topo, face, -2.0).unwrap();
+        let wire = topo.wire(inward).unwrap();
+        assert!(wire.is_closed());
+        assert_eq!(wire.edges().len(), 4);
+        assert!(
+            (wire_signed_area(&topo, inward).abs() - 256.0).abs() < 1e-6,
+            "CW bottom face offset -2 should enclose area 256, got {}",
+            wire_signed_area(&topo, inward).abs()
+        );
+
+        let outward = offset_wire(&mut topo, face, 2.0).unwrap();
+        assert!(
+            (wire_signed_area(&topo, outward).abs() - 576.0).abs() < 1e-6,
+            "CW bottom face offset +2 should enclose area 576, got {}",
+            wire_signed_area(&topo, outward).abs()
+        );
+    }


3D regression test doesn't exercise the fixed code path

make_cw_bottom_face constructs a CW-in-XY loop with N = -Z. The area2 calculation is dot(-Z, Σ Vi×Vj). For a CW-in-XY polygon the z-component of Σ Vi×Vj is negative (−800 for this 20×20 square), so area2 = (-1)*(−800) = 800 > 0. The new flip branch (area2 < 0.0) is never taken, and edge_dir × N already produces correct outward normals for this combination — the test would pass without the fix.

The actual bug case is area2 < 0, which occurs for CW-in-XY + N=+Z (or CCW-in-XY + N=−Z). A genuine regression test needs a face where traversal winding and face normal sign disagree, e.g. a CW-wound 20×20 loop with normal: Vec3::new(0.0, 0.0, 1.0) — that combination triggers the flip and was the scenario that produced area 576 before the fix.

Prompt To Fix With AI

This is a comment left during a code review. Path: crates/operations/src/offset_wire.rs Line: 492-569 Comment: **3D regression test doesn't exercise the fixed code path** `make_cw_bottom_face` constructs a CW-in-XY loop with `N = -Z`. The area2 calculation is `dot(-Z, Σ Vi×Vj)`. For a CW-in-XY polygon the z-component of `Σ Vi×Vj` is negative (−800 for this 20×20 square), so `area2 = (-1)*(−800) = 800 > 0`. The new flip branch (`area2 < 0.0`) is never taken, and `edge_dir × N` already produces correct outward normals for this combination — the test would pass without the fix. The actual bug case is `area2 < 0`, which occurs for CW-in-XY + N=+Z (or CCW-in-XY + N=−Z). A genuine regression test needs a face where traversal winding and face normal sign disagree, e.g. a CW-wound 20×20 loop with `normal: Vec3::new(0.0, 0.0, 1.0)` — that combination triggers the flip and was the scenario that produced area 576 before the fix. How can I resolve this? If you propose a fix, please make it concise.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

greptile-apps · 2026-05-29T15:43:19Z

+    let area2 = signed_area_2x(vertices);
+    if area2.abs() < tolerance {
+        return Err(MathError::EmptyInput);
+    }


Dimensionally inconsistent degenerate-area guard

area2 is twice the signed area (units of length²), but it is compared against tolerance, which is a linear distance (e.g. 1e-10). For a sliver polygon whose edge lengths are around sqrt(tolerance) ≈ 3e-5, area2 can be close to tolerance and the polygon would be incorrectly rejected. The analogous check in offset_wire.rs (line 124) has the same issue against tol.linear = 1e-7. A length²-scaled threshold such as tolerance * tolerance or tolerance * perimeter would be dimensionally consistent.

Prompt To Fix With AI

This is a comment left during a code review. Path: crates/math/src/polygon_offset.rs Line: 47-50 Comment: **Dimensionally inconsistent degenerate-area guard** `area2` is twice the signed area (units of length²), but it is compared against `tolerance`, which is a linear distance (e.g. `1e-10`). For a sliver polygon whose edge lengths are around `sqrt(tolerance) ≈ 3e-5`, `area2` can be close to `tolerance` and the polygon would be incorrectly rejected. The analogous check in `offset_wire.rs` (line 124) has the same issue against `tol.linear = 1e-7`. A length²-scaled threshold such as `tolerance * tolerance` or `tolerance * perimeter` would be dimensionally consistent. How can I resolve this? If you propose a fix, please make it concise.

🤖 I have created a release *beep* *boop* --- ## [2.101.2](v2.101.1...v2.101.2) (2026-05-29) ### Bug Fixes * **offset:** make planar wire/polygon offset sign winding-robust ([#741](#741)) ([fcabaeb](fcabaeb)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: brepkit[bot] <265643962+brepkit[bot]@users.noreply.github.com>

) ## Blocked on brepkit-wasm release This un-skip is **blocked** on the brepkit-wasm release that contains the upstream fix in brepkit PR andymai/brepkit#741 (`fix(offset): make planar wire/polygon offset sign winding-robust`). Keep as draft until that release is published and the `brepkit-wasm` dependency here is bumped. ## What this changes - Removes the `cannedSketches.faceOffset` entry from `tests/helpers/kernelDivergences.ts` (was: "brepkit: face offset area=576 vs expected<400 (offset not applied correctly)"). - Un-skips `sketchFaceOffset shrinks a face inward` in `tests/cannedSketches.test.ts` so it runs against both kernels, asserting the inward offset shrinks the face (area between 200 and 400; outward would yield 576). ## Background The brepkit kernel offset a box face *outward* for a negative distance (area 576 instead of 256) because the wire-offset perpendicular convention assumed CCW winding relative to the face normal. A box's bottom face winds clockwise about its outward (-Z) normal, inverting the sign. The upstream fix detects the loop's signed area relative to the plane normal and normalizes the effective offset sign. Verified locally against a brepkit-wasm build from PR #741: the test passes (brepkit area ≈ 252.6, well under 400).

Copilot AI review requested due to automatic review settings May 29, 2026 15:37

andymai enabled auto-merge (squash) May 29, 2026 15:37

Copilot started reviewing on behalf of andymai May 29, 2026 15:38 View session

andymai merged commit fcabaeb into main May 29, 2026
18 of 19 checks passed

andymai deleted the feat/parity-offset-wire-inward-direction-sign branch May 29, 2026 15:41

Copilot AI reviewed May 29, 2026

View reviewed changes

brepkit Bot mentioned this pull request May 29, 2026

chore(main): release 2.101.2 #742

Merged

greptile-apps Bot reviewed May 29, 2026

View reviewed changes

andymai mentioned this pull request May 29, 2026

test(offset): un-skip inward face-offset cross-kernel parity test andymai/brepjs#1096

Merged

andymai mentioned this pull request May 29, 2026

test: un-skip face-offset inward-shrink cross-kernel test andymai/brepjs#1098

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(offset): make planar wire/polygon offset sign winding-robust#741

fix(offset): make planar wire/polygon offset sign winding-robust#741
andymai merged 1 commit into
mainfrom
feat/parity-offset-wire-inward-direction-sign

andymai commented May 29, 2026

Uh oh!

github-actions Bot commented May 29, 2026

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

greptile-apps Bot commented May 29, 2026 •

edited

Loading

Comments Outside Diff (1)

Uh oh!

greptile-apps Bot May 29, 2026

Uh oh!

greptile-apps Bot May 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

andymai commented May 29, 2026

Summary

Fix

Tests

Uh oh!

github-actions Bot commented May 29, 2026

WASM Binary Size

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot commented May 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 4/5

Important Files Changed

Comments Outside Diff (1)

Uh oh!

greptile-apps Bot May 29, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot May 29, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

greptile-apps Bot commented May 29, 2026 •

edited

Loading