Use involved_face_index_diff for surgical index updates in YgorMeshesHoles.cc by Copilot · Pull Request #50 · hdclark/Ygor

Copilot · 2026-03-31T05:39:30Z

fv_surface_mesh operations in YgorMeshesHoles.cc and YgorMeshesRefinement.cc were reviewed for post-topology involved_faces maintenance. FillBoundaryChainsByZippering now uses apply_involved_face_index_diff() when safe, while loop_subdivide uses full recreate_involved_face_index() rebuilds because each iteration replaces the full face topology.

Changes

FillBoundaryChainsByZippering
- Uses a stronger validity gate before taking the diff path: verifies involved_faces is both correctly sized and consistent with current mesh connectivity.
- Uses size_t for face-index iteration and casts to I only when storing diff entries.
- Applies a surgical diff only for newly appended faces when the index is confirmed current; otherwise falls back to full rebuild.
loop_subdivide
- Reverted to full recreate_involved_face_index() after replacing faces each iteration, since the topology change is global and a full rebuild is the practical/efficient choice.
Tests (tests2/YgorMeshesHoles.cc, tests2/YgorMeshesRefinement.cc)
- Kept/updated regression subcases comparing involved_faces against a fresh recreate_involved_face_index() result.
- Refactored duplicated comparison logic into local helper lambdas for consistency.
YgorMeshesRemeshing.cc
- No changes; existing diff usage remains as-is.

Tests

Full suite passes: 290 test cases, 99554 assertions.

…es.cc and YgorMeshesRefinement.cc Convert FillBoundaryChainsByZippering() to apply a surgical diff for newly added faces when the index is already valid, falling back to a full rebuild when the index is absent. Convert loop_subdivide() to compute and apply a diff at the end of each iteration instead of clearing the index and rebuilding from scratch. The initial rebuild is still performed on the first iteration to handle any prior modifications to the mesh. Add unit tests that verify the diff-based index matches a full rebuild for both hole filling and subdivision operations. Agent-Logs-Url: https://github.com/hdclark/Ygor/sessions/12a742ef-e1eb-40e9-888a-4471bfddf506 Co-authored-by: hdclark <934858+hdclark@users.noreply.github.com>

Copilot

Pull request overview

This PR replaces full involved_faces index rebuilds with diff-based incremental updates after topology changes in hole filling and Loop subdivision, and adds regression tests to validate index correctness against recreate_involved_face_index().

Changes:

Update FillBoundaryChainsByZippering to apply a diff for newly appended faces when an index is present; otherwise rebuild.
Update loop_subdivide to rebuild only on the first iteration and then maintain involved_faces via diffs per iteration.
Add tests that compare diff-updated involved_faces against a full rebuild for hole filling and refinement.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

File	Description
src/YgorMeshesHoles.cc	Switches post-zippering index maintenance from full rebuild to diff-based additions when index is present.
src/YgorMeshesRefinement.cc	Replaces per-iteration/full-end rebuilds in `loop_subdivide` with diff-based remove/add updates.
tests2/YgorMeshesHoles.cc	Adds subcases to verify zippering diff/rebuild paths match `recreate_involved_face_index()`.
tests2/YgorMeshesRefinement.cc	Adds subcases to verify `loop_subdivide` diff-based index matches full rebuild after 1/2 iterations and on an icosahedron.

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

Copilot · 2026-03-31T06:10:39Z

+        if(fvsm.involved_faces.size() == fvsm.vertices.size()){
+            // Index is valid; apply surgical update for the newly added faces only.
+            involved_face_index_diff<I> diff;
+            for(auto f_idx = N_faces_before; f_idx < static_cast<I>(fvsm.faces.size()); ++f_idx){
+                for(const auto &v : fvsm.faces[f_idx]){
+                    diff.entries_to_add.emplace_back(v, f_idx);
+                }
+            }
+            fvsm.apply_involved_face_index_diff(diff);
+        }else{
+            fvsm.recreate_involved_face_index();
+        }


