Skip to content

Commit

Permalink
Fix incorrect return in is_polygon_clockwise
Browse files Browse the repository at this point in the history
When this was originally implemented in
16022da, the implementation treated the
vertices as being in a space where the y-axis is positive in the up
direction. This caused it to return the incorrect value, since Godot
treats the y-axis as being positive in the down direction.

All existing uses of `is_polygon_clockwise` have been updated
in a way which preserves the existing behavior with the comments being
updated to refer to the correct winding order.

Fixes godotengine#49716.
  • Loading branch information
exodrifter committed Feb 24, 2024
1 parent 2e7fc81 commit 7961ddf
Show file tree
Hide file tree
Showing 6 changed files with 13 additions and 13 deletions.
2 changes: 1 addition & 1 deletion core/math/geometry_2d.h
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,7 @@ class Geometry2D {
sum += (v2.x - v1.x) * (v2.y + v1.y);
}

return sum > 0.0f;
return sum < 0.0f;
}

// Alternate implementation that should be faster.
Expand Down
2 changes: 1 addition & 1 deletion scene/2d/navigation_obstacle_2d.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,7 @@ void NavigationObstacle2D::_update_fake_agent_radius_debug() {
#ifdef DEBUG_ENABLED
void NavigationObstacle2D::_update_static_obstacle_debug() {
if (get_vertices().size() > 2 && NavigationServer2D::get_singleton()->get_debug_navigation_avoidance_enable_obstacles_static()) {
bool obstacle_pushes_inward = Geometry2D::is_polygon_clockwise(get_vertices());
bool obstacle_pushes_inward = !Geometry2D::is_polygon_clockwise(get_vertices());

Color debug_static_obstacle_face_color;

Expand Down
2 changes: 1 addition & 1 deletion scene/2d/navigation_region_2d.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -445,7 +445,7 @@ void NavigationRegion2D::_update_avoidance_constrain() {
for (int i(0); i < outline_size; i++) {
new_obstacle_outline.push_back(_outline[outline_size - i - 1]);
}
ERR_FAIL_COND_MSG(Geometry2D::is_polygon_clockwise(_outline), "Outer most outline needs to be clockwise to push avoidance agent inside");
ERR_FAIL_COND_MSG(!Geometry2D::is_polygon_clockwise(_outline), "Outer most outline needs to be counter-clockwise to push avoidance agent inside");
} else {
for (int i(0); i < outline_size; i++) {
new_obstacle_outline.push_back(_outline[i]);
Expand Down
2 changes: 1 addition & 1 deletion scene/3d/navigation_obstacle_3d.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -465,7 +465,7 @@ void NavigationObstacle3D::_update_static_obstacle_debug() {
return;
}

bool obstacle_pushes_inward = Geometry2D::is_polygon_clockwise(polygon_2d_vertices);
bool obstacle_pushes_inward = !Geometry2D::is_polygon_clockwise(polygon_2d_vertices);

Vector<Vector3> face_vertex_array;
Vector<int> face_indices_array;
Expand Down
2 changes: 1 addition & 1 deletion scene/resources/convex_polygon_shape_2d.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ bool is_convex(const Vector<Vector2> &p_points) {

void ConvexPolygonShape2D::_update_shape() {
Vector<Vector2> final_points = points;
if (Geometry2D::is_polygon_clockwise(final_points)) { //needs to be counter clockwise
if (!Geometry2D::is_polygon_clockwise(final_points)) { // Needs to be counter-clockwise.
final_points.reverse();
}
#ifdef DEBUG_ENABLED
Expand Down
16 changes: 8 additions & 8 deletions tests/core/math/test_geometry_2d.h
Original file line number Diff line number Diff line change
Expand Up @@ -113,10 +113,10 @@ TEST_CASE("[Geometry2D] Polygon clockwise") {
p.push_back(Vector2(-5, -1));
p.push_back(Vector2(-1, 3));
p.push_back(Vector2(1, 5));
CHECK(Geometry2D::is_polygon_clockwise(p));
CHECK_FALSE(Geometry2D::is_polygon_clockwise(p));

p.reverse();
CHECK_FALSE(Geometry2D::is_polygon_clockwise(p));
CHECK(Geometry2D::is_polygon_clockwise(p));
}

TEST_CASE("[Geometry2D] Line intersection") {
Expand Down Expand Up @@ -467,7 +467,7 @@ TEST_CASE("[Geometry2D] Merge polygons") {
r = Geometry2D::merge_polygons(a, b);
REQUIRE_MESSAGE(r.size() == 2, "The merged polygons should result in 2 polygons.");

REQUIRE_MESSAGE(!Geometry2D::is_polygon_clockwise(r[0]), "The merged polygon (outline) should be counter-clockwise.");
REQUIRE_MESSAGE(Geometry2D::is_polygon_clockwise(r[0]), "The merged polygon (outline) should be clockwise.");
REQUIRE_MESSAGE(r[0].size() == 7, "The resulting merged polygon (outline) should have 7 vertices.");
CHECK(r[0][0].is_equal_approx(Point2(174.791077, 161.350967)));
CHECK(r[0][1].is_equal_approx(Point2(225, 180)));
Expand All @@ -477,7 +477,7 @@ TEST_CASE("[Geometry2D] Merge polygons") {
CHECK(r[0][5].is_equal_approx(Point2(81.911758, 126.852943)));
CHECK(r[0][6].is_equal_approx(Point2(160, 80)));

REQUIRE_MESSAGE(Geometry2D::is_polygon_clockwise(r[1]), "The resulting merged polygon (hole) should be clockwise.");
REQUIRE_MESSAGE(!Geometry2D::is_polygon_clockwise(r[1]), "The resulting merged polygon (hole) should be counter-clockwise.");
REQUIRE_MESSAGE(r[1].size() == 3, "The resulting merged polygon (hole) should have 3 vertices.");
CHECK(r[1][0].is_equal_approx(Point2(98.083069, 132.859421)));
CHECK(r[1][1].is_equal_approx(Point2(158.689453, 155.370377)));
Expand Down Expand Up @@ -529,13 +529,13 @@ TEST_CASE("[Geometry2D] Clip polygons") {
r = Geometry2D::clip_polygons(a, b);
REQUIRE_MESSAGE(r.size() == 2, "Polygon 'a' completely overlaps polygon 'b'. This should result in 2 clipped polygons.");
REQUIRE_MESSAGE(r[0].size() == 4, "The resulting clipped polygon should have 4 vertices.");
REQUIRE_MESSAGE(!Geometry2D::is_polygon_clockwise(r[0]), "The resulting clipped polygon (outline) should be counter-clockwise.");
REQUIRE_MESSAGE(Geometry2D::is_polygon_clockwise(r[0]), "The resulting clipped polygon (outline) should be clockwise.");
CHECK(r[0][0].is_equal_approx(a[0]));
CHECK(r[0][1].is_equal_approx(a[1]));
CHECK(r[0][2].is_equal_approx(a[2]));
CHECK(r[0][3].is_equal_approx(a[3]));
REQUIRE_MESSAGE(r[1].size() == 3, "The resulting clipped polygon should have 3 vertices.");
REQUIRE_MESSAGE(Geometry2D::is_polygon_clockwise(r[1]), "The resulting clipped polygon (hole) should be clockwise.");
REQUIRE_MESSAGE(!Geometry2D::is_polygon_clockwise(r[1]), "The resulting clipped polygon (hole) should be counter-clockwise.");
CHECK(r[1][0].is_equal_approx(b[1]));
CHECK(r[1][1].is_equal_approx(b[0]));
CHECK(r[1][2].is_equal_approx(b[2]));
Expand Down Expand Up @@ -575,13 +575,13 @@ TEST_CASE("[Geometry2D] Exclude polygons") {
r = Geometry2D::exclude_polygons(a, b);
REQUIRE_MESSAGE(r.size() == 2, "There should be 2 resulting excluded polygons (outline and hole).");
REQUIRE_MESSAGE(r[0].size() == 4, "The resulting excluded polygon should have 4 vertices.");
REQUIRE_MESSAGE(!Geometry2D::is_polygon_clockwise(r[0]), "The resulting excluded polygon (outline) should be counter-clockwise.");
REQUIRE_MESSAGE(Geometry2D::is_polygon_clockwise(r[0]), "The resulting excluded polygon (outline) should be clockwise.");
CHECK(r[0][0].is_equal_approx(a[0]));
CHECK(r[0][1].is_equal_approx(a[1]));
CHECK(r[0][2].is_equal_approx(a[2]));
CHECK(r[0][3].is_equal_approx(a[3]));
REQUIRE_MESSAGE(r[1].size() == 4, "The resulting excluded polygon should have 4 vertices.");
REQUIRE_MESSAGE(Geometry2D::is_polygon_clockwise(r[1]), "The resulting excluded polygon (hole) should be clockwise.");
REQUIRE_MESSAGE(!Geometry2D::is_polygon_clockwise(r[1]), "The resulting excluded polygon (hole) should be counter-clockwise.");
CHECK(r[1][0].is_equal_approx(Point2(40, 200)));
CHECK(r[1][1].is_equal_approx(Point2(150, 220)));
CHECK(r[1][2].is_equal_approx(Point2(140, 160)));
Expand Down

0 comments on commit 7961ddf

Please sign in to comment.