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(topology): delete_face's shell walk ends when its question is answered (#3259) - #3262
delete_face marks one face for each face it deletes in a batch cut, where it marked the whole shell each time: 2N faces in place of (7/2)N² + (23/2)N for a plate minus N cylinders. No result changes.
The problem
After it unlinks a face, delete_face has to learn whether the face's vertices are still joined to the shell (if not, the shell has come apart and is split). It asked by walking the shell from the first of those vertices (shell_traversal_with_marks, upstream's ShellTraversal), marking everything the shell joins to it, and then read the mark of each of the face's vertices.
The walk did not stop when those vertices were marked. Cutting N cylinders out of a plate in one boolean_lists call deletes 2N discs. Each has one vertex, marked on the walk's first step, and each deletion went on to mark every face of the plate. The same walk is mechanism C of #3249.
The change
topology/topology_traverser.rs.shell_traversal_to_vertices is the shell walk for a caller that asks only which of a set of vertices the shell still joins to the start. It advances the mark once and marks in the same order as the whole walk, and ends as it marks the last vertex asked about. Each one is taken out of the set as it is marked, so those left were not reached, and the walk then covered everything joined to the start. shell_traversal and shell_traversal_with_marks are unchanged: their callers use the lists the whole walk collects.
topology/brep/p3.rs, delete_face_nesting. The first walk asks about the deleted face's vertices that are not shell vertices already. The walks that follow when the shell has come apart are the whole ones, as before.
Which walk, decided by where the marks live. In a table of marks (delete_face) the marks are the deletion's working state, read for the face's vertices and nothing else: the walk ends at the last of them. In the caller's notification records (delete_face_with_body_identity) every mark stays after the call, as upstream leaves it in the objects, so that walk stays the whole one (see "Not in this PR").
WorkCounters::shell_traversal_faces. Counts each face a shell traversal marks, for the test.
There is no flag, no cache and no second attempt: the walk is one loop with one end condition, and an empty set asked about is the whole walk.
Counts
Plate 20 n x 20 n x 12 minus N = n² cylinders r 4 in one boolean_lists call, SolidCraft's fixes. Faces marked by shell traversals:
N
Before
After
25
2 475
50
100
36 150
200
Law
(7/2)N² + (23/2)N
2N
The law before was fitted on N = 25 to 400 with a probe; 25 and 100 are the committed test's own output before and after the fix. The result has N + 6 faces, 3N + 12 edges and the same volume to the last digit printed, before and after. For scale only: at N = 200 these walks were about 90 ms of a 950 ms boolean.
Why no result changes
Bodies and ids. The walk's answer is read for the vertices asked about, and each of them is marked by the short walk exactly when the whole walk marks it. Nothing is created or deleted in a different order.
The sequence of marks. The walk advances the mark once, as before, so every later claim gets the same values.
Two things do differ, and no operation reads either:
Marks left in a table. After delete_face its table holds the marks the question needed, not a marking of the shell. Every caller in the tree hands it a table nothing else reads (a fresh Marks, or FaceBuildEnv::marks, which only delete_face writes).
Errors on a broken B-rep. A walk that used to fail on a missing link in a part of the shell it no longer reaches now succeeds. On a B-rep whose links are whole the walk cannot fail.
Two independent reviews were told to break exactness and accepted the change. One correction came out of them: the description of what the marks table holds afterwards, as written above.
Tests
plate_cut_marks_one_face_for_each_face_it_deletes (committed first, failing with (2475, 36150) against (50, 200)): the cut at 25 and 100 holes, its faces, edges and exact volume, and one face marked for each face deleted.
a_shell_walk_asked_about_vertices_ends_at_the_last_of_them: on a box, nothing asked marks 6 faces and 8 vertices; the start alone marks 1 face and 1 vertex; every vertex asked leaves none; a vertex the shell does not join is left after the whole walk.
A test of delete_face_with_body_identity under an outer lock: after the deletion every face left in the shell is still marked in the caller's records.
work_counters_count_until_reset: the new counter.
Verification
On b8ac8df8d, 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 (2475, 36150); at the head it passes with (50, 200). boolean_api, boolean_no_dangling_vertex_refs, issue_1120_boolean_slits pass. The focused lib tests (topology::brep::p3, topology::topology_traverser, kernel::tests) pass: 46.
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 an order of visits (one disc with its single vertex, the first vertex use the walk reaches, and the rest of the shell it went on to mark), which a rendering of the body does not show, and I have no VectorCraft figure of it. Say if you want one before merging.
Not in this PR
delete_face_with_body_identity (the offset executive's step gaps and crossing loops, the fillet's insertion) keeps the whole walk, because its marks stay in the caller's records and the fillet executive's goldens read raw mark values from those records. Whether the marks a finished deletion leaves there are part of its result is yours to say; with that walk cut short the test added here fails, as it should.
Face deletion now stops targeted shell traversal once the relevant vertices are reached. Operations that retain deletion records continue traversing the full shell.
Tests
Added coverage for Boolean cuts in solid plates with multiple holes, checking the resulting geometry and shell traversal counts across different hole counts.
No actionable comments were generated in the recent review.
Walkthrough
Shell traversal now stops after it marks all requested vertices and counts newly marked faces. Face deletion uses early stopping for indexed marks and full-shell traversal for retained records. Tests cover traversal requests, retained marks, and perforated-plate cuts.
The shell walk tracks requested vertices and stops when all are marked. WorkCounters records newly marked faces, and tests cover empty, reached, and unreachable requests.
Face deletion mark modes and regression coverage crates/solidcore/src/topology/brep/p3.rs, crates/solidcore/tests/fork_edf7414_perforated_plate_work.rs
Indexed deletion marks use requested-vertex stopping; retained records continue across the shell. Tests check retained marks and verify two marked faces per hole in 5×5 and 10×10 plate cuts.
This change reduces redundant shell-walk work when deleting faces, and the supplied evidence shows no remaining merge-blocking risk. The author reports unchanged geometry results and passing focused tests.
Security architecture risk: Low · up to b8ac8
This is a bounded library change, with no demonstrated expansion of access or privileges. Callers that retain complete traversal records keep their existing behavior. Compatibility remains uncertain for callers that rely on complete working marks or incidental detection of malformed geometry; no exploitable security regression was established.
Retained concerns
No architecture-level concerns identified.
Security review details
Security Blast Radius
observed — The demonstrated affected state is the caller's in-process B-rep, traversal marks, notification records, and kernel counter. The inspected Boolean deletion adapter checks that the face belongs to the selected B-rep and allocates a local Marks table that is discarded after deletion.
Trust Boundaries and Controls
observed — The existing public deletion paths retain their caller-identity wrapper. Its preflight enforces notification-key ownership, including reserved-key restrictions, while deletion separately requires editing to be enabled and the face to belong to the B-rep. These controls are distinct from structural validation of every shell link and are not removed by early termination.
Pre-merge checks: 4
Check name
Status
Explanation
Title check
The title names the main change: delete_face's shell walk now stops once the queried vertices are all marked, which matches the raw summary's topology/traverser and p3.rs changes and the linked issue'…
Linked Issues check
[#3259] Objectives: (1) in delete_face, the shell walk must end once the asked vertices are marked; (2) an unreached vertex must leave the walk covering its whole component; (3) caller-owned notificat…
Out of Scope Changes check
All changed areas connect to #3259. (a) topology/topology_traverser.rs: shell_traversal_to_vertices, which implements the early-stop walk (body not read directly, see the linked-issue check). (b) topo…
Description Check
Check skipped - CodeRabbit’s high-level summary is enabled.
Full details: Title check
Explanation
The title names the main change: delete_face's shell walk now stops once the queried vertices are all marked, which matches the raw summary's topology/traverser and p3.rs changes and the linked issue's objective. It uses the conventional perf(topology) prefix. The number in the title, #3259, differs from the PR number #3262 in the objectives, but this does not affect whether the title describes the change. The title is specific and accurate.
Full details: Linked Issues check
Explanation
[#3259] Objectives: (1) in delete_face, the shell walk must end once the asked vertices are marked; (2) an unreached vertex must leave the walk covering its whole component; (3) caller-owned notification records must keep the whole walk. Evidence at head b8ac8df: In topology/brep/p3.rs, delete_face_nesting builds the asked set for DeletionMarks::Indexed from the deleted face's vertices that are not shell vertices, and passes it to tt::shell_traversal_to_vertices. For DeletionMarks::Retained the set is empty, so tt::shell_traversal_to_vertices runs the whole walk. The call-site diff matches the PR description. The loop body of shell_traversal_to_vertices in topology/topology_traverser.rs could not be read: the diff returned 'changed blobs are missing from the existing checkout'. Its behavior (removing each asked vertex when marked, ending when the set is empty, whole walk for an empty set) rests on the change summary and PR description. The reported tests (plate_cut_marks_one_face_for_each_face_it_deletes with (50, 200) at N=25/100 against (2475, 36150) before; a_shell_walk_asked_about_vertices_ends_at_the_last_of_them; the retained-records assertion added in p3.rs tests) support the claim but were not re-run here. The previous PASS is preserved on this basis; the batch-cut and unreached-vertex objectives are met as described. No non-coding requirement is involved.
Full details: Out of Scope Changes check
Explanation
All changed areas connect to #3259. (a) topology/topology_traverser.rs: shell_traversal_to_vertices, which implements the early-stop walk (body not read directly, see the linked-issue check). (b) topology/brep/p3.rs: the call-site change in delete_face_nesting; DeletionMarks variant documentation; the delete_face and delete_face_with_body_identity doc comments that state which walk each uses; and a test assertion that retained records mark every face left in the shell. The retained-records assertion checks the behavior that the issue requires to be preserved. (c) crates/solidcore/src/kernel.rs: WorkCounters::shell_traversal_faces, read by the count test. (d) crates/solidcore/tests/fork_edf7414_perforated_plate_work.rs: the plate-cut test that measures the batch-cut case from the issue. Each change supports the required behavior or its verification. No unrelated behavior change appears in the diff or summary.
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 bb4e890 and b8ac8df.
Files selected for processing: 2
crates/solidcore/src/kernel.rs
crates/solidcore/src/topology/brep/p3.rs
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.
…5, 36150)
`delete_face` asks whether the vertices of the face it deleted are still joined
to the shell, and answers by walking the shell from the first of them
(`shell_traversal_with_marks`) and marking everything it reaches. Cutting N
cylinders out of a plate in one `boolean_lists` call deletes 2N discs, each
with one vertex, and each deletion walks the whole shell although the first
vertex use it visits settles the question.
`WorkCounters::shell_traversal_faces` counts each face a shell traversal marks,
where it is marked. The new test cuts 25 and 100 holes with SolidCraft's fixes,
checks the result (N + 6 faces, 3N + 12 edges, the exact volume within 1e-6)
and asks for one face marked for each face deleted: 50 and 200.
Fails until the walk ends when its question is answered:
left: (2475, 36150) (7/2) N^2 + (23/2) N
right: (50, 200) 2 N
Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
…swered
After it unlinks a face, `delete_face` has to learn whether the face's vertices
are still joined to the shell: if not, the shell has come apart and is split. It
asked by walking the shell from the first of those vertices, marking everything
the shell joins to it, and then read the marks of the face's vertices. The walk
went on over the whole shell after the last of them was marked, so deleting N
faces of one body marked on the order of N^2 faces.
The walk now takes the vertices it is asked about and ends as it marks the last
of them (`shell_traversal_to_vertices`). It advances the mark once and marks in
the same order as before, as far as it goes; when a vertex is not reached it
covers everything joined to the start, as before, and the split that follows is
unchanged. `shell_traversal` and `shell_traversal_with_marks` are the whole
walk still: their callers use the lists it collects.
Which walk `delete_face` runs is decided by where its marks live:
- In a table of marks (`delete_face`: the boolean's merge step, the stitch, the
primitives, the planar faces, the cutter, fillet removal) the marks are the
deletion's working state, read for the face's vertices and nothing else. The
walk ends at the last of them.
- In the caller's notification records (`delete_face_with_body_identity`: the
offset executive's step gaps and crossing loops, the fillet's insertion) every
mark stays after the call, as upstream leaves it in the objects, and when the
shell holds together nothing advances the mark before the lock is released.
Cutting that walk short would leave fewer records marked, so it stays
upstream's whole walk. A test now pins that: after such a deletion every face
left in the shell is marked.
Results are unchanged: the same bodies and entity ids, and the same sequence of
marks claimed from the context. What differs is the content of a marks table
after `delete_face`, which no caller reads, and on a B-rep with a broken link
in a part of the shell the walk no longer reaches, the error the whole walk met
there.
Counts (plate minus N cylinders in one `boolean_lists` call, SolidCraft's
fixes), faces marked by shell traversals (`WorkCounters::shell_traversal_faces`):
N = 25: 2475 -> 50
N = 100: 36150 -> 200
law: (7/2) N^2 + (23/2) N -> 2 N, one for each face deleted
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 #3259
delete_facemarks one face for each face it deletes in a batch cut, where it marked the whole shell each time: 2N faces in place of (7/2)N² + (23/2)N for a plate minus N cylinders. No result changes.The problem
After it unlinks a face,
delete_facehas to learn whether the face's vertices are still joined to the shell (if not, the shell has come apart and is split). It asked by walking the shell from the first of those vertices (shell_traversal_with_marks, upstream'sShellTraversal), marking everything the shell joins to it, and then read the mark of each of the face's vertices.The walk did not stop when those vertices were marked. Cutting N cylinders out of a plate in one
boolean_listscall deletes 2N discs. Each has one vertex, marked on the walk's first step, and each deletion went on to mark every face of the plate. The same walk is mechanism C of #3249.The change
topology/topology_traverser.rs.shell_traversal_to_verticesis the shell walk for a caller that asks only which of a set of vertices the shell still joins to the start. It advances the mark once and marks in the same order as the whole walk, and ends as it marks the last vertex asked about. Each one is taken out of the set as it is marked, so those left were not reached, and the walk then covered everything joined to the start.shell_traversalandshell_traversal_with_marksare unchanged: their callers use the lists the whole walk collects.topology/brep/p3.rs,delete_face_nesting. The first walk asks about the deleted face's vertices that are not shell vertices already. The walks that follow when the shell has come apart are the whole ones, as before.delete_face) the marks are the deletion's working state, read for the face's vertices and nothing else: the walk ends at the last of them. In the caller's notification records (delete_face_with_body_identity) every mark stays after the call, as upstream leaves it in the objects, so that walk stays the whole one (see "Not in this PR").WorkCounters::shell_traversal_faces. Counts each face a shell traversal marks, for the test.There is no flag, no cache and no second attempt: the walk is one loop with one end condition, and an empty set asked about is the whole walk.
Counts
Plate
20 n x 20 n x 12minus N = n² cylinders r 4 in oneboolean_listscall, SolidCraft's fixes. Faces marked by shell traversals:The law before was fitted on N = 25 to 400 with a probe; 25 and 100 are the committed test's own output before and after the fix. The result has N + 6 faces, 3N + 12 edges and the same volume to the last digit printed, before and after. For scale only: at N = 200 these walks were about 90 ms of a 950 ms boolean.
Why no result changes
Two things do differ, and no operation reads either:
delete_faceits table holds the marks the question needed, not a marking of the shell. Every caller in the tree hands it a table nothing else reads (a freshMarks, orFaceBuildEnv::marks, which onlydelete_facewrites).Two independent reviews were told to break exactness and accepted the change. One correction came out of them: the description of what the marks table holds afterwards, as written above.
Tests
plate_cut_marks_one_face_for_each_face_it_deletes(committed first, failing with(2475, 36150)against(50, 200)): the cut at 25 and 100 holes, its faces, edges and exact volume, and one face marked for each face deleted.a_shell_walk_asked_about_vertices_ends_at_the_last_of_them: on a box, nothing asked marks 6 faces and 8 vertices; the start alone marks 1 face and 1 vertex; every vertex asked leaves none; a vertex the shell does not join is left after the whole walk.delete_face_with_body_identityunder an outer lock: after the deletion every face left in the shell is still marked in the caller's records.work_counters_count_until_reset: the new counter.Verification
On
b8ac8df8d, 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.(2475, 36150); at the head it passes with(50, 200).boolean_api,boolean_no_dangling_vertex_refs,issue_1120_boolean_slitspass. The focused lib tests (topology::brep::p3,topology::topology_traverser,kernel::tests) pass: 46.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
delete_face_with_body_identity(the offset executive's step gaps and crossing loops, the fillet's insertion) keeps the whole walk, because its marks stay in the caller's records and the fillet executive's goldens read raw mark values from those records. Whether the marks a finished deletion leaves there are part of its result is yours to say; with that walk cut short the test added here fails, as it should.delete_facewalks the whole shell to answer a question its first step settles: N² faces marked in a batch cut #3259) would bound it by the smaller piece, but visits in another order.delete_edge's wire branch (wire_split_shell) asks the same kind of question with the same walk, but uses the lists the walk collects. Not measured.🤖 Generated with Claude Code
Summary by CodeRabbit