The condition involved_faces.size() == vertices.size() doesn’t actually prove the index is current—only that it’s sized consistently. If involved_faces can be stale-but-sized (e.g., prior face edits without clearing/updating), applying a diff here will preserve incorrect existing associations and produce a wrong index. Consider gating the diff path on a stronger “index is known-valid/up-to-date” signal (e.g., an explicit validity flag/generation counter), or conservatively falling back to recreate_involved_face_index() unless the caller/function can guarantee the index is current.

Suggested change

if(fvsm.involved_faces.size() == fvsm.vertices.size()){

// Index is valid; apply surgical update for the newly added faces only.

involved_face_index_diff<I> diff;

for(auto f_idx = N_faces_before; f_idx < static_cast<I>(fvsm.faces.size()); ++f_idx){

for(const auto &v : fvsm.faces[f_idx]){

diff.entries_to_add.emplace_back(v, f_idx);

}

}

fvsm.apply_involved_face_index_diff(diff);

}else{

fvsm.recreate_involved_face_index();

}

// Conservatively recreate the involved-face index to ensure it is up to date.

fvsm.recreate_involved_face_index();

gate the diff path on a stronger “index is known-valid/up-to-date” signal

Copilot · 2026-03-31T06:10:40Z

    if(holes.has_nonmanifold_edges) return false;

    const auto eps_sq = eps * eps;
+    const auto N_faces_before = static_cast<I>(fvsm.faces.size());


This mixes size_t container sizes with I via static_cast<I>(fvsm.faces.size()) and uses auto f_idx = N_faces_before (deducing I). That can truncate/wrap if sizes approach/exceed I’s range and also makes the loop bounds dependent on narrowing casts. Prefer iterating with size_t (for(size_t f_idx = N_faces_before; f_idx < fvsm.faces.size(); ++f_idx)) and only casting f_idx to I at the point of storing it in the diff entries.

implement this suggestion

Copilot · 2026-03-31T06:10:40Z

+            for(auto f_idx = N_faces_before; f_idx < static_cast<I>(fvsm.faces.size()); ++f_idx){
+                for(const auto &v : fvsm.faces[f_idx]){
+                    diff.entries_to_add.emplace_back(v, f_idx);
+                }
+            }


This mixes size_t container sizes with I via static_cast<I>(fvsm.faces.size()) and uses auto f_idx = N_faces_before (deducing I). That can truncate/wrap if sizes approach/exceed I’s range and also makes the loop bounds dependent on narrowing casts. Prefer iterating with size_t (for(size_t f_idx = N_faces_before; f_idx < fvsm.faces.size(); ++f_idx)) and only casting f_idx to I at the point of storing it in the diff entries.

implement this suggestion

Copilot · 2026-03-31T06:10:40Z

+        // Compute the diff before replacing faces so we can reference the old face data.
+        involved_face_index_diff<I> diff;
+        diff.new_vertex_count = static_cast<I>(fvsm.vertices.size()) - N_orig_verts;
+
+        // Remove associations for all old faces.
+        for(size_t f_idx = 0; f_idx < N_orig_faces; ++f_idx){
+            for(const auto &v : fvsm.faces[f_idx]){
+                diff.entries_to_remove.emplace_back(v, static_cast<I>(f_idx));
+            }
+        }
+
+        // Add associations for all new faces.
+        for(size_t f_idx = 0; f_idx < new_faces.size(); ++f_idx){
+            for(const auto &v : new_faces[f_idx]){
+                diff.entries_to_add.emplace_back(v, static_cast<I>(f_idx));
+            }
+        }


