-
-
Notifications
You must be signed in to change notification settings - Fork 21.8k
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.
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
Add support to perform boolean operations on array of polypaths #29881
Conversation
Will give it a look, though I I may have to await the updated documentation. Did you mean var poly_a, b, and c in the first lines above? I'm not exactly sure about the parameters, other than what you had mentioned about subjects being the first parameter.
|
Nothing really changed API-wise, except methods can accept either/both arrays and vertices.
It may look confusing because an example shows the direct usage of array to be constructed within parameter. This is a more realistic situation: var polygons = [
PoolVector2Array([Vector2(), ...])
PoolVector2Array([Vector2(), ...])
PoolVector2Array([Vector2(), ...])
]
var cut = PoolVector2Array([Vector2(), ...])
var solution = Geometry.clip_polygons(polygons, cut)
for polygon in solution:
print(polygon)
If they all overlap geometrically ,
|
@avencherus ah, I think what you're describing could likely be handled nicely with: var result_poly = Geometry.convex_hull_2d(a_list_of_all_polygon_vertices_combined) alternatively, in the context of polyboolean operations: var triangles = Geometry.triangulate_delaunay_2d(a_list_of_all_polygon_vertices_combined)
# ... code to transfrom triangle indices to vertices here ...
# grow a bit to make sure all get merged, but likely this step can be skipped:
# yet this could also result in polygons get merged already by this operation alone!
# var triangles = Geometry.offset_polygons_2d(triangles, 1.0)
var polys = Geometry.merge_polygons_2d(triangles)
assert(polys.size() == 1)
var result = polys[0] For Delaunay triangulation see #29127. |
I've used them. The first one is out for concaves where the untouched areas need to be preserved, and I'm not too sure about complexity or optimization issues that might occur with generating dozens or more little triangle polygons with Delaunay. What I would be aiming at is dynamic 2D terrains using textured polygons. IE - Things would get carved out by explosions, or certain types of procedural generation. I suspect the clipper library would save a lot of reinventing the wheel when it comes time. Things would have to be profiled on an extensive prototype to know which to use. But that's some distant project for me, Godot 4 will probably be stable by then. XD |
I think the work is done but I'd like to test this more thoroughly. Method naming could also be discussed, but I think it makes sense for them to be in plural form even if a single polygon/polyline is passed. Example: var polyline : PoolVector2Array
var polygon : PoolVector2Array
# before:
Geometry.clip_polyline_with_polygon(polyline, polygon) # ok
# after:
Geometry.clip_polylines_with_polygons(polyline, polygon)
# ok but might be confusing (note: both parameters can accept an array of polygons too) It's going to be confusing in any case so documentation should clear that up either way. |
core/math/geometry.h
Outdated
@@ -31,6 +31,8 @@ | |||
#ifndef GEOMETRY_H | |||
#define GEOMETRY_H | |||
|
|||
#include "thirdparty/misc/clipper.hpp" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we avoid having a thirdparty include in a core header? That would mean pulling clipper code everywhere geometry.h
is included, I'd prefer that it stays only in the .cpp.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah... I added those for utility functions _scale_up_polypaths
, _scale_down_polypaths
, those could be taken away from Geometry
declaration I think (was afraid to pollute global namespace for some reason).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Used a GodotClipperUtils
namespace but reduz wouldn't like that according to #30736 (comment), so not sure. 😅
From all related opened PRs which are more likely to be merged and which would introduce merge conflicts with the current PR is #29758, so waiting for review I guess. But I'm not completely sure whether these enhancements would make the API more cumbersome or not, I guess it could be as well closed for now or moved to the next milestone, reduz haven't expressed clear opinion regarding this particular implementation though, yet I don't feel like it's going to be approved unless more users express interest in this (and there's no way to get more users to support this before they actually attempt to use these new features in 3.2-alpha/beta/stable). |
- polygon and polyline clipping is done on vector of polygons internally; - mix and match both single polygon operations and array of polygons; - merging polygons requires only one parameter as an array; - NonZero filling was choosen by default to make this happen. - similar changes done for polygon/polyline inflating/deflating methods; - updated documentation accordingly.
Closing for now as I've figured a more safe and unified way to achieve this at #35929. |
Enhances #28987.
Closes #29784.
polygons_a
akasubject
;polygons_b
akaclip
;Changes:
Variant
to mix and match both single polygon operations and array of polygons/polylines.Examples:
One could see how this kinda increased complexity a bit internally, but at the same time I've managed to reuse recurrent procedures used for binding. I also wonder how this could affect performance and interfere with static typing.
Feedback welcomed, inviting for discussion people who expressed interest in this:
@Dr4kzor @avencherus, @Daw11
Test project
geometry-clipper-array.zip