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

Remove polyline offset duplicates #91627

Closed

Conversation

charlieb
Copy link

@charlieb charlieb commented May 6, 2024

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

@charlieb charlieb requested a review from a team as a code owner May 6, 2024 17:05
@AThousandShips AThousandShips changed the title Remove polyline offset dupes Remove polyline offset duplicates May 6, 2024
@AThousandShips AThousandShips added this to the 4.3 milestone May 6, 2024
@smix8
Copy link
Contributor

smix8 commented May 6, 2024

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

@charlieb
Copy link
Author

charlieb commented May 6, 2024

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.

@smix8
Copy link
Contributor

smix8 commented May 6, 2024

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

@charlieb
Copy link
Author

charlieb commented May 6, 2024

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
Copy link
Author

charlieb commented May 7, 2024

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 charlieb closed this May 7, 2024
@akien-mga akien-mga removed this from the 4.3 milestone May 7, 2024
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.

Convex decomposition fails for offset_polyline of simple Curve2D
4 participants