diff --git a/Line.h b/Line.h index 8ff77cb..e198558 100644 --- a/Line.h +++ b/Line.h @@ -42,7 +42,8 @@ public: other -= from(); // Cross product greater than zero? - if (m_to_from_origin.x() * other.y() - m_to_from_origin.y() * other.x() > 0) + // Note: We need ">=" here so that we include points which are directly on the existing hull. + if (m_to_from_origin.x() * other.y() - m_to_from_origin.y() * other.x() >= 0) { return true; } diff --git a/Quickhull.h b/Quickhull.h index 3490842..5cfac6a 100644 --- a/Quickhull.h +++ b/Quickhull.h @@ -132,24 +132,20 @@ private: // these properly? We definitely need to remove them all from input later; // do we also need to handle them all further? This hotfix works, but seems // like an unnecessarily big performance hit for that edge case. - // FIXME: This workaround also causes problems with extremely large numbers - // of randomly generated numbers, causing random lines within the data! - // Points inside the hull are added because of random lines. for (const Point &point : input) { float this_distance = line.distance_squared_to(point); - // TODO: Both are required, otherwise only one side of the rectangle is - // taken -- why? - if (this_distance == furthest_distance || line.distance_squared_to(point) == 0) + if (this_distance == furthest_distance) { output.emplace_back(point); } } + // Remove the previously checked points from the input -- we can't do that in + // the previous loop because we can't delete from the list we're iterating over input.remove_if([furthest_distance, line](Point point) { return furthest_distance == line.distance_squared_to(point); }); - // Hotfix end output.emplace_back(furthest_point); input.remove(furthest_point); @@ -173,12 +169,9 @@ private: new_line2 = Line(furthest_point, line.to()); } - // TODO: Test if this improves performance - //Triangle triangle(a, b, c); - //input.remove_if([triangle](Point point) - //{ - // return triangle.is_point_inside(point); - //}); + // We don't need to remove points inside the triangle created by those lines. + // That happens implicitly since they are not handled further due to the + // following step, which assigns each line its corresponding points to handle: // Get points right of new_line1 and 2 std::list left_of_line1, left_of_line2; @@ -189,8 +182,7 @@ private: { left_of_line1.emplace_back(point); } - // TODO: Can we do else if here, or could we then miss out on points? - if (!new_line2.is_point_right(point)) + else if (!new_line2.is_point_right(point)) { left_of_line2.emplace_back(point); }