Skip to content

feat(wasm): route wire offsets through join-aware builder (offsetWire2DWithJoin)#737

Merged
andymai merged 1 commit into
mainfrom
feat/parity-offsetwire-jointype-routing
May 29, 2026
Merged

feat(wasm): route wire offsets through join-aware builder (offsetWire2DWithJoin)#737
andymai merged 1 commit into
mainfrom
feat/parity-offsetwire-jointype-routing

Conversation

@andymai
Copy link
Copy Markdown
Owner

@andymai andymai commented May 29, 2026

Summary

Adds a wire-based offset entry point, `offsetWire2DWithJoin(wire, distance, joinType)`, so consumers that hold only a wire can route a corner join type (`intersection` / `arc` / `chamfer`) into the join-aware offset builder.

Previously only the face-based `offsetWireWithJoinType` honored the join type. A wire-only caller (e.g. a 2D sketch offset that starts from a wire, not a face) had no equivalent entry point and fell back to a sharp-corner polygon offset that silently ignored the requested join. This is a developer-API parity gap: `arc` and `intersection`/`chamfer` collapsed to the same sharp result.

What changed

  • New binding `offsetWire2DWithJoin` in `crates/wasm/src/bindings/operations.rs`. It resolves the wire, builds a planar face from it internally, then delegates to `offset_wire_with_join` with the parsed join type. This reuses the existing topology-based join builder (real arcs/chamfers), not a polyline approximation.
  • Matching `executeBatch` dispatch op in `crates/wasm/src/bindings/batch.rs`.
  • Contract tests (driven through `executeBatch`) asserting:
    • the `arc` join produces a perimeter distinct from `chamfer` (proving the join type is actually threaded through), and
    • an unknown join-type string is rejected.

Validation

  • `cargo test -p brepkit-wasm` (new tests pass)
  • `cargo fmt --all`, `cargo clippy --all-targets -- -D warnings`
  • `./scripts/check-boundaries.sh`
  • pre-commit + pre-push hooks passed

Downstream

Unblocks the developer-API library from honoring its `arc`/`intersection`/`chamfer` wire-offset join kinds (the adapter currently ignores the join type for wire inputs). A follow-up consumer PR (kept in draft, blocked on the next published wasm release containing this change) removes the corresponding cross-kernel skip.

Expose offsetWire2DWithJoin(wire, distance, joinType) so consumers that
hold only a wire can route a corner join type (intersection / arc /
chamfer) into the join-aware offset builder. Previously only the
face-based offsetWireWithJoinType honored the join type; a wire-only
caller had to fall back to a sharp-corner polygon offset that silently
ignored the requested join. The new binding builds a planar face from
the wire internally, then delegates to offset_wire_with_join.

Adds the matching executeBatch dispatch op and contract tests proving
the arc join produces a distinct perimeter from chamfer and that an
unknown join type is rejected.
Copilot AI review requested due to automatic review settings May 29, 2026 13:25
@andymai andymai enabled auto-merge (squash) May 29, 2026 13:25
@github-actions
Copy link
Copy Markdown
Contributor

WASM Binary Size

File main PR Delta
brepkit_wasm.wasm 4.69 MB 4.69 MB +3.0 KB (+0%)

⚠️ Size increased slightly.

@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented May 29, 2026

Greptile Summary

Adds offsetWire2DWithJoin, a new wire-based entry point that internally constructs a planar face and delegates to the existing join-aware offset builder, closing the API parity gap where wire-only callers could not route a join type into the topology-based offset path.

  • New #[wasm_bindgen(js_name = \"offsetWire2DWithJoin\")] binding in operations.rs follows the existing offsetWireWithJoinType pattern; parse_join_type_str rejects unknown join strings with a clean error.
  • Matching dispatch_op arm in batch.rs is consistent with existing offsetWire/offsetWireWithJoinType arms.
  • Two contract tests (via executeBatch) verify that arc and chamfer joins produce distinct perimeters and that unknown join-type strings are rejected.

Confidence Score: 4/5

Safe to merge; the change is additive, tests pass, and the only gap is a missing early-validation guard on the distance parameter.

The new binding follows the established pattern for wire offset operations and the logic is correct. The one gap is that distance is not checked with validate_finite before being forwarded into the geometry kernel, unlike most other numeric-taking bindings in the file. A NaN or infinity distance would reach the offset algorithm rather than being caught at the binding boundary.

crates/wasm/src/bindings/operations.rs — the missing validate_finite(distance, "distance") call before the wire resolve.

Important Files Changed

Filename Overview
crates/wasm/src/bindings/operations.rs Adds offset_wire_2d_with_join WASM binding with correct js_name, error mapping, and doc-comments; distance is not validated with validate_finite before use (binding-rule gap).
crates/wasm/src/bindings/batch.rs Adds offsetWire2DWithJoin arm to dispatch_op; logic mirrors the direct binding and is consistent with existing offsetWire/offsetWireWithJoinType dispatch arms.

Fix All in Claude Code

Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
crates/wasm/src/bindings/operations.rs:1208-1214
Missing `validate_finite` on `distance`. The binding rules require every public `#[wasm_bindgen]` method to validate numeric inputs via helpers from `error.rs`. Passing `NaN` or `±Inf` here will propagate silently into `offset_wire_with_join`, which can produce garbage geometry or a panic inside the kernel instead of a clean JS error.

```suggestion
    pub fn offset_wire_2d_with_join(
        &mut self,
        wire: u32,
        distance: f64,
        join_type: &str,
    ) -> Result<u32, JsError> {
        validate_finite(distance, "distance")?;
        let wire_id = self.resolve_wire(wire)?;
```

