Skip to content
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

Closed
wants to merge 1 commit into from
Closed

Add support to perform boolean operations on array of polypaths #29881

wants to merge 1 commit into from

Conversation

Xrayez
Copy link
Contributor

@Xrayez Xrayez commented Jun 18, 2019

Enhances #28987.
Closes #29784.

multiple_polygons_clipping

  • orange polygons aka polygons_a aka subject;
  • blue polygons aka polygons_b aka clip;
  • green polygons are solution per boolean operation, in order: UNION, DIFFERENCE, INTERSECTION, XOR.

Changes:

  • polygon clipping is done on vector of polygons internally now.
  • bind Variant to mix and match both single polygon operations and array of polygons/polylines.
  • merging polygons requires only one parameter now.
    • NonZero filling was choosen by default to make this happen (see comment upstream).
  • did the same treatment for polygon/polyline inflating/deflating methods.
  • update docs.

Examples:

var poly_a = PoolVector2Array([Vector2(), ...])
var poly_a = PoolVector2Array([Vector2(), ...])
var poly_a = PoolVector2Array([Vector2(), ...])

# before: no total merge guaranteed
var res = Geometry.merge_polygons_2d(poly_a, poly_b)
res = Geometry.merge_polygons_2d(res[0], poly_c)

# after: merge guaranteed if all overlap
var res = Geometry.merge_polygons_2d([poly_a, poly_b, poly_c])

# Mix and match:

# `a` polygon is clipped by both `b` and `c` polygons:
var res = Geometry.clip_polygons_2d(poly_a, [poly_b, poly_c])

# `a` and `b` polygons are clipped by a single `c` polygon:
var res = Geometry.clip_polygons_2d([poly_a, poly_b], poly_c)

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

@ghost
Copy link

ghost commented Jun 19, 2019

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.

var res = Geometry.merge_polygons_2d([poly_a, poly_b, poly_c]) would merge them regardless of overlap?

@Xrayez
Copy link
Contributor Author

Xrayez commented Jun 19, 2019

Will give it a look, though I I may have to await the updated documentation.

Nothing really changed API-wise, except methods can accept either/both arrays and vertices.

I'm not exactly sure about the parameters, other than what you had mentioned about subjects being the first parameter.

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)

var res = Geometry.merge_polygons_2d([poly_a, poly_b, poly_c]) would merge them regardless of overlap?

If they all overlap geometrically , res will be an array containing a single polygon. Otherwise the result can contain:

  • two of the input polygons got merged into one, another polygon not merged (res.size() == 2).
  • all original polygons will be returned if no polygon overlap with each other (res.size() == 3).

@ghost
Copy link

ghost commented Jun 19, 2019

Oh I see, I was thinking one of points of the discussion was having a direct method of taking two distant polygons like:

image

And a merging them like this:

image

Likely I misunderstood.

@Xrayez
Copy link
Contributor Author

Xrayez commented Jun 19, 2019

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

@ghost
Copy link

ghost commented Jun 19, 2019

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

@Xrayez
Copy link
Contributor Author

Xrayez commented Jun 20, 2019

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.

@Xrayez Xrayez marked this pull request as ready for review June 20, 2019 13:30
@Xrayez Xrayez requested a review from a team as a code owner June 20, 2019 13:30
@@ -31,6 +31,8 @@
#ifndef GEOMETRY_H
#define GEOMETRY_H

#include "thirdparty/misc/clipper.hpp"
Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Contributor Author

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

@Xrayez
Copy link
Contributor Author

Xrayez commented Aug 23, 2019

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

@torshid
Copy link
Contributor

torshid commented Sep 1, 2019

I think that this PR is absolutely needed (maybe not for many people, but it is much more logical than the previous implementation). I was able to finalize an enhanced version of Navigation2D based on quad trees in a few minutes after struggling hours for nothing.
navigation

- 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.
@Xrayez
Copy link
Contributor Author

Xrayez commented Sep 1, 2019

Resolved merge conflicts, updated test project in accordance to recent #31761 and #31865.

@Xrayez
Copy link
Contributor Author

Xrayez commented Feb 5, 2020

Closing for now as I've figured a more safe and unified way to achieve this at #35929.

@Xrayez Xrayez closed this Feb 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to perform polygon boolean operation on a list of polygons simultaneously
5 participants