search

georust / geo

Geospatial primitives and algorithms for Rust
https://crates.io/crates/geo
Other
1.54k stars 197 forks source link

Falliable Boolean Operations #1032

Closed RobWalt closed 1 year ago

RobWalt commented 1 year ago

The idea comes from here.

tldr: I think it would be best to resultify every panic that could happen in the BooleanOps trait as a short term solution. This gives geo users the chance to catch and deal with these scenarios (e.g. adjust the geometry slightly and try again) or to ignore them if the results aren't too important.

Some things to discuss

The current state of this PR is merely a PoC for now. There are some things I would like to discuss first

Error handling

Most of the errors are just placeholders for the first iteration of this PR. We can use thiserror to make error handling more robust but I didn't want to go that route yet without any second opinion.

Breaking changes

I tried to avoid breaking changes as much as possible for now. Technically I probably did create some breaking changes which could be ironed out easiliy. I just didn't want to create a copy of all of the existing code parts yet. If you want a really clean solution, we can just create the falliable version of everything completely detached in separate functions from the current state of geo

Todos

rmanoka commented 1 year ago

Since this is a stop-gap until we can implement a more bug-free implementation, could we just catch the panic and error in the try_ versions? Changing the implementation itself would require us to un-change it if we get a different implementation in.

RobWalt commented 1 year ago

Since this is a stop-gap until we can implement a more bug-free implementation, could we just catch the panic and error in the try_ versions? Changing the implementation itself would require us to un-change it if we get a different implementation in.

I'm not entirely sure if I understand you correctly but I'll give it a try to answer you. Feel free to correct me:

catch the panic

I'm using the functions in with a wasm target. I already tried to catch the panics by using the std::panic::catch_unwind function but it still panics anyways. I suspect that is a wasm issue. They even state in the docs of catch_unwind

It is not recommended to use this function for a general try/catch mechanism. The Result type is more appropriate to use for functions that can fail on a regular basis. Additionally, this function is not guaranteed to catch all panics, see the “Notes” section below. ... Note that this function might not catch all panics in Rust. A panic in Rust is not always implemented via unwinding, but can be implemented by aborting the process as well. This function only catches unwinding panics, not those that abort the process. Note that if a custom panic hook has been set, it will be invoked before the panic is caught, before unwinding.

error in the try_ versions

The PR is currently just a PoC draft and if we really want to merge it in I would try to make the implementation more parallel to the existing code, i.e. duplicate existing code and just "resultify" it so it's easier to remove in the future.

rmanoka commented 1 year ago

Oh, good point, I didn't think about the wasm target; the impl. looks on track otherwise . Don't mind copying the algo, we can always do some git churn to revert back components.

Would be good to hear from other reviewers on this.

RobWalt commented 1 year ago

A friend of mine found another major point of failure of the normal BooleanOps algo that I wasn't able to patch yet. In the implementation of Ord for Active<T> there is a potential panic which does occur in the wild.

I'm not really sure how to fix that. We can potentially just make it not panic in the None case there by defaulting to Ordering::Less in this case but I'm not sure what the implications of this are tbh.