Skip to content

Provide graph traits and properties for pmp::SurfaceMesh #4460

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

Draft
wants to merge 22 commits into
base: master
Choose a base branch
from

Conversation

afabri
Copy link
Member

@afabri afabri commented Jan 10, 2020

Summary of Changes

This PR will make pmp::SurfaceMesh a model of a MutableFaceGraph. It further needs some additional typedefs and postfix increment of pmp iterators which I discuss with the authors.

See http://www.pmp-library.org/ and PR29 and PR30

Release Management

@lrineau lrineau added Not yet approved The feature or pull-request has not yet been approved. TODO labels Jan 10, 2020
@lrineau
Copy link
Member

lrineau commented Jan 10, 2020

TODO: add CMake support for pmp (http://www.pmp-library.org/).

@sloriot
Copy link
Member

sloriot commented Jan 14, 2020

Note that the build system is cmake so it works directly in config mode. It might just be a matter of documentation on our side. Side note, Eigen seems to be required so it should come with a find Eigen.

@MaelRL MaelRL changed the base branch from master to releases/CGAL-4.14-branch March 26, 2020 20:05
@MaelRL MaelRL changed the base branch from releases/CGAL-4.14-branch to master March 26, 2020 20:06
@MaelRL
Copy link
Member

MaelRL commented Sep 22, 2020

@afabri what is the status of this PR?

@maxGimeno maxGimeno added this to the Trash / Attic milestone Oct 20, 2020
@sloriot sloriot changed the base branch from master to 5.2.x-branch August 13, 2021 09:42
@sloriot sloriot changed the base branch from 5.2.x-branch to master August 13, 2021 09:42
@sloriot
Copy link
Member

sloriot commented Aug 13, 2021

There are still issues in pmp-library in the generatedpmpTargets.cmake:
The following is generated:

set_target_properties(pmp PROPERTIES
  INTERFACE_COMPILE_OPTIONS "-std=c++11"
  INTERFACE_INCLUDE_DIRECTORIES "/home/sloriot/projects/pmp-library.git/src/pmp/../"
  INTERFACE_LINK_LIBRARIES "OpenMP::OpenMP_CXX"
)

while something like that should be generated:

find_package(OpenMP)
if(OpenMP_CXX_FOUND)
set_target_properties(pmp PROPERTIES
  INTERFACE_LINK_LIBRARIES "OpenMP::OpenMP_CXX"
)
endif()

set_target_properties(pmp PROPERTIES
  CXX_STANDARD 11
  INTERFACE_INCLUDE_DIRECTORIES "/home/sloriot/projects/pmp-library.git/src/pmp/../"
)

I don't know how to do it property in the cmake machinery without using a template file.

@mbotsch @dsieger do you see a solution? Note that it is not specific to CGAL but to any project using cmake that do not use OpenMP and/or that use a CXX standard greater than c++11

@dsieger
Copy link

dsieger commented Aug 17, 2021

Hi Sebastien,

I'm sorry, but to me it's also not obvious what the solution would be.

Regarding the C++ standard I would even say that prescribing the exact version is the right thing to do. Simply using a later standard might lead to incompatibilities / deprecation issues.

Regarding OpenMP I would assume that it is not added to the interface libraries if OpenMP is not found. After all, we only add it conditionally:

find_package(OpenMP)
if(OpenMP_CXX_FOUND)
  target_link_libraries(pmp PUBLIC OpenMP::OpenMP_CXX)
endif()

But then, I'm not too familiar with that part of CMake...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Not yet approved The feature or pull-request has not yet been approved. Pkg::BGL TODO
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants