You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
perf(boolean): a proto vertex with proto edges is not solved against the proto edges (#3260) - #3263
With proto_vertex_miss_is_false on, a boolean no longer solves a proto vertex against a proto edge when the vertex has proto edges of its own: 0 point solves in place of 4N² − 2N for a plate minus N cylinders cut in one call. Every boolean that returned a body returns the same body. What a kernel cancelled mid-operation reports can differ (below).
Related: #3248, pass B (proto vertices against proto edges). That issue proposes a box index so fewer pairs are visited; this PR is the other half and does not overlap with it: for the pairs that are visited, the solve is not run when the proto vertex has proto edges. Not measured on #3248's case (whether its proto vertices have proto edges of their own decides how much of its pass B this removes).
The problem
After its face pairs, a boolean tests every proto vertex against every proto edge (RemoveCoincidentNoEdgeProtoVertex). A vertex found on an edge is removed only when it has no proto edges of its own. The point solve ran first all the same, for every pair.
Cutting N cylinders out of a plate in one boolean_lists call makes 2N section circles: 2N proto edges, each closed on its own proto vertex. Every vertex has an edge, so nothing can be removed, and 4N² − 2N solves ran for nothing.
The change
topology/proto_topology/p1/proto.rs, remove_coincident_no_edge_proto_vertex: with proto_vertex_miss_is_false (WR-73, #848) on, a miss answers false, and a hit on a vertex that has proto edges answers false too. No outcome of the solve gives another answer for such a vertex, so the function returns false before the solve. The checks before it (a deleted object, a missing curve) stay, in the same order.
Under an already-fired cancel token the solve still runs, so from the same caches the function returns what it returned (the solve is where this step could notice the cancel).
With the flag off nothing changes. There a miss answers true, which ends the caller's loop, so the solve does decide something. That path goes when the flag is retired.
WorkCounters::proto_vertex_point_solves counts each solve, for the test.
No new flag, no cache, no second attempt.
Counts
Plate 20 n x 20 n x 12 minus N = n² cylinders r 4 in one boolean_lists call, SolidCraft's fixes:
N
Solves before
Solves after
25
2 450
0
100
39 800
0
Law
4N² − 2N
0
The result has N + 6 faces, 3N + 12 edges and its exact volume, before and after.
What changes in the result
Bodies: nothing. Every boolean that returned a body returns the same body. Two independent reviews compared old against new: 384 booleans with 492 returned bodies byte for byte equal (native serialization, topology and geometry dumps, volume bits, validation reports) and all 32 errors equal, fixes on and off; a boolean where an edge-less proto vertex is removed and one where it is kept, noncircular ellipse sections included.
Cancellation: the label of a Cancelled error can differ. Skipping a solve also skips warming its curve's cache. A kernel cancelled later can therefore notice the cancel at a different check: CurveCache::tessellate where it was geom::global_solver::solve_branch.
With an already-fired token and equal starting caches, this function runs the old solve and returns what it returned.
Across call sequences that warm caches differently, even the outcome of a later direct call of this low-level function can differ: for a straight degree-1 B-spline, a second call after the token fires returned Ok(false) with the old solve's cache warming and Cancelled without it. The manager's state is unchanged either way.
No boolean was found to change between success and error: 1,536 cancellation probes (tokens fired before the call, kernels reused across booleans, cancellation from another thread) gave the same kind of result. The boolean polls the token again after this step (resolve_intersections).
Cancellation is cooperative: a boolean that finishes sooner can finish before a cancel signal arrives.
A deferral nobody could reach. An Unported raised only inside a solve that is no longer run, and whose answer was not read, would no longer end the boolean there. No boolean that reaches one is known: the site once suspected is not reachable, because the curve cache is built from a copy detached from its owner.
If a changed Cancelled label counts as a changed result here, say so and I will withdraw this: it cannot be kept identical without running the solve.
Tests
plate_cut_solves_no_proto_vertex_that_has_proto_edges (committed first, failing with (2450, 39800) against (0, 0)).
a_proto_vertex_with_proto_edges_is_not_solved: 0 solves for a vertex with edges on the curve or off it, 1 for an edge-less one, and under an already-fired cancel token the solve run for every vertex (3 solves, the old answers).
Verification
On 92df6735a, rebased on main dc5cfd7e7, Linux (WSL Ubuntu 22.04):
Goldens, --all --goldens: goldens: 283 unit(s), 282 passed, 1 failed. The one is curve.conic case gen2235 (values.r[4]: 3.776586443654626e-14 -> 3.552713678800501e-14), which differs the same way on unchanged main on that machine (checked on ba55b2f71 and on 7b29ffdb5); this change does not touch it.
The count test at the test commit fails with (2450, 39800); at the head it passes with (0, 0). boolean_api, boolean_no_dangling_vertex_refs, issue_1120_boolean_slits pass. The focused lib tests (topology::proto_topology, boolean::topology_intersector, boolean::merge, kernel::tests) pass: 134.
On the earlier base ba55b2f71, together with four other changes on one branch: the whole workspace suite (7181 passed, 113 skipped) and the kernel rig as CI runs it (1081 cases, 803 pass, 0 fail, 15 known-failing, none moved). Not re-run on this head: CI is the gate.
Not attached
A schematic. The mechanism is a computation whose answer is not read; there is no geometry to draw that a picture of the plate does not already show, and I have no VectorCraft figure of it. Say if you want one before merging.
Not in this PR
The same loop still visits every pair (4N² cheap visits); only the solves are gone.
Proto edge pairs are still intersected pairwise (2N² − N).
Vertices connected to existing edges can skip unnecessary point checks, while vertices without edges are still checked. This can reduce redundant work in solid boolean operations, including when cutting multiple holes through a plate.
Added coverage for cancellation behavior and for verifying that perforated plates retain the expected geometry.
No actionable comments were generated in the recent review.
Walkthrough
The change adds a per-kernel counter for proto-vertex point solves. When proto_vertex_miss_is_false is enabled and a proto vertex has incident proto edges, the method skips the solve unless cancellation has already been detected. Tests cover this behavior and 3D perforated-plate cases.
Changes: Boolean proto-vertex point solves
Layer / File(s)
Summary
Add and test the solve counter crates/solidcore/src/kernel.rs
WorkCounters adds a public getter and an internal saturating increment for proto-vertex point solves. reset clears the count, and the counter test checks its initial, incremented, and reset values.
Skip solves for edged proto vertices crates/solidcore/src/topology/proto_topology/p1/proto.rs, crates/solidcore/src/topology/proto_topology/p1.rs
When the option is enabled, incident proto edges exist, and cancellation has not been detected, the method returns false before solving. It increments the counter before solves that run. The test covers edged and edge-less vertices and the already-cancelled case.
Check 3D perforated-plate solve counts crates/solidcore/tests/fork_edf7414_perforated_plate_work.rs
The test cuts a solid plate with two hole-pattern sizes in one boolean_lists call. It checks result topology, volume, and zero proto-vertex point solves.
Change: Refactor · Severity of issue fixed: Medium
Possibly related issues
Issue 3248 describes several quadratic boolean passes, including proto-vertex-to-proto-edge checks. This change skips point solves for vertices with incident proto edges, but does not change the other passes described in that issue.
Suggested reviewers:jyviko, bherbruck
Merge Risk: Minimal · up to 92df6
The change skips redundant point solves only when an opt-in option is enabled, and behavior with the option off is unchanged. No merge-blocking risk was identified. The author notes that cancellation error labels may differ and that the full workspace suite was not rerun on this head.
Security architecture risk: Minimal · up to 92df6
The change exposes a diagnostic count and avoids redundant calculations. The inspected paths preserve input validation, state ownership and cancellation propagation. No material security risk introduced or worsened by this change was identified.
Retained concerns
No architecture-level concerns identified.
Security review details
Security Blast Radius
inferred — The inspected public addition exposes a numeric diagnostic for work performed by the caller's Kernel. It does not introduce an execution capability, credential, remote entrypoint or cross-Kernel data access. The solve eligibility decision does not depend on the counter.
Trust Boundaries and Controls
observed — Cancellation errors from solving are not converted into ordinary misses by either error-handling helper. After intersection processing, the caller enters resolution, which polls cancellation before processing each proto vertex and proto edge, before the subsequent imprinting stage.
Resilience and Maintainability Implications
observed — The added cancellation check is not side-effect-free for counting tokens: each poll advances their count and can trigger cancellation. Skipped solves also omit cache work, so cancellation checkpoints or diagnostics can differ. The existing cancellation contract already permits partially modified mutable inputs on error; this PR does not add transactional rollback.
Pre-merge checks: 4
Check name
Status
Explanation
Title check
The title describes the main change: proto vertices that have proto edges are no longer point-solved against those proto edges, under the proto_vertex_miss_is_false setting. The raw summary and object…
Linked Issues check
[#3260] requires that, with proto_vertex_miss_is_false on, a proto vertex that has proto edges gets no point solve, with the same body. The reviewed head summary of proto.rs states that remove_coincid…
Out of Scope Changes check
The changed areas are proto.rs (the OP_REMOVE_COINCIDENT label, the early return with the flag on, and the solve counter), kernel.rs (WorkCounters::proto_vertex_point_solves with getter, increment, an…
Description Check
Check skipped - CodeRabbit’s high-level summary is enabled.
Full details: Title check
Explanation
The title describes the main change: proto vertices that have proto edges are no longer point-solved against those proto edges, under the proto_vertex_miss_is_false setting. The raw summary and objectives both confirm this is the core change, with supporting counter, tests, and documentation edits. The title uses the perf(boolean) prefix and references issue #3260, which matches the linked issue. It is concise and specific, so it passes.
Full details: Linked Issues check
Explanation
[#3260] requires that, with proto_vertex_miss_is_false on, a proto vertex that has proto edges gets no point solve, with the same body. The reviewed head summary of proto.rs states that remove_coincident_no_edge_proto_vertex returns false before the solve when the flag is on, the vertex has incident proto edges, and cancellation has not been detected. The solve counter is incremented immediately before the point solve. The same summary states that the solve still runs under an already-fired cancel token. The test plate_cut_solves_no_proto_vertex_that_has_proto_edges is reported to fail at (2450, 39800) at the test commit and to pass with (0, 0) at head, which matches the issue's target of 0 instead of 4N² − 2N. The test a_proto_vertex_with_proto_edges_is_not_solved covers edge-bearing vertices, an edge-less vertex, and the already-cancelled case. The PR description states that cancellation labels can change, as the issue asks. Body equality rests on the author's reported comparison (384 booleans, 492 bodies) and not on a re-run here. Limits: the whole-PR text diff could not be read (changed blobs were unavailable), and the proto.rs file read was truncated before the function. The early return therefore rests on the file summary and the PR description, not on a direct read of the function. The changes since the previous review could not be checked from a diff.
Full details: Out of Scope Changes check
Explanation
The changed areas are proto.rs (the OP_REMOVE_COINCIDENT label, the early return with the flag on, and the solve counter), kernel.rs (WorkCounters::proto_vertex_point_solves with getter, increment, and reset), p1.rs (the unit test for flag-on vertex cases and the cancelled-token case), and the 3D solid-plate count test in fork_edf7414_perforated_plate_work.rs. Each supports the objective of [#3260], either by implementing the skipped solve or by counting and verifying it. The summary shows no change to the flag-off path, the vertex-edge pair loop, or proto-edge pair intersection, which the issue excludes. The file list comes from the summary because the whole-PR diff could not be read, so this verdict has that limit.
Finishing Touches
Generate unit tests (beta)
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Reviewing files that changed from the base of the PR and between 39a5ef9 and 92df673.
Files selected for processing: 1
crates/solidcore/src/kernel.rs
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.
…ls: 2450, 39800)
After its face pairs, a boolean tests every proto vertex against every proto
edge (`RemoveCoincidentNoEdgeProtoVertex`) and removes a vertex that lies on an
edge only when the vertex has no proto edges of its own. A plate minus N
cylinders in one `boolean_lists` call has 2N proto vertices, each the end of
its own closed proto edge, and 2N proto edges: nothing can be removed, and
4N^2 - 2N point solves run all the same.
`WorkCounters::proto_vertex_point_solves` counts each solve where it runs. The
new test asks for 0 at 25 and at 100 holes, with the cut's faces, edges and
exact volume. It fails with 2450 and 39800.
Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
…the proto edges
With proto_vertex_miss_is_false on, remove_coincident_no_edge_proto_vertex
answers false for a miss and for a hit on a proto vertex that has proto edges
of its own. No outcome of the point solve gives another answer for such a
vertex, so the function returns false before the solve.
A plate minus N cylinders cut in one boolean_lists call ran 4N^2 - 2N of
these solves (2450 at N = 25, 39800 at N = 100); it now runs 0. The body is
the same: N + 6 faces, 3N + 12 edges, the exact volume.
Under an already-fired cancel token the solve runs as it did before, so from
the same caches the function returns what it returned. A skipped solve leaves
its curve's cache cold, so a kernel cancelled later can report its `Cancelled`
error from another check (another operation label); the boolean asks the token
again after this step, and no boolean was found to change between success and
error. With the flag off nothing changes.
Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #3260
With
proto_vertex_miss_is_falseon, a boolean no longer solves a proto vertex against a proto edge when the vertex has proto edges of its own: 0 point solves in place of 4N² − 2N for a plate minus N cylinders cut in one call. Every boolean that returned a body returns the same body. What a kernel cancelled mid-operation reports can differ (below).Related: #3248, pass B (proto vertices against proto edges). That issue proposes a box index so fewer pairs are visited; this PR is the other half and does not overlap with it: for the pairs that are visited, the solve is not run when the proto vertex has proto edges. Not measured on #3248's case (whether its proto vertices have proto edges of their own decides how much of its pass B this removes).
The problem
After its face pairs, a boolean tests every proto vertex against every proto edge (
RemoveCoincidentNoEdgeProtoVertex). A vertex found on an edge is removed only when it has no proto edges of its own. The point solve ran first all the same, for every pair.Cutting N cylinders out of a plate in one
boolean_listscall makes 2N section circles: 2N proto edges, each closed on its own proto vertex. Every vertex has an edge, so nothing can be removed, and 4N² − 2N solves ran for nothing.The change
topology/proto_topology/p1/proto.rs,remove_coincident_no_edge_proto_vertex: withproto_vertex_miss_is_false(WR-73, #848) on, a miss answersfalse, and a hit on a vertex that has proto edges answersfalsetoo. No outcome of the solve gives another answer for such a vertex, so the function returnsfalsebefore the solve. The checks before it (a deleted object, a missing curve) stay, in the same order.true, which ends the caller's loop, so the solve does decide something. That path goes when the flag is retired.WorkCounters::proto_vertex_point_solvescounts each solve, for the test.No new flag, no cache, no second attempt.
Counts
Plate
20 n x 20 n x 12minus N = n² cylinders r 4 in oneboolean_listscall, SolidCraft's fixes:The result has N + 6 faces, 3N + 12 edges and its exact volume, before and after.
What changes in the result
Cancellederror can differ. Skipping a solve also skips warming its curve's cache. A kernel cancelled later can therefore notice the cancel at a different check:CurveCache::tessellatewhere it wasgeom::global_solver::solve_branch.Ok(false)with the old solve's cache warming andCancelledwithout it. The manager's state is unchanged either way.resolve_intersections).Unportedraised only inside a solve that is no longer run, and whose answer was not read, would no longer end the boolean there. No boolean that reaches one is known: the site once suspected is not reachable, because the curve cache is built from a copy detached from its owner.If a changed
Cancelledlabel counts as a changed result here, say so and I will withdraw this: it cannot be kept identical without running the solve.Tests
plate_cut_solves_no_proto_vertex_that_has_proto_edges(committed first, failing with(2450, 39800)against(0, 0)).a_proto_vertex_with_proto_edges_is_not_solved: 0 solves for a vertex with edges on the curve or off it, 1 for an edge-less one, and under an already-fired cancel token the solve run for every vertex (3 solves, the old answers).Verification
On
92df6735a, rebased on maindc5cfd7e7, Linux (WSL Ubuntu 22.04):cargo fmt --all --check,cargo clippy --workspace --all-targets -- -D warnings,check_panic_sites.py,check_record_ids.py,check-frozen-archives.sh,cargo check -p solidcore --target wasm32-unknown-unknown: all exit 0.--all --goldens:goldens: 283 unit(s), 282 passed, 1 failed. The one iscurve.coniccasegen2235(values.r[4]: 3.776586443654626e-14 -> 3.552713678800501e-14), which differs the same way on unchanged main on that machine (checked onba55b2f71and on7b29ffdb5); this change does not touch it.(2450, 39800); at the head it passes with(0, 0).boolean_api,boolean_no_dangling_vertex_refs,issue_1120_boolean_slitspass. The focused lib tests (topology::proto_topology,boolean::topology_intersector,boolean::merge,kernel::tests) pass: 134.ba55b2f71, together with four other changes on one branch: the whole workspace suite (7181 passed, 113 skipped) and the kernel rig as CI runs it (1081 cases, 803 pass, 0 fail, 15 known-failing, none moved). Not re-run on this head: CI is the gate.Not attached
Not in this PR
🤖 Generated with Claude Code
Summary by CodeRabbit