Skip to content

Fix issue with convex hull #2151

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

Open
Casper Lamboo (casperlamboo) wants to merge 5 commits into
base: main
Choose a base branch
from
+45 −2
Open

Fix issue with convex hull #2151

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d2033c9
Add unit test showcasing bug in convex hull
casperlamboo Oct 19, 2024
2f67d22
Fix issue with convex hull
casperlamboo Oct 19, 2024
0591a46
Applied clang-format.
casperlamboo Oct 19, 2024
26ef1a1
Add missing settings
casperlamboo Oct 19, 2024
8f7b84b
Merge branch 'main' into convex-hull-issue
casperlamboo Aug 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions src/geometry/Shape.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,8 +88,8 @@ void Shape::makeConvex()

for (const auto window : poly | ranges::views::sliding(2))
{
const Point2LL& current = window[0];
const Point2LL& after = window[1];
const auto& current = window[0];
const auto& after = window[1];

@wawanbreton Erwan MATHIEU (wawanbreton) Aug 17, 2026

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 ?

@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.

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.

@wawanbreton Erwan MATHIEU (wawanbreton) Aug 17, 2026

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 😃

@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.

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

@wawanbreton Erwan MATHIEU (wawanbreton) Aug 17, 2026

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.

@casperlamboo Casper Lamboo (casperlamboo) Aug 17, 2026 •
edited
Loading

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.

I think most (all) languages are converging to the same paradigm. That is use strong types for function annotations, use types when it's really needed, like rust

let x: i32 = "5".parse().unwrap();

Here you have to use i32 otherwise the compiler doens't know to what type it should parse.

but simple variable declarations type annotations do more harm then good. They introduce large git diffs when types changes, introduce visual clutter and longer lines. And especially for cpp there is a risk of un intentional type conversions.


if (LinearAlg2D::pointIsLeftOfLine(current, convexified.back(), after) < 0)
{
Expand All @@ -103,6 +103,11 @@ void Shape::makeConvex()
convexified.push_back(current);
}
}

while (convexified.size() >= 2 && (LinearAlg2D::pointIsLeftOfLine(convexified.back(), convexified[convexified.size() - 2], poly.back()) >= 0))
{
convexified.pop_back();
}

@wawanbreton Erwan MATHIEU (wawanbreton) Aug 17, 2026

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 ?

@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.

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 •
edited
Loading

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.

};

std::stable_sort(
Expand Down
38 changes: 38 additions & 0 deletions tests/utils/PolygonTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,16 @@

#include <numbers>

#include <range/v3/view/sliding.hpp>
#include <spdlog/spdlog.h>

#include <gtest/gtest.h>

#include "geometry/OpenPolyline.h"
#include "geometry/SingleShape.h"
#include "utils/Coord_t.h"
#include "utils/SVG.h" // helper functions
#include "utils/linearAlg2D.h"
#include "utils/polygonUtils.h" // helper functions

// NOLINTBEGIN(*-magic-numbers)
Expand Down Expand Up @@ -298,6 +302,40 @@ TEST_F(PolygonTest, convexHullStar)
}
}

TEST_F(PolygonTest, makeConvexPolygon)
{
Polygon polygon;
polygon.setPoints({
{ 110139, 106039 }, { 111866, 106001 }, { 114033, 106017 }, { 114409, 106046 }, { 114739, 106087 }, { 114982, 106136 }, { 115131, 106193 }, { 115113, 106326 },
{ 115118, 106460 }, { 114915, 106496 }, { 114540, 106512 }, { 114198, 106505 }, { 112338, 106507 }, { 112045, 106531 }, { 111795, 106577 }, { 111609, 106643 },
{ 111498, 106733 }, { 111495, 109561 }, { 111396, 109642 }, { 111229, 109704 }, { 110997, 109750 }, { 110995, 110250 }, { 111227, 110296 }, { 111395, 110358 },
{ 111494, 110438 }, { 111497, 113260 }, { 111553, 113334 }, { 111686, 113402 }, { 111895, 113457 }, { 112241, 113498 }, { 112582, 113507 }, { 114483, 113512 },
{ 114760, 113535 }, { 114879, 113573 }, { 114869, 113841 }, { 114637, 113904 }, { 114404, 113944 }, { 113970, 113985 }, { 113655, 114001 }, { 112302, 114010 },
{ 109861, 113961 }, { 108134, 113999 }, { 105967, 113983 }, { 105591, 113954 }, { 105261, 113913 }, { 105018, 113864 }, { 104869, 113807 }, { 104887, 113674 },
{ 104882, 113540 }, { 105085, 113504 }, { 105460, 113488 }, { 105796, 113495 }, { 107661, 113493 }, { 107811, 113484 }, { 108086, 113448 }, { 108307, 113392 },
{ 108501, 113294 }, { 108505, 110439 }, { 108604, 110358 }, { 108771, 110296 }, { 109004, 110250 }, { 109005, 109750 }, { 108773, 109704 }, { 108605, 109642 },
{ 108506, 109562 }, { 108506, 106720 }, { 108372, 106613 }, { 108106, 106544 }, { 107900, 106513 }, { 107418, 106493 }, { 105517, 106488 }, { 105240, 106465 },
{ 105121, 106427 }, { 105131, 106159 }, { 105363, 106096 }, { 105596, 106056 }, { 106030, 106015 }, { 106345, 105999 }, { 107698, 105990 }, { 110139, 106039 },
});

auto shape = Shape(polygon);
shape.makeConvex();
const auto convex_hull = shape[0];

// test that all corners are convex
EXPECT_TRUE(LinearAlg2D::pointIsLeftOfLine(convex_hull.front(), convex_hull.back(), convex_hull[1]) < 0);
EXPECT_TRUE(LinearAlg2D::pointIsLeftOfLine(convex_hull.back(), convex_hull[convex_hull.size() - 2], convex_hull.front()) < 0);

for (const auto window : convex_hull | ranges::views::sliding(3))
{
const auto& a = window[0];
const auto& b = window[1];
const auto& c = window[2];

EXPECT_TRUE(LinearAlg2D::pointIsLeftOfLine(b, a, c) < 0);
}
}

/*
* Multiple min-x points
* the convex hull the point with minimal x value. if there are multiple it might go wrong
Expand Down
Loading