Implement mergeplanar: merge coplanar triangles into planar faces - #15
Conversation
The flag was parsed but never used (and printed a wrong message while consuming the next argument). It now merges edge-connected coplanar triangles into single planar faces: - Union-find over edge-adjacent triangles with parallel normals; requiring adjacency keeps disjoint coplanar patches as separate faces (global plane binning is what broke the earlier merge_planes attempt) - Directed boundary edges chained into closed loops, preserving STL winding so hole loops stay correctly oriented within one face - Vertices and boundary edge curves shared globally so neighboring faces reference identical topology; open chains are dropped with a warning instead of emitting invalid loops Also fixes the arg parser consuming the argument following mergeplanar and documents the option in help/README. Tests: cube content check (12 tris -> exactly 6 faces/12 edges/8 verts, unmerged path unchanged) plus bucket and cat_dish smoke runs; bucket merges 11286 -> 6236 faces with no dropped chains. The *.cmake gitignore catch-all was removed so the test script can be tracked.
There was a problem hiding this comment.
Pull request overview
Adds a new mergeplanar CLI option to merge edge-connected coplanar STL triangles into single planar STEP faces, improving output topology for planar regions and fixing previously broken argument parsing for the flag.
Changes:
- Implement planar triangle merging in
StepKernelvia adjacency-based union-find and boundary loop extraction. - Wire up the
mergeplanarCLI flag, update usage/help text, and add regression/smoke tests. - Track the new CMake test script by removing the
*.cmakeignore rule.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/cube.stl | Adds a unit-cube STL fixture (12 triangles) for mergeplanar regression testing. |
| test/check_mergeplanar.cmake | Adds a CMake script to assert expected entity counts with/without mergeplanar. |
| StepKernel.h | Extends build_tri_body signature with merge_planar flag and declares merged implementation. |
| StepKernel.cpp | Implements build_tri_body_merged and routes build_tri_body based on merge_planar. |
| README.md | Documents the mergeplanar option in usage (needs alignment with other existing CLI options). |
| main.cpp | Fixes mergeplanar parsing (no longer consumes next arg) and passes flag to StepKernel. |
| CMakeLists.txt | Adds mergeplanar_* tests (cube regression + bucket/cat_dish smoke runs). |
| .gitignore | Removes *.cmake ignore so the new test script can be committed. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
@copilot resolve the merge conflicts in this pull request |
Resolved the merge conflicts. The three conflicting files were handled as follows:
All 11 tests pass after the merge. Commit: |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (2)
StepKernel.cpp:228
- The directed-edge map overwrites earlier owners when the same directed edge appears multiple times (non-manifold input). That can silently drop triangle adjacency and lead to incorrect grouping/boundary extraction; it’s better to detect duplicates and warn (or handle) instead of overwriting.
for (std::size_t t = 0; t < tri_cnt; t++)
for (int c = 0; c < 3; c++)
directed_edge_tri[make_edge_key(vert_of(tri_ids[t], c), vert_of(tri_ids[t], (c + 1) % 3))] = t;
StepKernel.cpp:367
- These newly added lines appear to include stray carriage-return characters (mixed line endings), which can cause noisy diffs and tooling/lint issues. Please normalize this block to use consistent LF line endings.
// Orthonormalize ref against the normal (matches the non-merged path's csys construction)
double dotnr = ref[0] * n[0] + ref[1] * n[1] + ref[2] * n[2];
ref[0] -= dotnr * n[0];
ref[1] -= dotnr * n[1];
ref[2] -= dotnr * n[2];
The flag was parsed but never used (and printed a wrong message while consuming the next argument). It now merges edge-connected coplanar triangles into single planar faces:
Also fixes the arg parser consuming the argument following mergeplanar and documents the option in help/README.
Tests: cube content check (12 tris -> exactly 6 faces/12 edges/8 verts, unmerged path unchanged) plus bucket and cat_dish smoke runs; bucket merges 11286 -> 6236 faces with no dropped chains. The *.cmake gitignore catch-all was removed so the test script can be tracked.