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

Updating the AABB-tree to work in both 2D and 3D #8057

Merged
merged 102 commits into from
May 15, 2024

Conversation

sloriot
Copy link
Member

@sloriot sloriot commented Mar 4, 2024

Make the AABB-tree dimension agnostic

Todo

  • update the implementation history (of several releases @sloriot )
  • Check that we have tests for the deprecated code (@soesau )
  • make it backward compatible
  • PackageDescription is missing deprecated AABB_traits
  • AABB_traits should be documented or not? --> document the template alias
    • Not sure, how to do this. They deprecated classes are listed in the PackageDescription with their replacement.
  • remove projection traits/Vector type/Point->Point_2 from example
  • Vertex point map in all 2D primitives as last template parameter
  • restructure polyline example into two functions
  • cleanup of types in AABB_traits_intersection_base_2/3 (private/public)
  • cleanup types to be private in AABB_traits_2/3
  • move Remove_optional in CGAL/AABB_tree/internal/Remove_optional.h
  • AABB_traits_base, is it used only n AABB_traits_3?
  • AABB_traits_intersection_base should have _3
  • AABB_tree still mentions 3D in comments
  • Is this guaranteed to be a actual point and not an index, etc.:
    • static const int dimension = AABB_traits::Point::Ambient_dimension::value;
    • yes, it is
  • breaking change to add in changes: refinement of spatial searching concept

Release Management

  • Affected package(s): AABB-tree
  • Issue(s) solved (if any):
  • Feature/Small Feature (if any): link, pre-approved by @MaelRL 22 April 2024
  • Link to compiled documentation (obligatory for small feature) link
  • License and copyright ownership: unchanged

@sloriot

This comment was marked as outdated.

This comment was marked as outdated.

@sloriot

This comment was marked as outdated.

Copy link

github-actions bot commented Mar 4, 2024

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

// AABB_traits_intersection_base brings in the Intersection_distance predicate,
// if GeomTraits is a model RayIntersectionGeomTraits.
template <typename GeomTraits, bool ray_intersection_geom_traits=Is_ray_intersection_geomtraits<GeomTraits>::value>
struct AABB_traits_intersection_base;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't they need the _3 suffix?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only renamed to avoid the _2 that would be misleading in the context of this PR. If nothing can be shared it indeed should be _3

- `CGAL::AABB_triangle_primitive_2<GeomTraits, Iterator, CacheDatum>`
- `CGAL::AABB_segment_primitive_2<GeomTraits, Iterator, CacheDatum>`
- `CGAL::AABB_triangle_primitive_3<GeomTraits, Iterator, CacheDatum>`
- `CGAL::AABB_segment_primitive_3<GeomTraits, Iterator, CacheDatum>`
- `CGAL::AABB_primitive<Id,ObjectPropertyMap,PointPropertyMap,ExternalPropertyMaps,CacheDatum>`
- `CGAL::AABB_halfedge_graph_segment_primitive<HalfedgeGraph,Vpm,OneHalfedgeGraphPerTree,CacheDatum>`
- `CGAL::AABB_face_graph_triangle_primitive<FaceGraph,Vpm,OneFaceGraphPerTree,CacheDatum>`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here an equivalent would be useful when we shoot rays inside a polygon.

AABB_tree/doc/AABB_tree/Concepts/AABBGeomTraits_2.h Outdated Show resolved Hide resolved
AABB_tree/doc/AABB_tree/Concepts/AABBTraits.h Show resolved Hide resolved
@@ -145,21 +138,21 @@ typedef unspecified_type Compare_distance;

/*!
A functor object to compute closest point from the query on a primitive. Provides the operator:
`Point_3 operator()(const Query& query, const Primitive& primitive, const Point_3 & closest);` which returns the closest point to `query`, among `closest` and all points of the primitive.
`Point operator()(const Query& query, const Primitive& primitive, const Point & closest);` which returns the closest point to `query`, among `closest` and all points of the primitive.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, but even with the explanation it's not clear to me and I don't know how I would implement this.

We can put it aside because it's not really what the PR is about...

@@ -5,12 +5,12 @@

/*!
\addtogroup PkgAABBTreeRef
\cgalPkgDescriptionBegin{3D Fast Intersection and Distance Computation,PkgAABBTree}
\cgalPkgDescriptionBegin{2D and 3D Fast Intersection and Distance Computation,PkgAABBTree}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
\cgalPkgDescriptionBegin{2D and 3D Fast Intersection and Distance Computation,PkgAABBTree}
\cgalPkgDescriptionBegin{Fast Intersection and Distance Computation,PkgAABBTree}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WIth or without 2D and 3D seems fine. The AABBTree should be able to handle d dimensions.

@MaelRL MaelRL added pre-approved For pre-approved small features. After 15 days the feature will be accepted. Ready to be tested and removed Not yet approved The feature or pull-request has not yet been approved. labels Apr 22, 2024
Co-authored-by: Mael <mael.rouxel.labbe@geometryfactory.com>
@sloriot
Copy link
Member Author

sloriot commented Apr 24, 2024

Successfully tested in CGAL-6.0-Ic-228

@github-actions github-actions bot removed the Tested label Apr 25, 2024
Copy link

This pull-request was previously marked with the label Tested, but has been modified with new commits. That label has been removed.

@sloriot sloriot added Under Testing Accepted small feature and removed pre-approved For pre-approved small features. After 15 days the feature will be accepted. labels May 2, 2024
@sloriot
Copy link
Member Author

sloriot commented May 6, 2024

Successfully tested in CGAL-6.0-Ic-235

@lrineau lrineau self-assigned this May 8, 2024
@lrineau
Copy link
Member

lrineau commented May 8, 2024

Successfully tested in CGAL-6.0-Ic-235

AABB_tree is really an important package for many CGAL packages, and for a lot of user code, including CGAL developers out-of-tree pieces of code.

As far as I can see, this package deprecates <CGAL/AABB_traits.h> and CGAL::AABB_traits, and we now have an template alias for backward compatibility.

template<typename GeomTraits, typename AABBPrimitive, typename BboxMap = Default>
using AABB_traits = AABB_traits_3<GeomTraits, AABBPrimitive, BboxMap>;

And similar for a new template classes about "primitives".

There are only two small test files testing the for deprecated names. Can we really be confident it is enough to trust that the compatibility is working well enough?

@sloriot
Copy link
Member Author

sloriot commented May 10, 2024

If you first apply the boost::optional -> std::optional update in CGAL-5.6 examples, the code compiles.
This PR does not really break something. The "real" breaking change is that the concept AABBTraits now refines the SearchTraits concept (instead of SearchGeomTraits_3). It means that a user defined traits might not longer works (but it's kind of unlikely to happen). Also, the swig bindings will no longer compile as they are using some classes that were deprecated since many years. I'll do the update after the merge.

@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 May 13, 2024
@lrineau lrineau merged commit 022b19d into CGAL:master May 15, 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 May 16, 2024
@lrineau lrineau deleted the AABB_tree-2d_and_3d branch May 16, 2024 10:26
@lrineau
Copy link
Member

lrineau commented May 23, 2024

If you first apply the boost::optional -> std::optional update in CGAL-5.6 examples, the code compiles. This PR does not really break something. The "real" breaking change is that the concept AABBTraits now refines the SearchTraits concept (instead of SearchGeomTraits_3). It means that a user defined traits might not longer works (but it's kind of unlikely to happen). Also, the swig bindings will no longer compile as they are using some classes that were deprecated since many years. I'll do the update after the merge.

@sloriot Can you please fix https://github.com/CGAL/cgal-swig-bindings?

See https://github.com/CGAL/cgal-swig-bindings/actions/runs/9144362571/job/25142119159#step:9:731

With all the downstream packagers testing our releases automatically, even the beta ones, that compilation error is a blocker for CGAL-6.0-beta1.

@sloriot
Copy link
Member Author

sloriot commented May 24, 2024

If you first apply the boost::optional -> std::optional update in CGAL-5.6 examples, the code compiles. This PR does not really break something. The "real" breaking change is that the concept AABBTraits now refines the SearchTraits concept (instead of SearchGeomTraits_3). It means that a user defined traits might not longer works (but it's kind of unlikely to happen). Also, the swig bindings will no longer compile as they are using some classes that were deprecated since many years. I'll do the update after the merge.

@sloriot Can you please fix https://github.com/CGAL/cgal-swig-bindings?

See https://github.com/CGAL/cgal-swig-bindings/actions/runs/9144362571/job/25142119159#step:9:731

With all the downstream packagers testing our releases automatically, even the beta ones, that compilation error is a blocker for CGAL-6.0-beta1.

Done in CGAL/cgal-swig-bindings#270

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.

None yet

5 participants