-
Notifications
You must be signed in to change notification settings - Fork 223
feat: Implement is_valid algorithm for polyhedral surfaces #1409
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR enhances the validity checking algorithm by introducing support for polyhedral surfaces in line with the OGC Simple Features Specification. Key changes include:
- Adding several new failure types for polyhedral surfaces (e.g. collinear, non-coplanar points, inconsistent orientation, etc.).
- Extending the test suite in is_valid.cpp with comprehensive cases for various valid and invalid polyhedral surfaces.
- Updating include directives to incorporate polyhedral surface support in the core algorithm implementation.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
test/algorithms/is_valid.cpp | Adds polyhedral surface test cases and minor documentation updates. |
include/boost/geometry/algorithms/validity_failure_type.hpp | Introduces new failure codes specific to polyhedral surfaces. |
include/boost/geometry/algorithms/detail/is_valid/implementation.hpp | Adds header inclusion for polyhedral surface implementation. |
include/boost/geometry/algorithms/detail/is_valid/polyhedral_surface.hpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noted some points inline, mostly minor but importantly (imho) the question of separating some computations into strategy.
A minor point, this PR should probably include the removal of
//TODO: add is_valid() check |
include/boost/geometry/algorithms/detail/is_valid/polyhedral_surface.hpp
Outdated
Show resolved
Hide resolved
include/boost/geometry/algorithms/detail/is_valid/polyhedral_surface.hpp
Outdated
Show resolved
Hide resolved
include/boost/geometry/algorithms/detail/is_valid/polyhedral_surface.hpp
Outdated
Show resolved
Hide resolved
include/boost/geometry/algorithms/detail/is_valid/polyhedral_surface.hpp
Outdated
Show resolved
Hide resolved
include/boost/geometry/algorithms/detail/is_valid/polyhedral_surface.hpp
Outdated
Show resolved
Hide resolved
include/boost/geometry/algorithms/detail/is_valid/polyhedral_surface.hpp
Outdated
Show resolved
Hide resolved
include/boost/geometry/algorithms/detail/is_valid/polyhedral_surface.hpp
Outdated
Show resolved
Hide resolved
include/boost/geometry/algorithms/detail/is_valid/polyhedral_surface.hpp
Outdated
Show resolved
Hide resolved
include/boost/geometry/algorithms/detail/is_valid/polyhedral_surface.hpp
Outdated
Show resolved
Hide resolved
include/boost/geometry/algorithms/detail/is_valid/polyhedral_surface.hpp
Outdated
Show resolved
Hide resolved
include/boost/geometry/algorithms/detail/is_valid/polyhedral_surface.hpp
Outdated
Show resolved
Hide resolved
Looks great! I will review it next Wednesday or Thursday. |
fe6743a
to
36cb224
Compare
include/boost/geometry/algorithms/detail/is_valid/polyhedral_surface.hpp
Outdated
Show resolved
Hide resolved
@tinko92 thanks for the comments. I fixed all but the ones about tolerance and the strategy. I need to think of a unified and consistent solution for those. |
include/boost/geometry/algorithms/detail/is_valid/polyhedral_surface.hpp
Outdated
Show resolved
Hide resolved
include/boost/geometry/algorithms/detail/is_valid/polyhedral_surface.hpp
Outdated
Show resolved
Hide resolved
include/boost/geometry/algorithms/detail/is_valid/polyhedral_surface.hpp
Outdated
Show resolved
Hide resolved
include/boost/geometry/algorithms/detail/is_valid/polyhedral_surface.hpp
Outdated
Show resolved
Hide resolved
include/boost/geometry/algorithms/detail/is_valid/polyhedral_surface.hpp
Outdated
Show resolved
Hide resolved
include/boost/geometry/algorithms/detail/is_valid/polyhedral_surface.hpp
Show resolved
Hide resolved
include/boost/geometry/algorithms/detail/is_valid/polyhedral_surface.hpp
Outdated
Show resolved
Hide resolved
include/boost/geometry/algorithms/detail/is_valid/polyhedral_surface.hpp
Outdated
Show resolved
Hide resolved
include/boost/geometry/algorithms/detail/is_valid/polyhedral_surface.hpp
Outdated
Show resolved
Hide resolved
include/boost/geometry/algorithms/detail/is_valid/polyhedral_surface.hpp
Outdated
Show resolved
Hide resolved
include/boost/geometry/algorithms/detail/is_valid/polyhedral_surface.hpp
Outdated
Show resolved
Hide resolved
include/boost/geometry/algorithms/detail/is_valid/polyhedral_surface.hpp
Outdated
Show resolved
Hide resolved
include/boost/geometry/algorithms/detail/is_valid/polyhedral_surface.hpp
Outdated
Show resolved
Hide resolved
include/boost/geometry/algorithms/detail/is_valid/polyhedral_surface.hpp
Outdated
Show resolved
Hide resolved
include/boost/geometry/algorithms/detail/is_valid/polyhedral_surface.hpp
Outdated
Show resolved
Hide resolved
571d387
to
0295306
Compare
@barendgehrels thanks for you comments! I addressed most of them. I need to work more on this PR. Apart from the orient3d some other simplifications can be implemented (the unaddressed comments of Barend are related to those simplifications). I will work on it next week. |
16c3e87
to
c45c3b7
Compare
I have updated this PR. Now strategies are used (with 2d and 3d orientation predicates) and there are no constructions on new geometries apart from coordinate projections (that are not that vulnerable to numerical errors). Also a new algorithm for segment-polygon intersection in 3D that only uses orientation predicates is implemented. |
c45c3b7
to
895de6b
Compare
include/boost/geometry/strategies/cartesian/side_3d_rounded_input.hpp
Outdated
Show resolved
Hide resolved
include/boost/geometry/strategies/cartesian/side_3d_rounded_input.hpp
Outdated
Show resolved
Hide resolved
include/boost/geometry/strategies/cartesian/side_rounded_input.hpp
Outdated
Show resolved
Hide resolved
include/boost/geometry/algorithms/detail/is_valid/polyhedral_surface.hpp
Show resolved
Hide resolved
include/boost/geometry/algorithms/detail/is_valid/polyhedral_surface.hpp
Outdated
Show resolved
Hide resolved
Cool changes, thanks for the splits - I'll try to finish my review this weekend |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all the work on incorporating the requested changes! This looks good to me. I left some comments on formatting (with diffs, see attached txt files in comments) and compiler warnings, but nothing that would require another review as far as I can see, so I'm marking it as approve.
include/boost/geometry/algorithms/detail/is_valid/polyhedral_surface.hpp
Outdated
Show resolved
Hide resolved
include/boost/geometry/algorithms/detail/is_valid/polyhedral_surface.hpp
Outdated
Show resolved
Hide resolved
include/boost/geometry/algorithms/detail/is_valid/polyhedral_surface.hpp
Outdated
Show resolved
Hide resolved
include/boost/geometry/algorithms/detail/is_valid/polyhedral_surface.hpp
Outdated
Show resolved
Hide resolved
895de6b
to
4a27cd5
Compare
Thanks for your reviews! I addressed all the comments. |
include/boost/geometry/algorithms/detail/is_valid/polyhedral_surface.hpp
Show resolved
Hide resolved
include/boost/geometry/algorithms/detail/is_valid/polyhedral_surface.hpp
Outdated
Show resolved
Hide resolved
include/boost/geometry/algorithms/detail/is_valid/polyhedral_surface.hpp
Outdated
Show resolved
Hide resolved
include/boost/geometry/algorithms/detail/is_valid/polyhedral_surface.hpp
Show resolved
Hide resolved
include/boost/geometry/algorithms/detail/overlay/graph/assign_counts.hpp
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Big step! Thanks!
Thanks for the quick response. The latest force push broke |
4a27cd5
to
50bc0c1
Compare
50bc0c1
to
0ff86d1
Compare
This PR introduces support for the
is_valid
algorithm for polyhedral surface geometries. A polyhedral_surface is a 3D composite geometry formed by a collection of polygonal faces that meet at shared edges. The implementation follows the OGC Simple Features Specification and introduces structural and topological checks for polyhedral surfaces.The following properties should hold (following OGC standard):
Six new validity failure types has been introduced that reflect the description of the standard.
Below there is a matrix with valid and invalid cases with visualizations. Those cases (and a few more) are included in the unit tests of this PR.
Two unit tests are failing and are commented out. Those are dependent on #1406