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

Bugfix in TestArithmeticalDSSOnSurfels #1742

Merged
merged 5 commits into from
Sep 13, 2024

Conversation

troussil
Copy link
Member

@troussil troussil commented Sep 11, 2024

PR Description

The main issue was an iterator dereferencing in the init method of the class ArithmeticalDSSOnSurfels. This was done because of a bad algorithmic choice: in short, the new point to process was computed by looking at the next surfel, instead of looking at the orientation of the current surfel. Several parts of the class has been rewritten to use the latter strategy, which nicely fixes the bug.

In summary, in this new version:

  • no *myEnd in the unit method,
  • for each surfel, an associated point is extracted based on its orientation.

In addition, the template non-type adjacency has been removed, since one always wants to use "standard" digital straight segment and "4"-connectivity in the use-case targeted by the class.

Checklist

(the three first entries are not relevant since it is a bug fix, but I will take care of the other ones)

  • 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)

@troussil troussil linked an issue Sep 11, 2024 that may be closed by this pull request
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.

Looks fine just some minor edits, and have a look to the template change on the example compilation (exampleMaximalSegmentSliceEstimation.cpp) (and see log https://github.com/DGtal-team/DGtal/actions/runs/10813115621/job/29996362923?pr=1742)

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.

👌🏻

@kerautret kerautret self-requested a review September 11, 2024 19:47
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.

Just a renaming aLine doxygen

makes parameter list and doxygen documentation match

Co-authored-by: Bertrand Kerautret <bertrand.kerautret@univ-lyon2.fr>
@kerautret
Copy link
Member

Fine for me thanks @troussil could merge when last CI build is done

@dcoeurjo
Copy link
Member

👌

@dcoeurjo dcoeurjo merged commit f74ed72 into DGtal-team:master Sep 13, 2024
10 checks passed
@dcoeurjo dcoeurjo deleted the testArithmeticalDSSOnSurfelsFix branch September 13, 2024 15:25
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.

Test fails testArithmeticalDSSComputerOnSurfels
3 participants