godot icon indicating copy to clipboard operation
godot copied to clipboard

Remove polyline offset duplicates

Open charlieb opened this issue 9 months ago • 4 comments

Fixes #91607 by removing duplicates generated by InflatePaths so that it doesn't cause decompose_polygon_in_convex to fail.

charlieb avatar May 06 '24 17:05 charlieb

Clipper2 has the TrimCollinear and PreserveCollinear properties that should be toggled instead to avoid doing another manual pass over the entire array. See https://angusj.com/clipper2/Docs/Units/Clipper/Functions/TrimCollinear.htm

smix8 avatar May 06 '24 18:05 smix8

The TrimCollinear function is around 50 lines that allows a decimal precision to be used to determine a more approximate equality of position. This inexact equality test is not needed to solve this bug which only manifests if the vertices are exactly the same.

I had a look at the PreserveCollinear property of the ClipperOffset class and it looks like to get access to it we'd have to basically copy-paste the whole InflatePaths interface function to add that setting.

Both of these solutions seem like overkill in the context of this bug and its solution. TrimCollinear in particular is just doing what I'm doing with extra (unnecessary) steps. Replicating InflatePaths also seems to me like adding a bunch of extra code for little benefit.

I'll make a version that copy-pastes InflatePaths and we'll see which is preferred. The existing code already passes across the entire array to copy and cast the vertices back into godot types.

charlieb avatar May 06 '24 19:05 charlieb

Makes sense, thanks for looking into it. Should imo pick whatever solution works better for performance at scale.

Performance regression was one of the concerns with the update https://github.com/godotengine/godot/pull/90153 from Clipper1 to Clipper2 and why certain Clipper2 features were toggled.

I don't expect you to do all the performance testing yourself as a first-time contributor but someone should imo do it so an informed decision can be made what to pick.

Not relevant for this PR but the convex decompose function has general issues with dirty input that includes duplicates that might be worth fixing in a central place on the decompose input, else we end up with x-amount of mostly duplicated fix-me functions across the engine.

smix8 avatar May 06 '24 21:05 smix8

Unrelated but I'm seeing a very large performance degradation when decomposing polygons generated by inflating with END_ROUND set vs v4.2. That's a different issue though.

charlieb avatar May 06 '24 21:05 charlieb

Here's the conclusion of my investigation: When the precision parameter is set to 5 (Godot's default) changing PreserveCollinear has no effect and duplicate points are still generated.

Reducing the precision parameter from Godot's default of 5 to clipper2's default of 2 eliminates duplicate points even when PreserveCollinear is true. It also gives a (subjectively) noticeable performance boost even when using END_ROUND which will freeze my PC at precision 5.

I've created a new, even smaller pull request that implements this.

charlieb avatar May 07 '24 03:05 charlieb