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

Update PolyPartition / Triangulator library #45125

Merged
merged 1 commit into from
Jan 12, 2021

Conversation

aaronfranke
Copy link
Member

@aaronfranke aaronfranke commented Jan 12, 2021

This PR updates the PolyPartition library (previously the files were called "triangulator") to commit 7bdffb4.

The only difference between the version in the repo and the version in this PR is that this PR uses Godot's types (List, Set, Vector, Vector2) instead of the standard library (std::list, std::set, std::vector) and PolyPartition's TPPLPoint.

@akien-mga
Copy link
Member

The only difference between the version in the repo and the version in this PR is that this PR uses Godot's types (List, Set, Vector, Vector2) instead of the standard library (std::list, std::set, std::vector) and PolyPartition's TPPLPoint.

Why? We shouldn't modify thirdparty code if we don't have to, and that's a very intrusive change to thirdparty code, making it difficult to do any future update.

@aaronfranke
Copy link
Member Author

@akien-mga Well, because the existing version in Godot was using Godot's types, so I kept that part the same.

It should be easy to change it back to using the standard library by just copying the upstream version again, though in the case of TPPLPoint I think it makes sense to keep using Godot's Vector2 since it's a drop-in replacement with one typedef.

@akien-mga
Copy link
Member

I see. Then I guess it's fine for this PR, but yeah I think a follow-up could try to undo all these Godot-specific changes and use the unmodified thirdparty code. If we need to write a Vector2 wrapper around TPPLPoint that can be done too.

Could you update thirdparty/README.md with the rename and the actual version documentation, now that it's known?

@akien-mga
Copy link
Member

Could you also add a .patch file that documents the changes made to change from STL to Godot types? You can do it by backing up your modified files, adding the unmodified upstream code on top, git add it, then re-copy your modified files on top and git diff > thirdparty/misc/patches/polypartition-godot-types.patch.

@aaronfranke
Copy link
Member Author

I will do that, keeping a .patch file is a good idea.

I looked into what it would take to keep std::. We would have to update the Godot code that depends on this to use the standard library, and add conversions to and from Godot types for the input and output of methods such as Geometry2D::decompose_polygon_in_convex, so I think that's not a good idea.

@Xrayez
Copy link
Contributor

Xrayez commented Jan 12, 2021

In Goost, I've exposed quite a bunch of hidden PolyPartition functionality, and discovered that there's actually a hidden bug in Triangulate_MONO (with Godot types): https://github.com/goostengine/goost/blob/c5ef6734b87057f0d73ca0fcbd6c3a50c862480c/core/math/2d/geometry/poly/decomp/polypartition/poly_decomp_polypartition.cpp#L94-L98

This is also something which #16423 proposed to enable over ear-clipping which is currently the default (would mostly avoid "Bad Polygon!" errors while drawing polygons).

So, I'm just saying that I'd be careful modifying thirdparty code, and yeah I'd certainly prefer to undo those Godot-specific changes because there's a high chance of breaking things that way, especially when the library is updated.

add conversions to and from Godot types for the input and output of methods such as Geometry2D::decompose_polygon_in_convex, so I think that's not a good idea.

That's how Clipper library is currently used in Godot for polygon boolean methods (mainly due to the way how points needs to be scaled up and down to achieve robust computations), and it's quite fast even with conversion going on.

This is something which can be done via dedicated PR though.

@aaronfranke
Copy link
Member Author

@Xrayez If you would like to add onto this PR, that would be nice too. It would result in fewer diffs than if this PR was accepted as it is now and then these files were changed and the patch deleted.

@Xrayez
Copy link
Contributor

Xrayez commented Jan 12, 2021

I'd certainly work on this but unfortunately I'm not using Godot 4.0 currently so it seems like such a low-priority task to be honest... I think it's fine to have those Godot types for now. Perhaps this update may fix the crash with Triangulate_MONO and this wouldn't be critical any longer (so it might not be due to Godot types used but a bug in PolyPartition itself...). It's just something to be ever of.

thirdparty/README.md Outdated Show resolved Hide resolved
@akien-mga akien-mga merged commit d214869 into godotengine:master Jan 12, 2021
@akien-mga
Copy link
Member

Thanks!

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.

3 participants