Add Triangulate_Planar_Contour_Connectivity for bridging offset planar contours#9
Add Triangulate_Planar_Contour_Connectivity for bridging offset planar contours#9
Conversation
…g gap between planar contours Co-authored-by: hdclark <934858+hdclark@users.noreply.github.com>
…safety check Co-authored-by: hdclark <934858+hdclark@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Implements a new triangulation utility to build an fv_surface_mesh that bridges two planar contour_collections on parallel, offset planes, plus unit tests and small test runner ignore updates.
Changes:
- Added
Triangulate_Planar_Contour_Connectivitytemplate declaration inYgorMathContourConnectivity.h, documenting the new API and its expected behavior. - Implemented a projected-plane, greedy nearest-neighbor triangulation in
YgorMathContourConnectivity.cc, including explicitfloat/doubleinstantiations. - Added doctest-based unit tests in
tests2/YgorMath/contour_connectivity.ccto validate basic shapes, multiple contours (including a hole pattern), oblique planes, float instantiation, and mesh integrity, and updated.gitignoreto ignore local test runners.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| src/YgorMathContourConnectivity.h | Declares the Triangulate_Planar_Contour_Connectivity API and documents inputs, outputs, and exceptions (note: documentation for contour orientation handling and distance_eps semantics should be aligned with the actual implementation). |
| src/YgorMathContourConnectivity.cc | Implements the triangulation algorithm: computes an approximate separation direction, projects vertices to 2D, connects top/bottom contour edges via nearest neighbors, filters degenerate triangles, and populates an fv_surface_mesh (contains some dead code and duplicated Average_Point logic that can be simplified). |
| tests2/YgorMath/contour_connectivity.cc | Adds tests covering simple and multiple contours, a hole configuration, oblique planes, float support, and mesh index/area validity (additional coverage is suggested for the "no vertices in collections" invalid-argument branch). |
| .gitignore | Ignores run_tests and run_connectivity_tests binaries/scripts to keep the repo clean from local test artifacts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/YgorMathContourConnectivity.cc
Outdated
| // Helper to check if triangle is valid (proper orientation and no edge crossings) | ||
| // Use 3D area calculation since the triangles span between the two planes | ||
| auto is_valid_triangle = [&](size_t v0, size_t v1, size_t v2) -> bool { | ||
| const auto &p0_3d = projected_verts[v0].pos_3d; | ||
| const auto &p1_3d = projected_verts[v1].pos_3d; | ||
| const auto &p2_3d = projected_verts[v2].pos_3d; | ||
|
|
||
| // Calculate 3D triangle area using cross product | ||
| const vec3<T> edge1 = p1_3d - p0_3d; | ||
| const vec3<T> edge2 = p2_3d - p0_3d; | ||
| const vec3<T> cross = edge1.Cross(edge2); | ||
| const T area = cross.length() / static_cast<T>(2); | ||
|
|
||
| if (area < distance_eps * distance_eps) { | ||
| return false; | ||
| } | ||
|
|
||
| return true; | ||
| }; |
There was a problem hiding this comment.
The comment on is_valid_triangle says it checks "proper orientation and no edge crossings", but the implementation only computes a 3D area and rejects nearly-degenerate triangles. To avoid misleading readers, either update the comment/name to reflect that it is purely an area-based degeneracy check, or extend the implementation to actually enforce orientation and perform edge-intersection checks against existing faces.
There was a problem hiding this comment.
Extend the implementation to actually enforce orientation and perform edge-intersection checks against existing faces.
There was a problem hiding this comment.
Extended is_valid_triangle to:
- Check for degenerate triangles (non-zero 3D area)
- Perform edge-intersection checks against all existing face edges in 2D projection
- Use O(1) map lookups for vertex_index to pos_2d mapping
The 2D segment intersection test uses cross-product orientation to detect edge crossings. Commit f143399.
| SUBCASE("empty top collection throws"){ | ||
| contour_collection<double> top_cc; | ||
|
|
||
| contour_collection<double> bottom_cc; | ||
| bottom_cc.contours.emplace_back(); | ||
| bottom_cc.contours.back().closed = true; | ||
| bottom_cc.contours.back().points.emplace_back(vec3<double>(0.0, 0.0, 0.0)); | ||
|
|
||
| REQUIRE_THROWS_AS( | ||
| Triangulate_Planar_Contour_Connectivity(top_cc, bottom_cc), | ||
| std::invalid_argument | ||
| ); | ||
| } | ||
|
|
||
| SUBCASE("empty bottom collection throws"){ | ||
| contour_collection<double> top_cc; | ||
| top_cc.contours.emplace_back(); | ||
| top_cc.contours.back().closed = true; | ||
| top_cc.contours.back().points.emplace_back(vec3<double>(0.0, 0.0, 1.0)); | ||
|
|
||
| contour_collection<double> bottom_cc; | ||
|
|
||
| REQUIRE_THROWS_AS( | ||
| Triangulate_Planar_Contour_Connectivity(top_cc, bottom_cc), | ||
| std::invalid_argument | ||
| ); | ||
| } |
There was a problem hiding this comment.
The tests cover empty contour collections via top_cc.contours.empty() / bottom_cc.contours.empty(), but they do not exercise the case where the collections are non-empty yet contain no points, which triggers the separate "Contour collections contain no vertices." invalid_argument path in the implementation. To keep the new API well-tested, consider adding a small test that constructs contour_collections with one or more empty contour_of_points entries and verifies that Triangulate_Planar_Contour_Connectivity throws std::invalid_argument in that scenario.
There was a problem hiding this comment.
Add a small test that constructs contour_collections with one or more empty contour_of_points entries and verifies that Triangulate_Planar_Contour_Connectivity throws std::invalid_argument in that scenario.
There was a problem hiding this comment.
Added two test cases:
"collections with empty contour_of_points entries throw"- top collection has empty contour, bottom has points"multiple contours with some empty entries throw"- top collection has multiple empty contours
Both verify std::invalid_argument is thrown. Commit f143399.
src/YgorMathContourConnectivity.cc
Outdated
| vec3<T> avg_top_point(static_cast<T>(0), static_cast<T>(0), static_cast<T>(0)); | ||
| vec3<T> avg_bottom_point(static_cast<T>(0), static_cast<T>(0), static_cast<T>(0)); | ||
| size_t top_count = 0; | ||
| size_t bottom_count = 0; | ||
|
|
||
| for (const auto &contour : top_cc.contours) { | ||
| for (const auto &pt : contour.points) { | ||
| avg_top_point += pt; | ||
| ++top_count; | ||
| } | ||
| } | ||
| for (const auto &contour : bottom_cc.contours) { | ||
| for (const auto &pt : contour.points) { | ||
| avg_bottom_point += pt; | ||
| ++bottom_count; | ||
| } | ||
| } |
There was a problem hiding this comment.
The computation of avg_top_point and avg_bottom_point manually iterates all contours and points, duplicating the contour_collection<T>::Average_Point() logic that already exists in YgorMath (see src/YgorMath.cc:5095). For consistency with the rest of the codebase and to reduce duplication, consider replacing this block with calls to top_cc.Average_Point() and bottom_cc.Average_Point(), letting that API handle the empty/NaN cases.
There was a problem hiding this comment.
Replace this block with calls to top_cc.Average_Point() and bottom_cc.Average_Point().
There was a problem hiding this comment.
Replaced with calls to top_cc.Average_Point() and bottom_cc.Average_Point(). The implementation now uses isfinite() check on returned values to detect empty vertex collections. Commit f143399.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@copilot apply changes based on the comments in this thread |
…e, add empty vertex tests Co-authored-by: hdclark <934858+hdclark@users.noreply.github.com>
All review comments have been addressed in commit f143399:
|
Implements triangulation of the gap between two
contour_collections on parallel, offset planes, producing anfv_surface_meshthat bridges them.Changes
src/YgorMathContourConnectivity.h— function declaration with documentationsrc/YgorMathContourConnectivity.cc— greedy nearest-neighbor triangulation algorithm with edge-intersection validationtests2/YgorMath/contour_connectivity.cc— 5 test cases covering basic shapes, multiple contours, holes, oblique planes, empty vertex collections, and edge casesAlgorithm
contour_collection::Average_Point()APIUsage
Notes
floatanddouble✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.