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

change the boolops trait to return result #1233

Closed
wants to merge 2 commits into from

Conversation

TotalKrill
Copy link

@TotalKrill TotalKrill commented Oct 24, 2024

This would enable us to start working on removing the crashes due to ".unwrap" and instead let a user handle them gracefully in the code

I intentionally did not solve any actual crashes as of now, more checking if this is a direction that is likely to be implemented

  • I agree to follow the project's code of conduct.
  • I added an entry to CHANGES.md if knowledge of this change could be valuable to users.

@michaelkirk
Copy link
Member

michaelkirk commented Oct 24, 2024

We've had some long standing crashing issues with our boolean ops, but I'd prefer to sit on this for just a bit longer (I'm talking days, not weeks or months).

I'm exploring integrating i_overlay as a replacement engine for our boolean operations and it's going quite well.

My understanding is that i_overlay is intended to be infallible, so it'd be a shame to introduce a fallible API if we can avoid it.

For those not following on discord, the crash (really just a debug assert) that elicited this PR has been resolved in i_overlay just now.
iShape-Rust/iOverlay#10 (comment)

@michaelkirk
Copy link
Member

I intentionally did not solve any actual crashes as of now, more checking if this is a direction that is likely to be implemented

In any case, there's no sense in returning a result until an actual crash is addressed, so I'm going to mark this as a draft for now.

@michaelkirk michaelkirk marked this pull request as draft October 24, 2024 18:03
@lnicola
Copy link
Member

lnicola commented Oct 24, 2024

I don't think it's worth changing the signature of that method just because of an implementation bug. Users can always catch_unwind as a workaround.

@TotalKrill
Copy link
Author

Looking through the code, there are far less unwraps in i_overlay code, compared to the code currently in geo. And I have not managed to break that code either.

I dont think that suggesting catch_unwind is a very user-friendly strategy, especially not in Rust, that actually has solutions for handling code that can fail, such as code that is liberally sprinkled with unwraps...

With that being said, im not sure if this way is actually needed if i_overlay solves all crashes people are experiencing. from what I can tell from other users on the bevy discord. It does seem to be the case.

@TotalKrill TotalKrill closed this Oct 24, 2024
@michaelkirk
Copy link
Member

See #1234

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.

3 participants