Skip to content

follow up PR #9274 "Mesh_3 - speedup protecting balls placement"#9366

Draft
lrineau wants to merge 21 commits intoCGAL:mainfrom
lrineau:CGAL-follow_up_PR_9274__Mesh_3-speedup_balls_wip-jtournois__-lrineau
Draft

follow up PR #9274 "Mesh_3 - speedup protecting balls placement"#9366
lrineau wants to merge 21 commits intoCGAL:mainfrom
lrineau:CGAL-follow_up_PR_9274__Mesh_3-speedup_balls_wip-jtournois__-lrineau

Conversation

@lrineau
Copy link
Copy Markdown
Member

@lrineau lrineau commented Mar 10, 2026

Summary of Changes

a few leftovers after PR #9274

TODO

See issue #9367.

Release Management

@sloriot
Copy link
Copy Markdown
Member

sloriot commented Mar 10, 2026

Why does this PR modify Surface_mesh?

@lrineau
Copy link
Copy Markdown
Member Author

lrineau commented Mar 10, 2026

Why does this PR modify Surface_mesh?

Because I sneaked aafe3b5 (virtualoverride) in this PR.

@sloriot
Copy link
Copy Markdown
Member

sloriot commented Mar 11, 2026

Why does this PR modify Surface_mesh?

Because I sneaked aafe3b5 (virtualoverride) in this PR.

Let me rephrase: Why are you doing this modification in this PR? A dedicated PR with some explanations would be better. Properties are not even used AFAIK in Mesh_3.

lrineau added 9 commits March 13, 2026 10:07
`CGAL::sqrt` for native types (`float`, `double`, `int`...)  does the same but through the
abstraction of `Algebraic_foundations`.
... and make it explicit that `weight_modifier` is the square of another constant.
- fix headers,
- comments,
- factorize the display of a range of ids into a function
I do not understand why the code is trying to recompute that value equal to 1.
Let's remove that complexity for now. I hope it was not a way to take floating point
inexactness into account.
move that block of code to where it is actually used
- rename `prims` to `curve_primitives`
- add a scope block, that is the "creation"  of `curve_primitives` (should be an [Immediately invoked function expression]
- rearrange block of comments, to distinguish the parts of the code that should be in different sub-functions

[Immediately invoked function expression]: https://en.wikipedia.org/wiki/Immediately_invoked_function_expression
Co-authored-by: Sebastien Loriot <sloriot.ml@gmail.com>
@sloriot
Copy link
Copy Markdown
Member

sloriot commented Mar 30, 2026

Successfully tested in CGAL-6.2-Ic-141

Copy link
Copy Markdown
Member Author

@lrineau lrineau left a comment

Choose a reason for hiding this comment

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

@sloriot

Here are the three points that currently block the integration of this draft pull-request. Those three points need to be discussed.

Once those three points are addressed, the pull-request could be merged. As least it fixes the issue #9369, and it will fix issue #9371 if we agree to document the is_parallel static data member of Sequential_tag and Parallel_tag.

@github-actions github-actions bot removed the Tested label Mar 30, 2026
@github-actions
Copy link
Copy Markdown

This pull-request was previously marked with the label Tested, but has been modified with new commits. That label has been removed.

@sloriot
Copy link
Copy Markdown
Member

sloriot commented Apr 1, 2026

Successfully tested in CGAL-6.2-Ic-144

@sloriot
Copy link
Copy Markdown
Member

sloriot commented Apr 1, 2026

@sloriot

Here are the three points that currently block the integration of this draft pull-request. Those three points need to be discussed.

Once those three points are addressed, the pull-request could be merged. As least it fixes the issue #9369, and it will fix issue #9371 if we agree to document the is_parallel static data member of Sequential_tag and Parallel_tag.

All fixed, right?

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.

follow up PR #9274 "Mesh_3 - speedup protecting balls placement"

3 participants