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

Improve the manuals for the 3D Polyhedral Surface and Triangulated Surface Mesh Segmentation pkgs #7540

Merged
merged 3 commits into from
Jul 12, 2023

Conversation

nmnobre
Copy link
Contributor

@nmnobre nmnobre commented Jun 23, 2023

Hi @sloriot,

Unfortunately, I still don't think the changes in #7523 correctly describe the mesh segmentation algorithm.
I've checked the code, and it looks sound to me, it's doing what intuitively I'd expect it to. :)
Now the description matches that code and intuition.

I hope I'm not coming across as too annoying...
Cheers,
-Nuno

@sloriot sloriot added Enhancement Not yet approved The feature or pull-request has not yet been approved. Pkg::Surface_mesh_segmentation Under Testing Batch_1 First Batch of PRs under testing and removed Under Testing labels Jun 26, 2023
@sloriot
Copy link
Member

sloriot commented Jul 5, 2023

Successfully tested in CGAL-6.0-Ic-15

@sloriot sloriot added Tested and removed Under Testing Batch_1 First Batch of PRs under testing labels Jul 5, 2023
@lrineau
Copy link
Member

lrineau commented Jul 5, 2023

Successfully tested in CGAL-6.0-Ic-15

@sloriot You marked the pull-request with https://github.com/CGAL/cgal/labels/not%20yet%20approved. Do you think it is ready to be merged, now?

- \f$\lambda \in [0,1]\f$ denotes a smoothness parameter.
</td>
</tr>
</table>

Note both terms of the energy function, \f$ e_1 \f$ and \f$ e_2 \f$, are always non-negative.
The first term of the energy function provides the contribution of the soft clustering probabilities.
The second term of the energy function is a geometric criterion that is larger when two adjacent facets sharing a sharp and concave edge are not in the same cluster.
The second term of the energy function is a geometric criterion that is larger when two adjacent facets sharing a straight dihedral angle are not in the same cluster.
Copy link
Member

Choose a reason for hiding this comment

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

@lrineau changes are OK and for "straight dihedral angle" I don't know what it means.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @sloriot,

I meant to write the complete opposite of what was written.
What if we write: "[...] sharing a flat edge, i.e. a straight dihedral angle, are not [...]"?

This comment was marked as outdated.

Copy link
Member

Choose a reason for hiding this comment

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

I guess we can use almost flat dihedral angle instead of straight.

@github-actions
Copy link

github-actions bot commented Jul 5, 2023

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

- \f$\lambda \in [0,1]\f$ denotes a smoothness parameter.
</td>
</tr>
</table>

Note both terms of the energy function, \f$ e_1 \f$ and \f$ e_2 \f$, are always non-negative.
The first term of the energy function provides the contribution of the soft clustering probabilities.
The second term of the energy function is a geometric criterion that is larger when two adjacent facets sharing a sharp and concave edge are not in the same cluster.
The second term of the energy function is a geometric criterion that is larger the larger the dihedral angle between two adjacent facets not in the same cluster.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've rephrased it a bit, how does this look?

Copy link
Member

Choose a reason for hiding this comment

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

Except if I'm wrong you have a greater contribution the closer you are to 180 degrees. I guess that's what you meant but largest angle could be 360 degree.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're quite right. I've reworded it again and included the valid range for theta in the description of the expression symbols.

@sloriot sloriot added Under Testing Batch_1 First Batch of PRs under testing Tested and removed Under Testing Not yet approved The feature or pull-request has not yet been approved. Batch_1 First Batch of PRs under testing labels Jul 5, 2023
@sloriot
Copy link
Member

sloriot commented Jul 12, 2023

Successfully tested in CGAL-6.0-Ic-20

@lrineau lrineau added this to the 5.6-beta2 milestone Jul 12, 2023
@lrineau lrineau added the rm only: ready for master For the release team only: that indicates that a PR is about to be merged in 'master' label Jul 12, 2023
@lrineau lrineau self-assigned this Jul 12, 2023
@lrineau lrineau added the 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 label Jul 12, 2023
@lrineau lrineau merged commit 5ef509c into CGAL:5.6.x-branch Jul 12, 2023
lrineau added a commit that referenced this pull request Jul 12, 2023
Improve the manuals for the 3D Polyhedral Surface and Triangulated Surface Mesh Segmentation pkgs
@lrineau lrineau removed rm only: ready for master For the release team only: that indicates that a PR is about to be merged in 'master' 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 Jul 12, 2023
@lrineau lrineau modified the milestones: 5.6-beta2, 5.6 Jul 20, 2023
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.

3 participants