Skip to content

Cleaning two headers.#4022

Closed
switzel wants to merge 1 commit intoCGAL:masterfrom
switzel:master
Closed

Cleaning two headers.#4022
switzel wants to merge 1 commit intoCGAL:masterfrom
switzel:master

Conversation

@switzel
Copy link
Copy Markdown

@switzel switzel commented Jun 20, 2019

Summary of Changes

Debug code removed from HyperbolicVoronoiGraphicsItem.h.
Systematically failing assertion commented out in
Hyperbolic_Delaunay_triangulation_traits_2.h.

Release Management

  • Affected package(s): Hyperbolic_Delaunay_triangulation
  • Issue(s) solved (if any):
  • Feature/Small Feature (if any): edge pen in Hyperbolic_...-demo can be changed
  • Link to compiled documentation
  • License and copyright ownership:

Debug code removed from HyperbolicVoronoiGraphicsItem.h.
Systematically failing assertion commented out in
Hyperbolic_Delaunay_triangulation_traits_2.h.
@lrineau
Copy link
Copy Markdown
Member

lrineau commented Sep 4, 2019

@MoniqueTeillaud Can you or one of your students have a look at that small contribution? It is announced as a bug-fix, but I am not sure if it is correct.

@lrineau
Copy link
Copy Markdown
Member

lrineau commented Oct 2, 2019

@lrineau commented on Sep 4, 2019, 11:27 AM GMT+2:

@MoniqueTeillaud Can you or one of your students have a look at that small contribution? It is announced as a bug-fix, but I am not sure if it is correct.

@MoniqueTeillaud, that contribution is now more that three months old.

@MoniqueTeillaud
Copy link
Copy Markdown
Member

I currently have 0 student and I was assigned only one month ago. We could have discussed this during the CGAL meeting two weeks ago, it would have been more efficient.
@imiordanov could you please have a look at the diff?
There are two changes:

  • One is in the actual demo code, which I don't know, so, I cannot have an opinion on it.
  • The other is more important as it changes a precondition in the traits, it requires more care. Isn't this function called by some tests in the test-suite?

@imiordanov
Copy link
Copy Markdown
Member

Hello @lrineau, @MoniqueTeillaud ,
Sorry for the lack of response so far, somehow this issue escaped my attention.

I will have a look as soon as possible! I will get back to you once I have had the chance to review the modifications.

Comment on lines -102 to -118
// delete
QPen temp = painter->pen();
QPen old = temp;
temp.setWidthF(0.01);
painter->setPen(temp);
//

for(typename DT::All_edges_iterator eit = dt->all_edges_begin();
eit != dt->all_edges_end();
eit++)
{
typename DT::Hyperbolic_segment s = dt->dual(*eit);
pos << s;
}

// delete
painter->setPen(old);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should be OK, it's a part of the demo and it has no critical impact on the actual code.

Copy link
Copy Markdown
Member

@imiordanov imiordanov left a comment

Choose a reason for hiding this comment

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

Cleaning up the debug code from the Qt interface is OK, no issues there.
However, I would like to investigate the failing assertion a bit more, before just commenting it out of the code. I'll try to find the time to debug as soon as possible.

Comment on lines -379 to -380
CGAL_triangulation_precondition((_gt.side_of_oriented_circle_2_object()(p, q, r,s) == ON_NEGATIVE_SIDE) &&
(_gt.side_of_oriented_circle_2_object()(p, s, q, r) == ON_NEGATIVE_SIDE));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This precondition asserts that the faces (p,q,r) does not contain s, and the face (p,s,q) does not contain r, which is satisfied if the two faces (p,q,r) and (p,s,q) are Delaunay. If the assertion is failing, I feel like more investigation is needed, instead of just commenting out the check altogether.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Indeed, just commenting out is dangerous.
I am a bit surprised that, if the assertion is problematic, this has not been detected by the test-suite...

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@switzel Can you comment on why you removed that precondition?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ping @switzel!

@MaelRL MaelRL added the Not yet approved The feature or pull-request has not yet been approved. label Oct 9, 2019
@MaelRL MaelRL added this to the Trash / Attic milestone Apr 7, 2020
@sloriot sloriot closed this Aug 26, 2021
@sloriot
Copy link
Copy Markdown
Member

sloriot commented Aug 26, 2021

Closed due to inactivity

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::Hyperbolic_triangulation_2 Waiting for answer

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants