Skip to content

BUG: fix an infinite loop in itk::VoronoiDiagram2DGenerator #5400

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
Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

BUG: fix an infinite loop in itk::VoronoiDiagram2DGenerator #5400

wants to merge 2 commits into from

Conversation

ILoveGoulash
Copy link

@ILoveGoulash ILoveGoulash commented Jun 16, 2025 •
edited
Loading

The loop exit condition was present in the code, but never wired and removed via cd97879

PR Checklist

  • No API changes were made (or the changes have been approved)
  • No major design changes were made (or the changes have been approved)
  • Added test (or behavior not changed)
  • Updated API documentation (or API not changed)
  • Added license to new files (if any)
  • Added Python wrapping to new files (if any) as described in ITK Software Guide Section 9.5
  • Added ITK examples for all new major features (if any)

Refer to the ITK Software Guide for
further development details if necessary.

@github-actions github-actions bot added type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances area:Segmentation Issues affecting the Segmentation module labels Jun 16, 2025
github-actions[bot]
github-actions bot reviewed Jun 16, 2025
View reviewed changes
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Thank you for contributing a pull request! 🙏

Welcome to the ITK community! 🤗👋☀️

We are glad you are here and appreciate your contribution. Please keep in mind our community participation guidelines. 📜
More support and guidance on the contribution process can be found in our contributing guide. 📖

This is an automatic message. Allow for time for the ITK community to be able to read the pull request and comment
on it.

@dzenanz dzenanz requested review from seanm and hjmjohnson June 16, 2025 13:45
@ILoveGoulash
Copy link
Author

Don't know if you want a regression test, and if your framework can handle "infinite looping" type failures.

@dzenanz
Copy link
Member

dzenanz commented Jun 16, 2025

A regression test is advantageous. I don't think we have an explicit support for infinite looping, but such a test would fail via timeout.

@github-actions github-actions bot added type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct labels Jun 16, 2025
@ILoveGoulash
Copy link
Author

Added the original case by doing "monkey see monkey do", but it looks like I can't build locally due to IPFS + work firewall or something.

@ILoveGoulash ILoveGoulash force-pushed the fix-voronoi-diagram-2d-generator-inf-loop branch 2 times, most recently from f5ff22b to 62457de Compare June 16, 2025 14:39
dzenanz
dzenanz reviewed Jun 16, 2025
View reviewed changes
Copy link
Member

@dzenanz dzenanz left a comment

Choose a reason for hiding this comment

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

OK, I can reproduce locally an infinite loop with this test.

@dzenanz
Copy link
Member

dzenanz commented Jun 16, 2025

@bradking Is ghostflow acting up? Commit 62457de has a very short commit message.

@ILoveGoulash
Copy link
Author

Nah, the full "http://..." link is probably what triggers it, GitHub prettifies it on display.

@ILoveGoulash ILoveGoulash force-pushed the fix-voronoi-diagram-2d-generator-inf-loop branch from 62457de to 288b65e Compare June 16, 2025 14:51
hjmjohnson
hjmjohnson reviewed Jun 17, 2025
View reviewed changes
Modules/Segmentation/Voronoi/include/itkVoronoiDiagram2DGenerator.hxx Outdated
EdgeInfo front = curr;
EdgeInfo back = curr;
while (!(rawEdges[i].empty()))
auto maxStop = static_cast<int>(rawEdges[i].size());
Copy link
Member

Choose a reason for hiding this comment

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

Why is there a static_cast to a signed int here? What if there are more edges than will fit in the positive range of an integer?

I think maxStop could just be the same type as size() ( likely size_t ).

Copy link
Author

@ILoveGoulash ILoveGoulash Jun 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The > 0 condition can't work on an unsigned. It'd make sense to cast it to a bigger type or use size_t and check for != SIZE_MAX, though. Which one do you prefer?

Copy link
Contributor

Choose a reason for hiding this comment

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

@ILoveGoulash Thank you for your pull request. 👍 Specifically regarding your comment here, I think you're mistaken! When u is an unsigned number, the condition u > 0 just works fine! (It is equivalent to the condition u != 0.)

HTH, Niels

Copy link
Author

Choose a reason for hiding this comment

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

You're right. I'll try to find the time tomorrow to fix that.

@hjmjohnson
Copy link
Member

@ILoveGoulash Thank you for your contribution. There are just a few final actions that need to be finalized so that we can incorporate this into the main tree.

@ILoveGoulash ILoveGoulash force-pushed the fix-voronoi-diagram-2d-generator-inf-loop branch from 288b65e to 380c33d Compare August 14, 2025 07:24
@ILoveGoulash
Copy link
Author

Should be okay now.

@hjmjohnson hjmjohnson force-pushed the fix-voronoi-diagram-2d-generator-inf-loop branch from 380c33d to 8f868e4 Compare August 14, 2025 12:52
@ILoveGoulash @hjmjohnson
BUG: fix an infinite loop in itk::VoronoiDiagram2DGenerator
b3cf50c 
The loop exit condition was present in the code, but never wired and removed via InsightSoftwareConsortium@cd97879

Fixes InsightSoftwareConsortium#4386
@hjmjohnson hjmjohnson force-pushed the fix-voronoi-diagram-2d-generator-inf-loop branch from 8f868e4 to c6ccef6 Compare August 14, 2025 12:57
@hjmjohnson
Copy link
Member

@ILoveGoulash I rebased and removed the merge conflict, then updated the commit message for the regression test to be more descriptive.

@ILoveGoulash
Copy link
Author

Thanks!

@hjmjohnson hjmjohnson force-pushed the fix-voronoi-diagram-2d-generator-inf-loop branch from c6ccef6 to cec88e2 Compare August 14, 2025 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hjmjohnson hjmjohnson hjmjohnson left review comments

@N-Dekker N-Dekker N-Dekker left review comments

@github-actions github-actions[bot] github-actions[bot] left review comments

@dzenanz dzenanz dzenanz approved these changes

@seanm seanm Awaiting requested review from seanm

Assignees
No one assigned
Labels
area:Segmentation Issues affecting the Segmentation module type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ILoveGoulash @dzenanz @hjmjohnson @N-Dekker