Skip to content

[Feature] Weights New Package#5737

Merged
sloriot merged 60 commits intoCGAL:masterfrom
danston:Weights-new_package-danston
Aug 12, 2021
Merged

[Feature] Weights New Package#5737
sloriot merged 60 commits intoCGAL:masterfrom
danston:Weights-new_package-danston

Conversation

@danston
Copy link
Contributor

@danston danston commented May 28, 2021

This PR proposes a new Feature called Weights that implements a set of analytic and barycentric weights.

The weights implemented are:

  • Wachspress weight
  • authalic weight
  • discrete harmonic weight
  • cotangent weight
  • mean value weight
  • tangent weight
  • three-point family weight
  • uniform weight
  • Shepard weight
  • inverse distance weight
  • uniform region weight
  • triangular region weight
  • barycentric region weight
  • Voronoi region weight
  • mixed Voronoi region weight

This package is used in the new version (PR #5738) of the Barycentric_coordinates_2 package for computing weights. It will also be used later to update several CGAL packages with new weights: Polygon mesh Processing, several Surface Mesh related packages, and Heat Method to unify the weight usage among different CGAL packages.

Release Management

This PR depends on #5736.

  • Affected package(s): Feature, New Package
  • Issue(s) solved (if any): no issues
  • Feature/Small Feature (if any): Feature
  • Link to compiled documentation (obligatory for small feature): docs
  • License and copyright ownership: GF

@MaelRL MaelRL added Feature Not yet approved The feature or pull-request has not yet been approved. labels May 31, 2021
@MaelRL MaelRL added this to the 5.4-beta milestone May 31, 2021
@maxGimeno
Copy link
Contributor

@danston
Copy link
Contributor Author

danston commented Aug 10, 2021

@maxGimeno Isn't this warning related to the Surface_mesh? It does not seem to be caused by the Weights package. I simply use surface mesh to store a few faces and vertices in the corresponding example.

@maxGimeno
Copy link
Contributor

@danston oh, you are right

@lrineau
Copy link
Member

lrineau commented Aug 10, 2021

@maxGimeno Isn't this warning related to the Surface_mesh? It does not seem to be caused by the Weights package. I simply use surface mesh to store a few faces and vertices in the corresponding example.

That warning is actually here:

/mnt/testsuite/include/CGAL/Surface_mesh/Surface_mesh.h: In constructor 'CGAL::Surface_mesh<P>::Surface_mesh() [with P = CGAL::Point_3<CGAL::Simple_cartesian<double> >]':
/mnt/testsuite/include/CGAL/Surface_mesh/Surface_mesh.h:2201:54: note: '<anonymous>' declared here
 2201 |     vpoint_   = add_property_map<Vertex_index, Point>("v:point").first;
      |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~

The compiler cannot know that add_property_map<Vertex_index, Point>("v:point") will not fail.

The code should be:

auto add_vpoint_pmap = add_property_map<Vertex_index, Point>("v:point");
CGAL_assume(add_vpoint_pmap.second == true);
vpoint_ = add_vpoint_pmap.first;

@danston
Copy link
Contributor Author

danston commented Aug 10, 2021

@lrineau I suppose it should be a different PR to fix it, not this one, right?

@lrineau
Copy link
Member

lrineau commented Aug 10, 2021

@lrineau I suppose it should be a different PR to fix it, not this one, right?

That is right. And it should be discussed with the authors of Surface_mesh.h, in particular @afabri and @sloriot.

@maxGimeno
Copy link
Contributor

@sloriot
Copy link
Member

sloriot commented Aug 12, 2021

@maxGimeno Isn't this warning related to the Surface_mesh? It does not seem to be caused by the Weights package. I simply use surface mesh to store a few faces and vertices in the corresponding example.

That warning is actually here:

/mnt/testsuite/include/CGAL/Surface_mesh/Surface_mesh.h: In constructor 'CGAL::Surface_mesh<P>::Surface_mesh() [with P = CGAL::Point_3<CGAL::Simple_cartesian<double> >]':
/mnt/testsuite/include/CGAL/Surface_mesh/Surface_mesh.h:2201:54: note: '<anonymous>' declared here
 2201 |     vpoint_   = add_property_map<Vertex_index, Point>("v:point").first;
      |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~

The compiler cannot know that add_property_map<Vertex_index, Point>("v:point") will not fail.

The code should be:

auto add_vpoint_pmap = add_property_map<Vertex_index, Point>("v:point");
CGAL_assume(add_vpoint_pmap.second == true);
vpoint_ = add_vpoint_pmap.first;

Returning false does not mean failing, it means that the property already exist internally. I'd be surprised if the proposed code fixes the warning.

@danston
Copy link
Contributor Author

danston commented Aug 12, 2021

Should I prepare a small news entry for this feature for cgal.org?

@sloriot
Copy link
Member

sloriot commented Aug 12, 2021

Sure

@sloriot
Copy link
Member

sloriot commented Aug 12, 2021

I pushed 89aec88 directly in master adding the missing entry in the package overview.

@danston danston deleted the Weights-new_package-danston branch August 18, 2021 11:09
@MaelRL MaelRL added the Feature label Aug 23, 2021
@danston
Copy link
Contributor Author

danston commented Nov 16, 2021

@MaelRL I have prepared a news entry for cgal.org and sent you it in pm. Thanks.

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.

6 participants