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.

Sign up for GitHub

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

Jump to bottom

Add PMP::longest_edge() #6496

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

Add PMP::longest_edge() #6496

wants to merge 15 commits into from

Conversation

hoskillua
Copy link
Member

Summary of Changes

I have implemented the longest_edge() function requested in this issue. I have tested it on simple input and it works fine.

Release Management

@hoskillua hoskillua changed the title adding longest_edge() PMP: adding longest_edge() Apr 19, 2022
@lrineau lrineau linked an issue Apr 19, 2022 that may be closed by this pull request
PMP Feature request: Add longest_edge() #5799
Open
@lrineau lrineau added the CLA/CAA The contributor must sign a CLA or a CAA so that the contribution can be merged into CGAL label Apr 19, 2022
@MaelRL MaelRL added Not yet approved The feature or pull-request has not yet been approved. and removed Feature request labels Apr 20, 2022
@hoskillua hoskillua requested a review from sloriot April 20, 2022 12:47
@MaelRL MaelRL changed the title PMP: adding longest_edge() Add PMP::longest_edge() Apr 20, 2022
@hoskillua hoskillua requested a review from afabri April 20, 2022 14:14
@afabri
Copy link
Member

afabri commented Apr 20, 2022 •
edited
Loading

What is missing:
A line in the Reference Manual index page
A line in the change log
A test program.

@hoskillua
Copy link
Member Author

@afabri, I updated the Reference Manual and change log files. about testing, I added calls to the function in measure_test.cpp and it ran as expected on these tests. But I can also make a testing program for this function if required.

@hoskillua
Copy link
Member Author

Would it be beneficial to add functions for shortest edge, largest and smallest faces (area) for completion? If so, I can implement them and add tests in general for the 4 functions.

@sloriot sloriot added this to the 5.6-beta milestone May 16, 2022
@sloriot
Copy link
Member

sloriot commented May 19, 2022

There is a test failing here. Basically it says that the edge iterator of Polyhedron is not detected as a ForwardIterator. It might be a bug on our side with the definition of the edge iterator type.

@MaelRL MaelRL modified the milestones: 5.6-beta, 5.7-beta Mar 23, 2023
@janetournois
Copy link
Member

Hello, what is the status of this PR?
I can't find the associated small feature.

@sloriot
Copy link
Member

sloriot commented Oct 24, 2023

@janetournois from what I remember there is a technical issue that I wasn't able to easily workaround at the time I looked at it. Let me have a fresh look.

@janetournois janetournois modified the milestones: 6.0-beta, 6.1-beta May 16, 2024
@MaelRL
Re-implement without std::max_element
a5b79b9 
std::max_element demands a forward iterator, which we do not have
for Polyhedron_3: its iterator type is a boost transform_iterator
with the edge_descriptor built on-the-fly when dereferenced.

The error in practice is a static assertion for compilers
that do in fact enforce the requirement of the iterator category
like:

error: static assertion failed due to requirement
'std::is_same<
   boost::iterators::detail::iterator_category_with_traversal<
     std::input_iterator_tag,
     boost::iterators::bidirectional_traversal_tag>,
   std::forward_iterator_tag>::value'

We could probably have an iterator with state or something, but
it is not worth it for now...
@MaelRL
Minor text fix
7313b98
@sloriot
Copy link
Member

sloriot commented Dec 23, 2024

Successfully tested in CGAL-6.1-Ic-45

@sloriot sloriot added Tested and removed Not yet approved The feature or pull-request has not yet been approved. CLA/CAA The contributor must sign a CLA or a CAA so that the contribution can be merged into CGAL Ready to be tested Under Testing labels Dec 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@afabri afabri afabri left review comments

@sloriot sloriot Awaiting requested review from sloriot

Assignees
No one assigned
Labels
CHANGES.md not updated Pkg::PMP Small feature Tested
Projects
None yet
Milestone
6.1-beta
Development

Successfully merging this pull request may close these issues.

PMP Feature request: Add longest_edge()
6 participants
@hoskillua @afabri @sloriot @janetournois @MaelRL @lrineau