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

PMP: Replace Location_traits by a simpler API #4277

Merged
merged 3 commits into from
Nov 6, 2019

Conversation

MaelRL
Copy link
Member

@MaelRL MaelRL commented Oct 8, 2019

Summary of Changes

Users have rightfully pointed out that the API used in PMP Location function was not practical, sometimes requiring the user to pass the type of the named parameters, which is very unpractical.

Instead, template aliases for most types, and template by the field type (FT) when necessary, which is much easier to write for the user. Since template aliases cannot automatically deduce template parameters, a non-Doxygen version of a function is written without them from time to time.

updated doc: https://cgal.geometryfactory.com/~mrouxell/PMP_locate_v3/Polygon_mesh_processing/group__PMP__locate__grp.html

This PR also adds an example for the PMP Location functions.

Release Management

  • Affected package(s): Polygon_mesh_processing
  • Issue(s) solved (if any): --

@MaelRL MaelRL added this to the 5.0-beta2 milestone Oct 8, 2019
@MaelRL MaelRL requested a review from sloriot October 8, 2019 13:53
@MaelRL MaelRL force-pushed the PMP-Locate_rework_traits-GF branch from 203de6f to 22092a3 Compare October 8, 2019 13:55
@sloriot
Copy link
Member

sloriot commented Oct 8, 2019

This PR targets the 5.0 release because the feature is new in this release.

@lrineau
Copy link
Member

lrineau commented Oct 10, 2019

locate was introduced by https://cgal.geometryfactory.com/CGAL/Members/wiki/Features/Small_Features/PMP-Locate. @janetournois, can you please have a look at the new API, and approve it again?

typedef typename boost::graph_traits<Mesh>::vertex_descriptor vertex_descriptor;
typedef typename boost::graph_traits<Mesh>::face_descriptor face_descriptor;

namespace CP = CGAL::parameters;
Copy link
Member

Choose a reason for hiding this comment

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

I don't mind too much, but it seems to me that everywhere else (at least in this package) we call this namespace NP for "Named Parameters".
Is there a reason to change that?

@janetournois
Copy link
Member

I re-approve the new documentation (and wrote it in the small feature).

There is only one detail I already mentioned in the my first review is : for template parameters descriptions, I would stick to "a model of" everywhere, and avoid mixing with "must be a model of", which makes the reader wonder why this one is different, imho.

@lrineau
Copy link
Member

lrineau commented Oct 15, 2019

Travis still find errors:
https://travis-ci.org/CGAL/cgal/jobs/598094517#L7619

@lrineau lrineau self-assigned this Oct 15, 2019
@lrineau
Copy link
Member

lrineau commented Oct 15, 2019

Actually, I do not understand why Travis sees new dependencies in the Property_maps package.

@MaelRL
Copy link
Member Author

MaelRL commented Oct 15, 2019

Yes, I couldn't understand either, so I thought it might have been an issue present in master when I forked (doesn't ring a bell, though), and ignored it. I can try updating it with master.

@MaelRL MaelRL changed the base branch from master to releases/CGAL-4.14-branch October 29, 2019 18:08
@MaelRL MaelRL changed the base branch from releases/CGAL-4.14-branch to master October 29, 2019 18:08
lrineau added a commit that referenced this pull request Oct 31, 2019
PMP: Replace `Location_traits` by a simpler API
@lrineau lrineau added rm only: ready for release branch For the release team only: that indicates that a PR is about to be merged in a release branch and removed rm only: ready for release branch For the release team only: that indicates that a PR is about to be merged in a release branch labels Oct 31, 2019
@lrineau lrineau modified the milestones: 5.0-beta2, 5.0-beta3 Oct 31, 2019
@maxGimeno
Copy link
Contributor

@lrineau lrineau added rm only: ready for master For the release team only: that indicates that a PR is about to be merged in 'master' and removed Ready to be tested labels Nov 6, 2019
lrineau added a commit that referenced this pull request Nov 6, 2019
PMP: Replace `Location_traits` by a simpler API
@lrineau lrineau merged commit 09683a8 into CGAL:master Nov 6, 2019
@lrineau lrineau deleted the PMP-Locate_rework_traits-GF branch November 6, 2019 17:04
@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 Nov 6, 2019
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.

5 participants