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

Winding number from libigl in DGtal #1697

Merged
merged 59 commits into from
Mar 9, 2024
Merged

Winding number from libigl in DGtal #1697

merged 59 commits into from
Mar 9, 2024

Conversation

dcoeurjo
Copy link
Member

@dcoeurjo dcoeurjo commented May 29, 2023 •
edited
Loading

PR Description

Adding winding number as a model of Euclidean oriented shape.

Checklist

  • Unit-test of your feature with Catch.
  • Doxygen documentation of the code completed (classes, methods, types, members...)
  • Documentation module page added or updated.
  • New entry in the ChangeLog.md added.
  • No warning raised in Debug mode.
  • All continuous integration tests pass (Github Actions & appveyor)

@dcoeurjo dcoeurjo marked this pull request as ready for review November 13, 2023 07:49
@dcoeurjo
Copy link
Member Author

/builddoc

@github-actions GitHub Actions
Copy link

There was an error while building the doc. Check the GitHub actions for debugging.

@dcoeurjo
Copy link
Member Author

/builddoc

@github-actions GitHub Actions
Copy link

The documentation is built. It will be available, after a few minutes, here: https://dgtal-team.github.io/doc-pr/pr1697/index.html

@kerautret
Copy link
Member

This PR can be reviewed

Nice I look it in the coming days-!

@JacquesOlivierLachaud
Copy link
Member

Looks great ! Quick questions. Since many files have been changed, it is hard to see where to focus the PR. For instance, you are editing a module Winding doc, but I do not see in the generated doc any link to this doc module (in shapes package), and I do not see a new example in the list of examples. Thanks for your help !

@dcoeurjo
Copy link
Member Author

The Winding module doc is included in "shapes"
The example is in the polyscope-examples folder

@dcoeurjo
Copy link
Member Author

dcoeurjo commented Mar 3, 2024

Can someone review this PR?
I had to edit some doc pages in this PR but the core contribution is in the WindingNumber classes.

@kerautret
Copy link
Member

@dcoeurjo sorry I miss it, for tomorrow is it fine ? I think I can have some time tomorrow.

@dcoeurjo
Copy link
Member Author

dcoeurjo commented Mar 3, 2024

👍 thx

kerautret
kerautret reviewed Mar 5, 2024
View reviewed changes
Copy link
Member

@kerautret kerautret left a comment

Choose a reason for hiding this comment

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

Nice new features, tested and working directly without any comp issue 👌🏻 (even with CGal add). Just some minor suggestions from generating the doc locally.

examples/polyscope-examples/windingNumbersShape.cpp Outdated Show resolved Hide resolved
examples/polyscope-examples/windingNumbersShape.cpp Outdated Show resolved Hide resolved
examples/polyscope-examples/windingNumbersShape.cpp Show resolved Hide resolved
examples/polyscope-examples/windingNumbersShape.cpp Outdated Show resolved Hide resolved
src/DGtal/shapes/WindingNumbersShape.h Show resolved Hide resolved
src/DGtal/shapes/doc/moduleWinding.dox
DGtal::Orientation orientation = winding.orientation(q); //returns DGtal::INSIDE, DGtal::ON or DGtal::OUTSIDE
@endcode

As the WindingNumberShape class is a model of concepts::CEuclideanBoundedShape, the GaussDigitizer could be used (see @ref moduleShape).
Copy link
Member

Choose a reason for hiding this comment

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

Same here, WindingNumberShape is not linked to the doc page

Copy link
Member Author

Choose a reason for hiding this comment

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

It looks like the page is available (Shape→ ..) https://dgtal-team.github.io/doc-pr/pr1697/index.html
can you confirm ?

tests/shapes/testWindingNumbersShape.cpp Outdated Show resolved Hide resolved
tests/shapes/testWindingNumbersShape.cpp Outdated Show resolved Hide resolved
tests/shapes/testWindingNumbersShape.cpp Outdated Show resolved Hide resolved
tests/shapes/testlibigl.cpp Outdated Show resolved Hide resolved
@dcoeurjo
Copy link
Member Author

dcoeurjo commented Mar 8, 2024

/builddoc

@github-actions GitHub Actions
Copy link

github-actions bot commented Mar 8, 2024

The documentation is built. It will be available, after a few minutes, here: https://dgtal-team.github.io/doc-pr/pr1697/index.html

@dcoeurjo
Copy link
Member Author

dcoeurjo commented Mar 9, 2024

Merging this one, I will check the master for the python issues

@dcoeurjo
Copy link
Member Author

dcoeurjo commented Mar 9, 2024

thanks @kerautret for the review

@dcoeurjo dcoeurjo merged commit f49e724 into master Mar 9, 2024
21 checks passed
@dcoeurjo dcoeurjo deleted the Winding branch March 9, 2024 09:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kerautret kerautret kerautret left review comments

@JacquesOlivierLachaud JacquesOlivierLachaud Awaiting requested review from JacquesOlivierLachaud

Assignees
No one assigned
Labels
waiting author feedbacks
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dcoeurjo @kerautret @JacquesOlivierLachaud