Reviews (1): Last reviewed commit: "feat(wasm): add wire-based offsetWire2DW..." | Re-trigger Greptile

Comment on lines +1208 to +1214
pub fn offset_wire_2d_with_join(
&mut self,
wire: u32,
distance: f64,
join_type: &str,
) -> Result<u32, JsError> {
let wire_id = self.resolve_wire(wire)?;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Missing validate_finite on distance. The binding rules require every public #[wasm_bindgen] method to validate numeric inputs via helpers from error.rs. Passing NaN or ±Inf here will propagate silently into offset_wire_with_join, which can produce garbage geometry or a panic inside the kernel instead of a clean JS error.

Suggested change
pub fn offset_wire_2d_with_join(
&mut self,
wire: u32,
distance: f64,
join_type: &str,
) -> Result<u32, JsError> {
let wire_id = self.resolve_wire(wire)?;
pub fn offset_wire_2d_with_join(
&mut self,
wire: u32,
distance: f64,
join_type: &str,
) -> Result<u32, JsError> {
validate_finite(distance, "distance")?;
let wire_id = self.resolve_wire(wire)?;
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/wasm/src/bindings/operations.rs
Line: 1208-1214

Comment:
Missing `validate_finite` on `distance`. The binding rules require every public `#[wasm_bindgen]` method to validate numeric inputs via helpers from `error.rs`. Passing `NaN` or `±Inf` here will propagate silently into `offset_wire_with_join`, which can produce garbage geometry or a panic inside the kernel instead of a clean JS error.

```suggestion
    pub fn offset_wire_2d_with_join(
        &mut self,
        wire: u32,
        distance: f64,
        join_type: &str,
    ) -> Result<u32, JsError> {
        validate_finite(distance, "distance")?;
        let wire_id = self.resolve_wire(wire)?;
```

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

Fix in Claude Code

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a wire-only WASM entry point that preserves corner join semantics (intersection/arc/chamfer) by internally constructing a planar face from a wire and delegating to the existing join-aware offset builder. This closes an API parity gap where wire-only consumers could not select join style and effectively got sharp corners regardless of request.

Changes:

  • Added offsetWire2DWithJoin(wire, distance, joinType) binding that routes join type through offset_wire_with_join.
  • Added matching executeBatch dispatcher op ("offsetWire2DWithJoin").
  • Added contract tests (via executeBatch) to verify join-type threading and rejection of unknown join types.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
crates/wasm/src/bindings/operations.rs Introduces offsetWire2DWithJoin binding and adds tests validating join behavior and error handling.
crates/wasm/src/bindings/batch.rs Adds batch dispatcher support for "offsetWire2DWithJoin" with consistent argument parsing and error mapping.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@andymai andymai merged commit cd41676 into main May 29, 2026
19 checks passed
@andymai andymai deleted the feat/parity-offsetwire-jointype-routing branch May 29, 2026 13:28
@brepkit brepkit Bot mentioned this pull request May 29, 2026
brepkit Bot added a commit that referenced this pull request May 29, 2026
🤖 I have created a release *beep* *boop*
---


##
[2.101.0](v2.100.0...v2.101.0)
(2026-05-29)


### Features

* **wasm:** route wire offsets through join-aware builder
(offsetWire2DWithJoin)
([#737](#737))
([cd41676](cd41676))

---
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>
andymai added a commit to andymai/brepjs that referenced this pull request May 29, 2026
…tion/chamfer) (#1089)

## Summary

Routes \`offsetWire2D\`'s \`joinType\` through the brepkit kernel's
join-aware wire-offset builder instead of the sharp-corner polygon
offset that silently ignored it.

The brepkit adapter previously built a 2D polygon from the wire and
called \`offsetPolygon2d\`, which always produces sharp/miter corners —
so \`arc\`, \`intersection\`, and \`chamfer\` all collapsed to the same
result. This is a developer-API parity gap.

## What changed

- \`src/kernel/brepkit/modifierOps.ts\`: \`offsetWire2D\` now calls the
new \`offsetWire2DWithJoin(wire, distance, joinType)\` kernel binding
when available, mapping the public kind
(\`arc\`/\`intersection\`/\`tangent\`; \`chamfer\` is pre-mapped to
\`intersection\` upstream) to the kernel's join-type strings. Falls back
to the legacy polygon path on older wasm builds.
- \`src/kernel/brepkit/brepkitWasmTypes.ts\`: declares the optional
\`offsetWire2DWithJoin\` binding.
- \`tests/helpers/kernelDivergences.ts\`: removes the
\`offsetWire2D.chamferJoin\` divergence entry (the kernel now implements
the join types).
- \`tests/offsetWire2D.test.ts\`: un-skips the suite and drops the
reference-kernel-specific naming — it is now a normal cross-kernel test.

## Blocked on

This depends on the brepkit-wasm release that ships the
\`offsetWire2DWithJoin\` binding.

- brepkit PR: andymai/brepkit#737

Until that wasm version is published and bumped here, CI will run the
un-skipped \`tests/offsetWire2D.test.ts\` against the older wasm (which
lacks the binding) and the \`arc differs from chamfer\` assertion will
fail — hence this PR is kept in **draft**. Locally validated against a
wasm build from PR #737 (all 9 offsetWire2D tests pass across both
kernels; full suite + typecheck + lint + knip green).

Do not merge until the brepkit-wasm dependency is bumped.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants