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

[WIP] Clipper2 (beta) wrapper implementation #23559

Closed
wants to merge 7 commits into from
Closed

[WIP] Clipper2 (beta) wrapper implementation #23559

wants to merge 7 commits into from

Conversation

Xrayez
Copy link
Contributor

@Xrayez Xrayez commented Nov 6, 2018

Clipper2 is beta and is undergoing a rewrite by Angus Johnson

This PR is at experimental state. I have stable 6.4.2 exposed with the interface to be forward compatible with the new version (10.0.0) and could open a separate PR for it.


There's been some recent demand on implementing some kind of ability to perform various operations on polygons, and this PR brings many tasty stuff like:

  • Polygon merging/subtraction/intersection/xor.
  • Polygon deflating/offsetting (grow/shrink boundary).
  • Curve clipping with polygon.
  • Polygon triangulation (new feature as of Clipper2).

The underlying library differs from already used version present in the engine which is stable 6.4.2.

The new version should have a built-in wrapper for float2int conversion as clipping occurs with integer coordinates to avoid floating point accumulation errors.

Notice that I've already made necessary changes so it works as before in places where Clipper is used.

Relevant issues/feature proposals:
#22275, #3821

Module repo:
https://github.com/Xrayez/godot-clipper

Clipper2 source (beta):
https://sourceforge.net/p/polyclipping/code/HEAD/tree/sandbox/Clipper2/

@Xrayez Xrayez changed the title Clipper wrapper implementation [WIP] Clipper wrapper implementation Nov 6, 2018
@reduz
Copy link
Member

reduz commented Nov 6, 2018 via email

@Zireael07
Copy link
Contributor

@reduz: This version of clipper has more features, including triangulation.

@reduz
Copy link
Member

reduz commented Nov 6, 2018

If it's better than our current one we could aswell replace it too, i guess

@Xrayez
Copy link
Contributor Author

Xrayez commented Nov 6, 2018

If it's better than our current one we could aswell replace it too, i guess

Yeah this is the main concern I have with Clipper. It's currently used by the sprite editor plugin:

