Added Geometry2D unit tests

[mbrlabs]

Some functions are not completely unit-testable, because the results are not always either right or wrong (e.g. make_atlas or the triangulate_* functions can have many correct results). Some are also not tested yet, but this should be good for now (feel free to add more tests in seperate PRs). I included the complete list of tested functions below.

Part of #43440

Tests included in this PR

• is_point_in_circle
• is_point_in_triangle
• is_point_in_polygon
• is_polygon_clockwise
• line_intersects_line
• segment_intersects_segment
• get_closest_point_to_segment
• get_closest_point_to_segment_uncapped
• get_closest_points_between_segments
• make_atlas
• intersect_polygons
• merge_polygons
• clip_polygons
• exclude_polygons
• intersect_polyline_with_polygon
• clip_polyline_with_polygon

Not tested:

• offset_polygon
• offset_polyline
• convex_hull
• triangulate_polygon
• triangulate_delaunay
• segment_intersects_circle (not exposed to GDScript)
• decompose_polygon_in_convex (not exposed to GDScript)
• pack_rects (not exposed to GDScript)
• partial_pack_rects (not exposed to GDScript)
• vec2_cross (not exposed to GDScript)

cc @Calinou @Xrayez for review if you have time 🙂

[Xrayez]

Looks good!

Boolean methods cover the documented functionality, as I recall (so is the rest I presume). 🙂

[mbrlabs]

So turns out is_point_in_triangle and is_point_in_polygon treat points on the edge as outside, while is_point_in_circle treats them as inside. That's an inconsistency and should probably be documented or even changed (for 4.0 at least). For now i added checks for them anyway.

I also converted the plain asserts in the polygon tests to subcases.

[Xrayez]

That's an inconsistency and should probably be documented or even changed (for 4.0 at least).

There's a better and more efficient implementation in Goost (ported from Clipper's code currently used by boolean methods in Godot): https://github.com/goostengine/goost/blob/gd3/core/math/2d/geometry/goost_geometry_2d.cpp#L165-L212, the implementation treats those cases separately by returning 0 if outside, 1 if inside, and -1 if a point is exactly on the edge.

So yeah that's something which can be done, I recall @reduz wasn't against the general idea but he still had doubts whether it's worth to replace the implementation last time we talked about this... Some IRC logs months ago:

vmjcv> Hello everyone, I want to ask about the algorithm of the is_point_in_polygon function, I am confused about the Vector2 (1.221313, 1.512312) used in it,
reduz> its probably to avoid numerical precision issues when a ray passes between two points
i mean, between two segments
since the algorithm i think counts the amount of times the point passes through a segment
reduz> vmjcv: it had to be something around 1 with a lot of decimals of any kind to make intersecting a point less likely. Could have used 1.323423 or 0.928742 or anything really
the numbers also had to be different to avoid a situation of hitting something diagonally
any two numbers close to 1 with lots of decimals, different between them will work
vmjcv> Thank you very much for your answers, otherwise I guess I can't sleep for a day <3
Xrayez> vmjcv: you can also checkout Clipper's implementation of point_in_polygon: https://github.com/godotengine/godot/blob/master/thirdparty/misc/clipper.cpp#L472
reduz> Xrayez: thats probably a better implementation, using determinant, should likely replace it
Xrayez> reduz: I have it implemented in one of my modules already working with Godot types, could work on that.
reduz> its only used for tools honestly so it does not make much of a difference
Xrayez> is_point_in_polygon is currently exposed to scripts, so not entirely editor now. :)
Clipper's implementation can detect whether a point lies outside, inside, or directly on the polygon as well.

[Xrayez]

So turns out is_point_in_triangle and is_point_in_polygon treat points on the edge as outside, while is_point_in_circle treats them as inside.

Recent report for this #44717. 🙂

[mbrlabs]

@akien-mga Rebased and updated. As discussed on IRC i did 2 more changes:

• Added comments to the cases in the tests for is_point_in_triangle, is_point_in_polygon and is_point_in_circle where the point lies directly on the edge because the current behaviour may not be desired or will change in the future (see Geometry::is_point_in_triangle returns false on triangle borders #44717 and Xrayez's previous comment about that).
• Changed how the 0-check is done in Geometry2D::line_intersects_line to hopefully prevent an incorrect MSVC warning/error (C4723) about division by 0

[akien-mga]

Needs another rebase now that the /bigobj PR is merged.

[mbrlabs]

Done, but now C4723: potential divide by 0 is back :(

2020-12-30T14:39:05.2679851Z D:\a\godot\godot\core\math\geometry_2d.h(160) : error C2220: the following warning is treated as an error
2020-12-30T14:39:05.5461610Z D:\a\godot\godot\core\math\geometry_2d.h(160) : warning C4723: potential divide by 0

Bug report with an official response from the MSVC developers: https://developercommunity.visualstudio.com/content/problem/961455/spurious-c4723-warning.html

[mbrlabs]

I wasn't able to fix the C4723: potential divide by 0 warning directly in Geometry2D::line_intersects_line, so i reverted the changes there and excluded the CHECK that exposed that MSVC bug from Windows builds. This is ugly, but shouldn't be an issue since the check is still executed on the Linux and Mac builds.

[akien-mga]

I think you can disable the warning around the problematic code with:

#ifdef _MSC_VER
// False positive, we correctly check for zero earlier.
#pragma warning ( push )
#pragma warning ( disable C4723 )
#endif
// Problematic code
#ifdef _MSC_VER
#pragma warning ( pop )
#endif

See https://docs.microsoft.com/en-us/cpp/preprocessor/warning?view=msvc-160

[mbrlabs]

That's better and works, but i had to disable the warning before the function definition and re-enable it afterwards.

[akien-mga]

Thanks!