In loop_subdivide, every iteration “diffs” by removing all old face associations and adding all new ones, which is effectively a full rebuild expressed as many individual remove/add operations. If apply_involved_face_index_diff() performs per-entry search/erase in per-vertex vectors, this can be measurably slower than rebuilding from scratch, especially for high-valence vertices (many removals). Consider either (a) keeping recreate_involved_face_index() here since the topology replacement is total, or (b) adding/using a specialized fast-path in the diff application for “replace all faces” that clears/repacks adjacency in linear time.

implement this suggestion, using a full index rebuild when it would not be practical to use a surgical diff. Fully recreating the index from scratch is a good idea when the entire mesh is being altered (as is the case in Loops' subdivision).

Copilot · 2026-03-31T06:10:41Z

+        for(size_t v = 0; v < mesh.vertices.size(); ++v){
+            auto got = mesh.involved_faces[v];
+            auto expected = reference.involved_faces[v];
+            std::sort(got.begin(), got.end());
+            std::sort(expected.begin(), expected.end());
+            REQUIRE(got == expected);
+        }


The “sort and compare involved_faces vs recreate reference” loop is duplicated across multiple subcases (and similarly in tests2/YgorMeshesHoles.cc). To reduce repetition and keep future updates consistent, consider extracting this into a small helper (e.g., a local lambda require_involved_faces_matches_recreate(mesh)).

implement this suggestion

hdclark · 2026-03-31T14:36:40Z

@copilot apply changes based on the comments in this thread

hdclark · 2026-04-01T02:49:17Z

@copilot apply changes based on the comments in this thread

Agent-Logs-Url: https://github.com/hdclark/Ygor/sessions/a95af4b3-c53c-4551-aac3-03327e8c8520 Co-authored-by: hdclark <934858+hdclark@users.noreply.github.com>

Copilot · 2026-04-01T03:00:13Z

@copilot apply changes based on the comments in this thread

Implemented the requested changes from that review thread and pushed them in commit 1c41ed2.

Specifically: stronger validity gating + size_t-safe face iteration in zippering, full involved_faces rebuild in loop_subdivide for full topology replacement, and helper-lambda deduplication of involved_faces/recreate comparisons in both test files.

Initial plan

f2ecabe

Copilot AI assigned Copilot and hdclark Mar 31, 2026

Copilot started work on behalf of hdclark March 31, 2026 05:39 View session

Copilot AI changed the title ~~[WIP] Update involved face index to use diff approach~~ Use involved_face_index_diff for surgical index updates in YgorMeshes*.cc Mar 31, 2026

Copilot finished work on behalf of hdclark March 31, 2026 05:57

Copilot AI requested a review from hdclark March 31, 2026 05:57

hdclark marked this pull request as ready for review March 31, 2026 05:59

Copilot AI review requested due to automatic review settings March 31, 2026 05:59

Copilot started reviewing on behalf of hdclark March 31, 2026 06:00 View session

Copilot AI reviewed Mar 31, 2026

View reviewed changes

Copilot started work on behalf of hdclark April 1, 2026 02:49 View session

Address PR review feedback on involved_faces updates

1c41ed2

Agent-Logs-Url: https://github.com/hdclark/Ygor/sessions/a95af4b3-c53c-4551-aac3-03327e8c8520 Co-authored-by: hdclark <934858+hdclark@users.noreply.github.com>

Copilot AI changed the title ~~Use involved_face_index_diff for surgical index updates in YgorMeshes*.cc~~ Use involved_face_index_diff for surgical index updates in YgorMeshesHoles.cc Apr 1, 2026

Copilot finished work on behalf of hdclark April 1, 2026 03:01

Conversation

Copilot AI commented Mar 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Tests

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

hdclark Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

hdclark Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

hdclark Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

hdclark Mar 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

hdclark Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

hdclark commented Mar 31, 2026

Uh oh!

hdclark commented Apr 1, 2026

Uh oh!

Copilot AI commented Apr 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Copilot AI commented Mar 31, 2026 •

edited

Loading

hdclark Mar 31, 2026 •

edited

Loading