attempt to fix bsp trees implementation - #88
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1244342c42
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| #include <tuple> | ||
| #include <utility> | ||
| #include <vector> | ||
|
|
||
| #include <boost/multiprecision/cpp_int.hpp> |
There was a problem hiding this comment.
In builds that do not already have Boost headers installed, this new include makes the library fail to compile before ygor is built; I checked the repo build metadata and CMakeLists.txt/nix/ygor_derivation.nix still only declare the existing dependencies, not Boost. Either avoid Boost here or add it to the declared build/package dependencies so clean environments can build this commit.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Do not introduce any new dependencies. Remove use of Boost. Instead, implement a replacement integer multiprecision class to emulate the cpp_int class provided by boost/multiprecision/cpp_int.hpp. Move the existing multiprecision classes and helper functions (ExactInt, ExactScalar, ExactPoint2, ExactPlane3, ExactLine3, etc.) from src/YgorMeshesBSPTree.cc into new, self-contained files src/YgorMultiprecision.{h,cc} so they can be added as a library. Do not use the header-only approach so that multiple copies can potentially be linked by downstream consumers. Add unit tests for the multiprecision class and Exact* classes and helpers.
| Arrangement3DCell inside; | ||
| inside.plane_signs.assign(result.planes.size(), int8_t(-1)); | ||
| inside.classification = ArrangementCellClass::In; | ||
| result.cells.push_back(std::move(inside)); |
There was a problem hiding this comment.
For any valid non-convex, disconnected, or nested-shell mesh, replacing the real classified cells with a single all-negative In cell means from_fv_surface_mesh() builds the intersection of every support-plane back halfspace rather than the actual solid. For example, two disjoint closed boxes or a box with an inward-wound cavity have no single cell that is behind every surface plane, so conversion either loses components/holes or produces an empty/incorrect volume instead of the input mesh.
Useful? React with 👍 / 👎.
| for(size_t i = 0; i < c.size(); ++i) { | ||
| key.coeffs[i] = c[i].n * (lcm / c[i].d); | ||
| common = (i == 0) ? abs_exact_int(key.coeffs[i]) : gcd_exact_int(common, key.coeffs[i]); | ||
| } |
There was a problem hiding this comment.
When the first two plane coefficients are zero, as on horizontal planes, this gcd accumulation sets common to 1 after seeing (0, 0) and never removes later common factors. A z = 0 plane from a triangle with normal (0,0,2) therefore gets a different key from the same plane with normal (0,0,1), so coplanar horizontal facets with different triangle areas are treated as distinct support planes and exact coplanar merge detection can miss them.
Useful? React with 👍 / 👎.
| if(tree_has_extractable_bounded_boundary<T, I>(node.get())) return node; | ||
| return make_out_node<T, I>(); |
There was a problem hiding this comment.
For public BSPs that represent an unbounded but non-empty volume, such as a single partition with one In half-space, tree_has_extractable_bounded_boundary() returns false because the boundary polygon is unbounded, and this turns the result into Out. That makes operations like intersecting a half-space with itself silently become empty even though the boolean tree can represent the unbounded result.
Useful? React with 👍 / 👎.
| s2.push_back(true); | ||
| extract_boundary_faces<T, I>(node->front.get(), a2, s2, | ||
| bbox_size, faces); | ||
| if(!exact_closed) return; |
There was a problem hiding this comment.
If boundary extraction misses a face or otherwise produces exact triangles whose edges are not paired, exact_closed becomes false and validation returns after only the finite/index/degenerate checks. In that failure mode to_fv_surface_mesh() can hand callers an open, non-manifold boolean result instead of surfacing the internal extraction error.
Useful? React with 👍 / 👎.
|
|
||
| if(!IsTriangularMesh(working_mesh)) | ||
| throw std::invalid_argument("bsp_tree_volume::from_fv_surface_mesh: mesh must contain only triangular faces."); | ||
| if(!HasValidFaceIndices(working_mesh)) | ||
| throw std::invalid_argument("bsp_tree_volume::from_fv_surface_mesh: mesh contains out-of-range face indices."); | ||
| working_mesh.remove_degenerate_faces(); |
There was a problem hiding this comment.
For meshes that contain a mix of valid faces and zero-area faces, this removes the bad faces before the exact canonicalization path can report them. That silently changes invalid input into a different mesh, so callers never get the explicit invalid_argument promised for zero-area faces unless every face was removed.
Useful? React with 👍 / 👎.
| const vec3<T> dl = exact_point_to_vec3<T>(lhs) - c; | ||
| const vec3<T> dr = exact_point_to_vec3<T>(rhs) - c; | ||
| const T al = std::atan2(dl.Dot(v), dl.Dot(u)); | ||
| const T ar = std::atan2(dr.Dot(v), dr.Dot(u)); | ||
| if(al != ar) return al < ar; | ||
| return exact_point_less(lhs, rhs); |
There was a problem hiding this comment.
When a boundary polygon has vertices with very close angular positions or large coordinates, converting exact points back to T for atan2 can round two distinct angles together or reorder them. The following fan triangulation then uses that rounded order, which can create inverted or self-crossing output even though the exact coordinates still define a valid face.
Useful? React with 👍 / 👎.
|
@codex address the feedback from thread #88 (review) |
|
Summary
Testing
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c3b2cdac85
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| if(tris.empty()) return mesh; |
There was a problem hiding this comment.
When callers build a public BSP for a half-space, e.g. a Partition node with In on one side and Out on the other, extract_boundary_faces_from_tree() cannot form any bounded polygon because there are no additional clipping constraints, so tris is empty here and the method returns an empty mesh. That makes a non-empty unbounded volume indistinguishable from Out; the root-In case just above already treats unbounded volumes as unmeshable, so this path should also throw or otherwise signal that the BSP is unbounded instead of returning empty output.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Implement this suggestion, throwing to signal that the result is unmeshable.
| const size_t front_cell = cell_id_for(classify_near_patch(centroid, normal, plane_id), | ||
| ArrangementCellClass::Out, | ||
| arrangement); | ||
| const size_t back_cell = cell_id_for(classify_near_patch(centroid, normal * ExactScalar(-1), plane_id), | ||
| ArrangementCellClass::In, |
There was a problem hiding this comment.
For valid high-aspect meshes, using the full unnormalized face normal as the offset can jump past other support planes instead of sampling the cell adjacent to this patch. Fresh evidence is that this code now derives signs from centroid ± normal; for a very thin box with a large side face, the inward sample can land beyond the opposite side, so the same sign vector may be recorded as both In and Out and BSP construction can throw or classify the solid incorrectly.
Useful? React with 👍 / 👎.
|
@codex address the feedback in thread #88 (review) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c3b2cdac85
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| if(tris.empty()) return mesh; |
There was a problem hiding this comment.
When callers build a public BSP for a half-space, for example a Partition node with one In child and one Out child, extract_boundary_faces_from_tree() cannot form a bounded polygon because there are no clipping constraints, so tris stays empty here and the method returns an empty mesh. That makes a non-empty unbounded volume indistinguishable from Out; the root-In case above already treats unbounded volumes as unmeshable, so this path should throw or otherwise signal that the result is unbounded instead of returning an empty mesh.
Useful? React with 👍 / 👎.
| const size_t front_cell = cell_id_for(classify_near_patch(centroid, normal, plane_id), | ||
| ArrangementCellClass::Out, | ||
| arrangement); | ||
| const size_t back_cell = cell_id_for(classify_near_patch(centroid, normal * ExactScalar(-1), plane_id), |
There was a problem hiding this comment.
For high-aspect valid meshes, using the full unnormalized face normal as the sampling offset can jump past nearby support planes instead of sampling the cell adjacent to this patch. In a very thin box with a large side face, centroid ± normal can land beyond the opposite face, so the recorded sign vector may be assigned to the wrong In/Out side or become contradictory, causing from_fv_surface_mesh() to throw or build an incorrect BSP.
Useful? React with 👍 / 👎.
| if(same_oriented_cycle(seen->second, idx)) { | ||
| continue; |
There was a problem hiding this comment.
For an input mesh that contains a duplicate triangle with the same winding, this continue drops the extra face before the edge-manifold checks run, so a non-manifold/overlapping input is accepted and converted as though the duplicate never existed. Since the converter is supposed to reject invalid closed-surface inputs rather than silently changing them, same-orientation duplicates should throw just like opposite-orientation duplicates.
Useful? React with 👍 / 👎.
| const T frac = std::frexp(value, &exp); | ||
| const int digits = std::numeric_limits<T>::digits; | ||
| const T scaled = std::ldexp(frac, digits); | ||
| const auto mantissa = static_cast<int64_t>(scaled); |
There was a problem hiding this comment.
When from_binary() is instantiated for long double on platforms with a 64-bit significand, even 1.0L produces a scaled mantissa of 2^63, which is outside int64_t; the narrowing cast then yields an implementation-defined negative value and the exact scalar has the wrong sign/value. Because this helper is public and accepts any IEC 559 floating type, long-double callers get corrupted exact coordinates instead of an exact conversion.
Useful? React with 👍 / 👎.
1 participant
No description provided.