Skip to content
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

Orthtree generalization #7672

Merged
merged 311 commits into from
Mar 22, 2024
Merged

Conversation

JacksonCampolattaro
Copy link
Member

@JacksonCampolattaro JacksonCampolattaro commented Aug 28, 2023

Summary of Changes

Orthtree is no longer restricted to only subdividing a Point_set_3, the contents of the tree are now a customization point of its Traits type.

Orthtree nodes are now represented as IDs. All node data is now stored using a new property system which allows users to add custom data to nodes. The new property system is included in Property_map. It initially also replaced the property system used by Surface_mesh and Point_set_3, but as this introduced problems we postpone that.

Release Management

  • Affected package(s): Orthtree
  • Issue(s) solved (if any): N/A
  • Feature/Small Feature (if any): link pre-approved on 24/03/06
  • Link to compiled documentation link
  • License and copyright ownership: INRIA

TODO

  • Polish the Property system
    • Give Properties.h a more descriptive name
    • Fix issue with emplace_group on MSVC.
    • Fix iteration over deleted elements. unsplit is not implemented so no issue for now.
    • Delete old property system from Surface_mesh
    • Expand documentation of Property_array, Property_container, etc. These will be left undocumented for now, as we expect the API to change in the future.
    • Consider preferring the use of Property_array_handle and eliminating direct access to Property_array; mixing the two is a potential source of confusion. This doesn't need to be handled until the new property system is documented, but I'd like to come back to this if there's time.
  • Polish the generalized Orthtree
    • Merge Orthtree_traits_point_* classes; only the typedefs are different, so the functionality should be de-duplicated.
      • Use template machinery to choose the appropriate base class based on the dimension tag.
    • Ensure shared corners of adjacent nodes have identical coordinates (Sven's solution should resolve this)
      • This should have a unit test
      • Seems to be working after switching to Iso_cubioid. I might just need to find the right numerical edge case though.
    • Remove enlarge_ratio feature
      • Call bbox.dilate(1) instead; this behavior is more appropriate in almost all cases. This does not seem to be necessary now that Iso_cuboid is used.
      • Add an example that extends a Traits class to provide the enlargement feature using a modified root_node_bbox functor.
    • Replace Array type in Traits.
      • Remove integer offsets. Simple_cartesian_d<int>::Point may work as an alternative, but I'll need generic kernel conversion to integrate it. An array type is still used internally, but never exposed to the user.
    • Replace Bbox with Iso_cuboid
    • Change the new Traits methods to use the functor object pattern. (Is there a page I can link to in my documentation that explains why this pattern is necessary?)
      • Document functor objects (may require some template magic, along with the macros for unspecified_type) Functor objects only need to be documented in the concept itself, because the explanation will appear in model documentation too.
    • Nearest neighbors functionality only makes sense for some traits, and only has examples which work with point sets. It should become an external function. This may also let us remove element access from the traits class.
    • Comb through documentation to make sure everything matches the new interface.
      • Update user manual text
      • Some features have been moved to Traits, their explanations should be moved.
      • Orthtree documentation should not specifically mention points or point sets, now that it has been generalized.
    • Update list of models for orthtree traits
    • Expand and clarify root_node_contents_object() documentation.
    • Replace all remaining instances of typedef with using for consistency.
      • Does using Dimension = unspecified_type; break doxygen? If not, Concepts should also use using.
    • Run benchmarks to measure performance change vs original Orthtree implementation.
      • Benchmark results should be mentioned in the history section.
      • Benchmark plots must be re-created for the manual.
    • Implement unsplit(n). This features isn't actually used anywhere now, but it could be useful in the future.
    • The new orthtree requires boost::span; the boost requirement will either need to be bumped to 1.78, or a span can be added to Stl_extension.
  • Adapt Efficient_RANSAC package to account for the new interface
  • Adapt Polyhedron package to account for non-nullable property handles
  • Double check license headers in all newly added files. Dates may need to be updated in some other files.
  • Fix bbox computation (doc box overlaps?)
  • Update changes.md
  • Check branch size(todo for @sloriot)

JacksonCampolattaro and others added 29 commits June 22, 2023 10:23
bug: moved-from is not reset
(only Point_3 is currently supported)
There's a lot of duplicated code in the traits classes, at the very least `reassign_points` should be shared.
example currently segfault, to be debugged...
…ence to one)

This resolves the issue with segfaults during traversal. In some circumstances, the index would go out of scope before being used for access.
@soesau
Copy link
Member

soesau commented Mar 5, 2024

/force-build:v0

Copy link

github-actions bot commented Mar 5, 2024

The documentation is built. It will be available, after a few minutes, here: https://cgal.github.io/7672/v0/Manual/index.html

@sloriot sloriot added the pre-approved For pre-approved small features. After 15 days the feature will be accepted. label Mar 6, 2024
@sloriot sloriot added Batch_1 First Batch of PRs under testing Accepted small feature Tested and removed Under Testing Not yet approved The feature or pull-request has not yet been approved. pre-approved For pre-approved small features. After 15 days the feature will be accepted. Batch_1 First Batch of PRs under testing labels Mar 20, 2024
@sloriot
Copy link
Member

sloriot commented Mar 21, 2024

Successfully tested in 6.0-Ic-197

@lrineau lrineau self-assigned this Mar 21, 2024
@lrineau lrineau added the rm only: ready for master For the release team only: that indicates that a PR is about to be merged in 'master' label Mar 21, 2024
@lrineau lrineau merged commit 897499e into CGAL:master Mar 22, 2024
9 checks passed
@lrineau lrineau removed the rm only: ready for master For the release team only: that indicates that a PR is about to be merged in 'master' label Mar 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants