Skip to content

Partition_2: add Container template parameter to Partition_traits_2#9351

Open
RajdeepKushwaha5 wants to merge 1 commit intoCGAL:mainfrom
RajdeepKushwaha5:fix/partition-traits-2-container-param
Open

Partition_2: add Container template parameter to Partition_traits_2#9351
RajdeepKushwaha5 wants to merge 1 commit intoCGAL:mainfrom
RajdeepKushwaha5:fix/partition-traits-2-container-param

Conversation

@RajdeepKushwaha5
Copy link
Contributor

@RajdeepKushwaha5 RajdeepKushwaha5 commented Mar 1, 2026

Fixes #7545

The Container type used for the vertices of the output Polygon_2 was hardcoded to std::list<Point_2> inside Partition_traits_2. This prevented users from requesting a different container (e.g., std::vector<Point_2>) for the output polygons produced by all partitioning functions (y_monotone_partition_2, approx_convex_partition_2, greene_approx_convex_partition_2, optimal_convex_partition_2).

Root Cause

Partition_traits_2.h line 65 had:
typedef ::std::list<Point_2> Container;
with no mechanism for users to change it.

Fix

Add a third template parameter to Partition_traits_2:
class Container_ = std::list<typename boost::property_traits::key_type>

The parameter defaults to std::list<Point_2>, preserving full backward
compatibility with all existing uses of Partition_traits_2<K> and
Partition_traits_2<K, PM>.

Users can now write:
typedef CGAL::Partition_traits_2<K, CGAL::Identity_property_map<K::Point_2>,
std::vector<K::Point_2>> VecTraits;
std::listVecTraits::Polygon_2 parts;
CGAL::optimal_convex_partition_2(pts.begin(), pts.end(),
std::back_inserter(parts), VecTraits());

Files Changed

  • Partition_2/include/CGAL/Partition_traits_2.h

    • Forward declaration: added Container_ template param with default.
    • Class definition: added Container_ to template param list and Self typedef.
    • Replaced hardcoded typedef ::std::list<Point_2> Container with
      typedef Container_ Container.
  • Partition_2/doc/Partition_2/CGAL/Partition_traits_2.h

    • Added \tparam Container documentation.
    • Updated template declaration in doc header.
    • Updated Container typedef description.
  • Partition_2/doc/Partition_2/PackageDescription.txt

    • Updated class listing from Partition_traits_2<R,P> to
      Partition_traits_2<R,P,Container>.

Copilot AI review requested due to automatic review settings March 1, 2026 16:50
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds a Container template parameter to CGAL::Partition_traits_2 so users can choose the vertex container type (e.g., std::vector<Point_2>) for the output Polygon_2 produced by Partition_2 algorithms, while preserving the existing defaults for backward compatibility.

Changes:

  • Extend Partition_traits_2 with a third template parameter Container_ (defaulting to std::list<Point_2>-equivalent) and use it for the Container typedef.
  • Update the reference documentation for Partition_traits_2 to document the new template parameter and updated typedef.
  • Update the package description class listing to reflect the new template signature.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
Partition_2/include/CGAL/Partition_traits_2.h Adds the Container_ template parameter and wires it into the Polygon_2 container type.
Partition_2/doc/Partition_2/CGAL/Partition_traits_2.h Documents the new container template parameter and updates the Container typedef description.
Partition_2/doc/Partition_2/PackageDescription.txt Updates the class list to show Partition_traits_2<R,P,Container>.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +28 to 31
template <class Base_traits,
class PointPropertyMap = Identity_property_map<typename Base_traits::Point_2>,
class Container_ = std::list<typename boost::property_traits<PointPropertyMap>::key_type> >
class Partition_traits_2;
Copy link

Copilot AI Mar 1, 2026

Choose a reason for hiding this comment

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

This change adds a new Container_ template parameter intended to let callers choose the output polygon vertex container, but there isn't a regression test exercising a non-default container (e.g., std::vector<Point_2>) with at least one partitioning function. Adding a small compile+run test would help ensure the new API works and stays supported.

Copilot uses AI. Check for mistakes.
Comment on lines 28 to 59
@@ -46,9 +51,11 @@ typedef boost::property_traits<PointPropertyMap>::key_type Point_2;


/*!

The sequence container used to store the vertices of the output polygon.
This is determined by the `Container` template parameter.
The default is `std::list<Point_2>`.
*/
typedef std::list<Point_2> Container;
typedef Container Container;

Copy link

Copilot AI Mar 1, 2026

Choose a reason for hiding this comment

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

In the documentation header, the template parameter is named Container, and the nested typedef is written as typedef Container Container;. This effectively redefines the template parameter name in the same scope and does not document the actual container type alias (it should alias the template parameter type, not itself). Consider renaming the template parameter to Container_ (matching the implementation) and defining typedef Container_ Container; so the documented API matches the real one and parses cleanly.

Copilot uses AI. Check for mistakes.
@afabri
Copy link
Member

afabri commented Mar 9, 2026

Before pushing you should test locally. This would have revealed the typedef Container Container; error copilot reports you.
Also please stop flooding this repository with pull requests, where one has the impression that you just started an agent.

\tparam PointPropertyMap a property map that maps to points of type `R::Point_2`
\tparam Container the container type used to store the vertices of the output
polygon. It must be a model of the `SequenceContainer` concept with
value type `Point_2`. Defaults to `std::list<Point_2>`.
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
value type `Point_2`. Defaults to `std::list<Point_2>`.
value type `R::Point_2`. Defaults to `std::list<R::Point_2>`.

Fixes CGAL#7545

The Container type used for the vertices of the output Polygon_2 was
previously hardcoded to std::list<Point_2> inside Partition_traits_2.
This prevented users from requesting a different container (e.g.,
std::vector<Point_2>) for the output polygons produced by all
partitioning functions.

Changes:
  * Partition_2/include/CGAL/Partition_traits_2.h
    - Add a third template parameter
        class Container_ = std::list<typename boost::property_traits<PointPropertyMap>::key_type>
      to both the forward declaration and the class definition.
    - Replace 'typedef ::std::list<Point_2> Container' with
      'typedef Container_ Container', making the typedef an alias for
      the template argument.
    - Update the Self typedef to include all three template parameters.
    All existing uses of Partition_traits_2<K> or Partition_traits_2<K,PM>
    are fully backward-compatible since the new parameter defaults to
    std::list, preserving the previous behaviour.

  * Partition_2/doc/Partition_2/CGAL/Partition_traits_2.h
    - Add \\tparam Container documentation.
    - Add Container to the class template declaration.
    - Document the Container typedef as reflecting the template parameter.

  * Partition_2/doc/Partition_2/PackageDescription.txt
    - Update the class listing to Partition_traits_2<R,P,Container>.
@RajdeepKushwaha5 RajdeepKushwaha5 force-pushed the fix/partition-traits-2-container-param branch from 10a96a1 to 9391fb5 Compare March 12, 2026 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The (Polygon) Partition traits polygon-definition is erroneous

3 participants