Vector<Vector2> expand(const Vector<Vector2> &points, const Rect2i &rect, float epsilon = 2.0) {
int size = points.size();
ERR_FAIL_COND_V(size < 2, Vector<Vector2>());
ClipperLib::Path subj;
ClipperLib::PolyTree solution;
ClipperLib::PolyTree out;
for (int i = 0; i < points.size(); i++) {
subj << ClipperLib::IntPoint(points[i].x * PRECISION, points[i].y * PRECISION);
}

PRECISION here is used to scale up and down points because clipper requires input to be performed on integers to avoid floating point accumulation error. This is handled by the proposed module transparently.

//turn the result into simply polygon (AKA, fix overlap)
//clamp into the specified rect
ClipperLib::Clipper cl;
cl.StrictlySimple(true);
cl.AddPath(p->Contour, ClipperLib::ptSubject, true);

This is no longer needed in Clipper2 as it guarantees strictly simple polygons as output.

I'll see if I can refactor the existing use of clipper with this one.

@Xrayez
Copy link
Contributor Author

Xrayez commented Nov 7, 2018

Replaced with new Clipper and refactored expand method in sprite editor plugin and it works alright on my side. There are several compilation errors spotted by CI but these can be fixed I guess. I'm more curious whether core devs are OK with having beta version of Clipper to be present in core.

It's been one year since Angus Johnson released his beta version... Yet he's been active for the last month or so working on this. If anything we could stick to 6.4.2 and just expose methods as reduz suggested. But that would mean no triangulation unit and twice as fast calculations.

I wish Angus to get better ❤
https://sourceforge.net/p/polyclipping/discussion/1148419/thread/ff9241dd3b/#cb12

@@ -101,8 +103,8 @@ namespace clipperlib {

PolyPath& PolyPath::GetChild(unsigned index)
{
if (index < 0 || index >= childs_.size())
throw ClipperException("invalid range in PolyPath::GetChild.");
// ERR_FAIL_INDEX_V(index, childs_.size(), PolyPath());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how to return non-const reference here...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct me if I'm wrong, but I'm don't think we're supposed to be touching thirdparty sources unless it's a bugfix or we're updating it to a new version -- any code related to Godot's implementation of a library is supposed to go into the related modules/ files, to my knowledge.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure either, it wouldn't compile otherwise for some other platforms with not exception-safe code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seeing thirdparty/README.md gives a hint that it can be done via denoting changes and/or generating an actual diff file against the original source.

@tengkuizdihar
Copy link

@Xrayez thank you

@Xrayez
Copy link
Contributor Author

Xrayez commented Nov 22, 2018

Seems like Clipper2 development has resumed recently!

https://sourceforge.net/p/polyclipping/code/541/

  1. All C++ and C# code has been removed, as the new Clipper.pas will need to be translated.
  2. All triangulation code has removed (temporarily) as it no longer works with the updated Clipper class.

https://sourceforge.net/p/polyclipping/bugs/188/#db79

Angus Johnson: I'm currently totally focused on getting Clipper2 finished...

That means current wrapper code needs to be updated too at some point...

Taking this into account, it seams reasonable to wrap the older stable version (6.4.2) for now... So I might as well create another pull request just for 6.4.2 and leave this PR until the newer version comes out.

@Xrayez Xrayez changed the title [WIP] Clipper wrapper implementation [WIP] Clipper2 wrapper implementation Nov 22, 2018
@blurymind
Copy link

Perhaps the new triangulation features could be used to add internal vertices to 2d meshes? #20601
not sure if the current implementation uses clipper to create the mesh

@Xrayez
Copy link
Contributor Author

Xrayez commented Dec 14, 2018

@blurymind triangulation in new clipper seems to implement ear-clipping algorithm (just guessing), unfortunately it doesn't have ability to specify internal vertices. It could be made possible to emulate internal vertices as holes in clipper, that way it can triangulate the input taking into account hole's vertices. Not sure if that would interfere with mesh deformation, I haven't dealt with it.

not sure if current implementation uses clipper to create the mesh

Currently it's only used for sprite editor plugin to expand and simplify the mesh of a sprite when creating mesh instance.

I've had experience using poly2tri that does allow to create inner vertices via Steiner points, but the library can't handle degenerate input and throws exceptions:

Since there are no Input validation of the data given for triangulation you need
to think about this. Poly2Tri does not support repeat points within epsilon.

Meaning If inner vertices are added outside polygon, it would crash.

@tengkuizdihar
Copy link

will it get merged I wonder

@Xrayez
Copy link
Contributor Author

Xrayez commented Dec 24, 2018

@balenol not in 3.1 at least. The underlying library's version might be unsatisfactory to be merged (because beta). If that's the case, I'll open a separate pull request with exposed older stable Clipper 6.4.2 with the same interface as in this one (currently present in engine), so the update is smooth in case newer version (10.0.0) is declared stable by Angus Johnson. But that would mean no triangulation unit. Most likely we'll just have to stick to 6.4.2 in this case...

@reduz
Copy link
Member

reduz commented Dec 24, 2018

@blurymind For internal vertices, I am not sure if anything other than painting the internal polygons by hand would work, at least not if you want deformation to happen in any way you expect it.. I was planning to implement it that way.

@Xrayez Xrayez changed the title [WIP] Clipper2 wrapper implementation [WIP] Clipper2 (beta) wrapper implementation Dec 25, 2018
@blurymind
Copy link

@blurymind For internal vertices, I am not sure if anything other than painting the internal polygons by hand would work, at least not if you want deformation to happen in any way you expect it.. I was planning to implement it that way.

@reduz I don't mind if it's done by painting them by hand, for as long as we can do it :)
I was actually wondering if we could do it with blender and import the mesh via something like @ndee85 's coa-tools godot exporter

@Xrayez
Copy link
Contributor Author

Xrayez commented Jan 16, 2019

Recent update by Angus:
https://sourceforge.net/p/polyclipping/code/543/

We'll have to wait for C++ version, the code in this PR shall be changed too then.

Andrii Doroshenko (Xrayez) added 6 commits March 4, 2019 18:16
* Replace 6.4.2 with 10.0.0 beta (?) (dedicated offset and triangulation)
* Scons: add thirdparty clipper sources via module, not editor
* Refactor `expand` method in sprite editor plugin that is used for mesh instance creation
…tion

Open paths clipping is supported via specifying `is_a_open` parameter.
@Xrayez Xrayez requested a review from akien-mga as a code owner March 4, 2019 16:55
@Xrayez
Copy link
Contributor Author

Xrayez commented May 17, 2019

Recently I started to question the idea of exposing Clipper as a wrapper class, I think users shouldn't really care about this and it would add unnecessary complexity...

I was thinking that maybe it would be a better idea to expose the relevant methods in Geometry singleton. As reduz pointed out, there's already triangulate_polygon and even clip_polygon (which seems like it's tailored for 3D case rather than 2D), what I'm proposing instead is to expose these kind methods to be available in Geometry singleton:

Array merge_poly_2d(const Vector<Vector2> &poly_a, const Vector<Vector2> &polygon_b, bool is_a_open = false);
Array clip_poly_2d(const Vector<Vector2> &poly_a, const Vector<Vector2> &polygon_b, bool is_a_open = false);
Array intersect_poly_2d(const Vector<Vector2> &poly_a, const Vector<Vector2> &polygon_b, bool is_a_open = false);
Array exclude_poly_2d(const Vector<Vector2> &poly_a, const Vector<Vector2> &polygon_b, bool is_a_open = false); // xor
Array offset_poly_2d(const Vector<Vector2> &poly, real_t p_offset, bool is_open = false, int end_type = 0); // negative to shrink, positive to expand
// Array triangulate(const Vector<Vector2> &poly);

Note that clipping operation can be done on poly_a to be a polyline (open path) rather than a polygon (closed path), hence the naming. Also note that the result of any such method can also yield multiple polygons (possibly with holes which could be distinguished by computing orientation), hence Array is returned (could also return a dictionary separating outer from inner polygons so you won't have to recompute orientation).

Offsettng polylines could also have different flavors which would result in a polygon (end types):

types

What do you think?

EDIT: also it's unlikely that a stable version is going to be released anytime soon, so I'm thinking to reverting back to 6.4.2 which is currently used by the engine.

@TotCac
Copy link

TotCac commented May 17, 2019

Thx for your work. I hope somedays, something will get merged, as it's a very useful feature for 2d guys.

A thing I can't remember, is polygon2d can have holes in godot? Because, how this will get managed if not?

@Xrayez
Copy link
Contributor Author

Xrayez commented May 17, 2019

@TotCac this is the reason I don't use Polygon2D at all. In my project I've come up with Model2D resource that encapsulates boundary and holes as an entity and which could be applied to different 2D objects in theory. In fact I've adapted CSG 3D implementation presented by reduz to work in 2D using Clipper for my project.

That's quite a lot of work to rewrite different parts of engine to incorporate this natively, so I'm so far satisfied with my own specific implementation, so at least something could be exposed and generalized as much as possible to be useful in many use cases.

I also wasn't satisfied with the computational complexity of the underlying triangulation and decomposition libraries in Godot which are quite slow if you want to triangulate a polygon with, for instance, 100 holes in it, and can't handle degenerate input. I'm not really motivated to implement such system too because generalization could negatively impact the performance which I'm not going to trade off in my project.

Clipper 10.0.0 triangulation is quite fast and robust but alas it's in beta/alpha for now, so must stick to 6.4.2.

@Xrayez
Copy link
Contributor Author

Xrayez commented May 20, 2019

This one is unlikely to be merged:

  1. Modified thirdparty code bug fixes provided by the original author that I lost track of.
  2. Clipper 10.0.0 is not stable (alpha/beta now).

Closing as being (hopefully) superseded by #28987 implementation that should be more compact and uses stable version currently used by the engine (6.4.2).

@Xrayez
Copy link
Contributor Author

Xrayez commented May 23, 2019

Note: even though the branch was deleted, most fixes and additional enhancements were translated to yet work-in-progress module with Clipper 10.0.0 under the hood. Once it's stable enough, some features (like robust triangulation) could be used internally and exposed (or even replaced) in Geometry singleton, see #28987. That would mean no more Bad Polygon! errors when drawing polygons with draw_polygon or similar.

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.

None yet

8 participants