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

Replace Clipper1 library by Clipper2 library #90153

Merged
merged 1 commit into from
Apr 22, 2024

Conversation

rburing
Copy link
Member

@rburing rburing commented Apr 2, 2024

Reduces release template binary size (by 54.1 kB on Linux).

To-do:

  • Resolve TODOs in code
  • Use floating point coordinates directly
  • Test more extensively

Fixes #90103

@rburing rburing added this to the 4.x milestone Apr 2, 2024
@smix8
Copy link
Contributor

smix8 commented Apr 2, 2024

You can replace the Clipper XYZ_64 classes with the Clipper D class equivalents. Those do the same except that they allow for direct float input that gets up- and downscaled to the integer grid internally.

So the old Clipper1 manual scaling with SCALE_FACTOR can be removed.

I did a similar upgrade for the 2D navigation mesh baking as an example with PR #89929 here.

The manual Rect clipping in the sprite editor can also be replaced by the "real" RectD Clip of Clipper2 which is far more performant see https://angusj.com/clipper2/Docs/Units/Clipper/Functions/RectClip.htm

@ghmart
Copy link

ghmart commented Apr 2, 2024

What's about #29886?

@smix8
Copy link
Contributor

smix8 commented Apr 3, 2024

What's about #29886?

This PR should imo stay focus on only the upgrade so it can be added without much issues and bike-shed.

Adding new parameters is feature territory as it creates compatibility issues.

It is likely that there is need for a more general rework of the Geometry2D boolean ops later which would be a good place for this and other features. E.g. the single polygon per function limit comes to mind. It is kinda arbitrary as Clipper2 can do way more.

@rburing rburing force-pushed the clipper2_electric_boogaloo branch from 777cd31 to faa9614 Compare April 6, 2024 11:21
@rburing
Copy link
Member Author

rburing commented Apr 6, 2024

@smix8 Thanks for the tips! I used the D variants of Clipper functions/types now, and used RectClip.

I could not remove the SCALE_FACTOR from Geometry2D since the unit tests expect higher precision.

Should we actually be using Point<real_t> instead of PointD etc.?

Edit: I see I should be using the ClipperD constructor with the precision argument.

@rburing rburing marked this pull request as ready for review April 6, 2024 11:32
@rburing rburing requested review from a team as code owners April 6, 2024 11:32
@rburing rburing force-pushed the clipper2_electric_boogaloo branch 4 times, most recently from e7b0d83 to d0e7907 Compare April 6, 2024 12:55
Copy link
Contributor

@smix8 smix8 left a comment

Choose a reason for hiding this comment

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

Looks good for the Clipper2 replacements.

Not really a 2D and sprite editor user so can't say if there are some subtle differences in the results that could cause some compatibility problems for older projects.

@akien-mga akien-mga requested a review from lawnjelly April 7, 2024 08:06
@akien-mga akien-mga modified the milestones: 4.x, 4.3 Apr 7, 2024
@akien-mga akien-mga requested a review from Geometror April 7, 2024 08:06
@KoBeWi
Copy link
Member

KoBeWi commented Apr 8, 2024

I'm using Geometry2D for merging polygons and this change makes my tool a few times slower. Here's a test scene:
GeometryTest.tscn.txt

This was already discussed in the chat, I thought I'd mention it here for the record.

@rburing rburing force-pushed the clipper2_electric_boogaloo branch from d0e7907 to 8a28f81 Compare April 20, 2024 11:49
@rburing
Copy link
Member Author

rburing commented Apr 20, 2024

Here is an image that explains why it was slower:

clipper2-collinear

I have now added

clp.PreserveCollinear(false); // Remove redundant vertices.

to make the resulting polygon the same as it was with Clipper1 (i.e. having just the corner vertices).

Performance is still a bit lower than with Clipper1, but the difference is no longer orders of magnitude.

Edit: In a release build (or a non-dev editor build) there is now no measurable performance difference when running @KoBeWi's project.

@KoBeWi
Copy link
Member

KoBeWi commented Apr 20, 2024

In dev build my code runs 10ms slower on average, but it's acceptable in this case.

@akien-mga akien-mga merged commit fb3c3ac into godotengine:master Apr 22, 2024
16 checks passed
@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.

Geometry2D polygon boolean operations still use old Clipper1 library
6 participants