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

Aagu/replace rust-geo-booleanop with geoclipper #41

Conversation

AnAverageGitUser
Copy link

rust-geo-booleanop seems to have an unfixed bug ( 21re/rust-geo-booleanop#17 ). One possibility might be to switch to a similar library e.g. (Clipper2, https://angusj.com/clipper2/Docs/Overview.htm).

I ran into this while using bevy_pathmesh as one task thread kept on crashing after my application ran X seconds.

Note 1

The Clipper2 library seems to work on integer grid points, therefore one has to choose a precision for the coordinates that one is comfortable with. In this PR I chose 3 digits behind the decimal point.

Note 2

One of the tests is_in_mesh_overlapping_simplified failed initially, because the amount of polygons was not reduced from 16 to 8, but was stable from 8 to 8 after the update. The function merge_overlapping_obstacles now returns 8 polygons instead of the 16 previous. I haven't looked into this further, might be an issue.

@AnAverageGitUser AnAverageGitUser changed the title Aagu/replace rust geo booleanop with geo clipper Aagu/replace rust-geo-booleanop with geoclipper Jan 14, 2024
@mockersf mockersf changed the base branch from main to radius-baking January 17, 2024 05:15
@mockersf
Copy link
Member

mockersf commented Jan 17, 2024

Thanks!

This bug is actually the reason I never merged that branch, it was too unstable 🙁
I also tried geo-clipper, but it's not working in wasm. It's much faster though!

What I wanted to do at the time but got busy with other things (and also waiting for lelongg/geo-clipper#23 to be released) was to use geo-clipper in native, and geo-booleanop in wasm. Could you add that to your PR?

@mockersf
Copy link
Member

mockersf commented Jan 17, 2024

oh lelongg/geo-offset#56 got merged but not yet released, this would be needed for the radius-baking branch to be merged to main too...

…compatible"

the wasm-compatible variant includes the lib geo-booleanop that is currently in some cases not reliable
the wasm-incompatible variant includes the lib geo-clipper seems to be more reliable, faster but not wasm-compatible
@AnAverageGitUser
Copy link
Author

I've used 2 mutually exclusive feature-flags, because I couldn't find a way to use a "not(feature = "x")" in the Cargo.toml. Otherwise the two libs couldn't both be optional.

Btw, thanks for all the work that you put into the bevy ecosystem. You're doing quite a lot.

@AnAverageGitUser
Copy link
Author

@mockersf Do you have outstanding requests for this pull request or is it fine as is?

@mockersf
Copy link
Member

mockersf commented Mar 9, 2024

Sorry, I'm coming back to this now.

I was hoping some crates would have been published by now, I'll try to move forward. but this can be merged anyway, thanks!

@mockersf mockersf merged commit 0cd74fa into vleue:radius-baking Mar 9, 2024
4 checks passed
mockersf pushed a commit that referenced this pull request Mar 9, 2024
* swapped out geo-booleanop through geo-clipper, because of issue 21re/rust-geo-booleanop#17

* made clippy happy

* added mutually exclusive feature-flags "wasm-compatible" and "wasm-incompatible"
the wasm-compatible variant includes the lib geo-booleanop that is currently in some cases not reliable
the wasm-incompatible variant includes the lib geo-clipper seems to be more reliable, faster but not wasm-compatible

* cargo fmt
@AnAverageGitUser AnAverageGitUser deleted the aagu/replace-rust-geo-booleanop-with-geo-clipper branch March 21, 2024 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants