Skip to content

Fix issue with convex hull - #2151

Open
Casper Lamboo (casperlamboo) wants to merge 5 commits into
mainfrom
convex-hull-issue
Open

Fix issue with convex hull#2151
Casper Lamboo (casperlamboo) wants to merge 5 commits into
mainfrom
convex-hull-issue

Conversation

@casperlamboo

@casperlamboo Casper Lamboo (casperlamboo) commented Oct 19, 2024

Copy link
Copy Markdown
Contributor

Description

For some cases the convex hull produces the incorrect (non-convex) results, see attached screenshot.

Screenshot 2024-10-19 at 08 59 52

In this PR a test case with the exact polygon of the example above is added along with a fix for that polygon.

Bug originally found by Thomas Rahm (@ThomasRahm)

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Added a unit test showcasing the issue, with the fix the issue is resolved.

Test Configuration:

  • Operating System:

MacOS 13.3

Checklist:

  • My code follows the style guidelines of this project as described in UltiMaker Meta
  • I have read the Contribution guide
  • I have commented my code, particularly in hard-to-understand areas
  • I have uploaded any files required to test this change

CURA-13293

@casperlamboo
Casper Lamboo (casperlamboo) marked this pull request as ready for review August 14, 2026 13:53
@github-actions

Copy link
Copy Markdown
Contributor

Test Results

31 tests   31 ✅  5s ⏱️
 1 suites   0 💤
 1 files     0 ❌

Results for commit 8f7b84b.

Comment thread src/geometry/Shape.cpp
const Point2LL& current = window[0];
const Point2LL& after = window[1];
const auto& current = window[0];
const auto& after = window[1];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is there a need for this change ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Because we also don't write type names for for (const auto& x : xs). This is just the extension of a for loop iterator variable imo.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Well, I would say it makes it even more confusing because of the window not being typed. Now if you want to know what types are current and after, you have to go up to the definition of poly. Not super annoying in this case because it is just above, but I really don't see the value in changing a readable typed object to auto. But I know we do have different personal opinions about this topic 😃

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

official CPP guidelines talk about AAA (almost always use auto), see https://cginternals.github.io/guidelines/articles/almost-always-auto/

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I know, but I still don't agree with this paradigm. There are benefits, but according to me they are lesser than the issues it brings, especially on read-expliciteness and the risk of error that it adds of using a wrong type.
I don't really understand that popular languages like Python and JS got a typed-ish version, but on the other side, C++ is moving towards loosely-typed. In some specific cases, auto really saves your day, but the AAA principle looks really counter-intuitive to me.

Comment thread src/geometry/Shape.cpp
while (convexified.size() >= 2 && (LinearAlg2D::pointIsLeftOfLine(convexified.back(), convexified[convexified.size() - 2], poly.back()) >= 0))
{
convexified.pop_back();
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This seems to fix the symptom rather than the root cause of the issue, or is it the actual proper way of fixing this ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It has been some time ago that I wrote this code. But from what I recall this was indeed a proper fix since we did not properly account for the end points in the top and bottom convex hull's.

@casperlamboo Casper Lamboo (casperlamboo) Aug 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, just re-read the code. This is indeed the proper fix. We're doing the following things.

  1. sort points based on it's x-coord (from low to high. If x-coord is the same then order is defined on y-coord
  2. maintain a invariant of the convex hull, $P^{\text{convex}}$, then for each point $p$ execute the following sub routine
    a. check if adding the current point invalidates any previous point $P^{\text{convex}}_{|P| - 1}$. as long as the $P^{\text{convex}}_{|P| - 1}$ is no longer convex given point $p$ and $P^{\text{convex}}_{|P| - 2}$ remove $P^{\text{convex}}_{|P| - 1}$. Do this until the $P^{\text{convex}}_{|P| - 1}$ is located on the convex hull invariant.
    b. add the point $p$ to $P^{\text{convex}}$
  3. Reverse the points and perform the same procedure defined in 2

The issue here is that we're always adding the current point in step 2.b. However, it could be that the last point(s) are not on the convex hull (due to the stable sort sorting first on x then on y, so either for the forward pass or backward pass it could be that the last inserted point is not on the convex hull). This code is to fix this oversight where keep removing the last point(s) untill all points are convex.

Comment thread src/geometry/Shape.cpp
const Point2LL& current = window[0];
const Point2LL& after = window[1];
const auto& current = window[0];
const auto& after = window[1];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Well, I would say it makes it even more confusing because of the window not being typed. Now if you want to know what types are current and after, you have to go up to the definition of poly. Not super annoying in this case because it is just above, but I really don't see the value in changing a readable typed object to auto. But I know we do have different personal opinions about this topic 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants