Falliable Boolean Operations #1032

RobWalt · 2023-07-10T08:46:49Z

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.

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

un-break intersections iterator
re-export the error type to let users handle them correctly

rmanoka · 2023-07-17T13:57:28Z

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 · 2023-07-18T09:22:48Z

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 · 2023-07-18T15:30:15Z

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.

michaelkirk

I'm on board with the idea of returning runtime errors where users are hitting panics, but I'd like to make sure we're not overly convoluting the code. See my comments on the GeoError enum.

michaelkirk · 2023-07-18T17:06:39Z

geo/Cargo.toml

@@ -40,6 +40,7 @@ rand_distr = "0.4.3"

 ### boolean-ops test deps
 wkt = "0.10.1"
+serde_json = "1.0"


Can you use WKT for your test fixtures? We already have the dependency and IMO it's easier to read as a human.

Yeah that makes total sense. I'll change that 👍

michaelkirk · 2023-07-18T17:08:32Z

geo/src/types.rs

@@ -37,6 +37,16 @@ impl<F: GeoFloat> Closest<F> {
    }
 }

+#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)]
+pub enum GeoError {


Calling this GeoError is a bit aspirational since they are only for boolean ops. Is it actually useful to distinguish between all these cases? (genuine question)

yeah that isn't final yet 😅 I just figured it would be a good start to test it all out.

I'm totally on board with you in creating a more specialized error enum there!

The distinction between all the cases was made so that we would basically still have some rough information in the normal boolops code. Since we're just unwrapping there we will at least see the error enum variant which was chosen to mirror the previous error as close as possible.

michaelkirk · 2023-07-18T17:13:51Z

geo/src/types.rs

+    SegmentAlreadyFoundInActiveSet(usize),
+    ExpectedNonemptyEvents,
+    NotEulierian(usize),
+    Unreachable(&'static str),


Just to put my cards on the table, I think writing panics into our code is reasonable if the programmer truly expects the code to be unreachable. And I think that trying to turn every single panic into a runtime error is probably an anti-pattern.

My preference is that this be only the set of run time errors we're actually encountering. Is it the case that you're hitting all of these?

If we are actually encountering one of the "Unreachable" cases, it's not unreachable. Can you think of a better name that might help people understand what's going on?

Yeah, unfortunately I was able to trigger the unreachable code (see the 3rd case in the spoiler of this comment).

Again, I agree here as well. We should probably pick a better name here. I'll look into that. I wasn't diving too deep into that code yet since I just wanted to have some kind of parity to the existing panicing implementation, but I guess now's a good time for that.

michaelkirk · 2023-07-18T17:15:57Z

geo/src/types.rs

@@ -37,6 +37,16 @@ impl<F: GeoFloat> Closest<F> {
    }
 }

+#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)]
+pub enum GeoError {


I'd guess we want this to be non-exhaustive so we can add to it without breaking peoples code. WDYT?

I'm not really getting what you meant here. Is it showing the right code here?

RobWalt · 2023-07-19T14:16:49Z

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.

michaelkirk · 2023-07-25T22:47:21Z

geo/src/types.rs

@@ -37,6 +37,16 @@ impl<F: GeoFloat> Closest<F> {
    }
 }

+#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)]
+pub enum GeoError {


I'd guess we want this to be non-exhaustive

I mean like this:

Suggested change

pub enum GeoError {

#[non_exhaustive]

pub enum GeoError {

That way, if we need to add new error cases in a subsequent release, we can do so without breaking existing user's code.

RobWalt mentioned this pull request Jul 11, 2023

Panic in BooleanOps::difference for Multipolygon #976

Open

frewsxcv marked this pull request as draft July 16, 2023 19:08

michaelkirk reviewed Jul 18, 2023

View reviewed changes

RobWalt force-pushed the feat/falliable-boolops branch from 51cb2ac to f7e0c2f Compare July 19, 2023 10:15

RobWalt added 17 commits July 20, 2023 09:16

feat: wip - resultify boolop trait

cc2096e

feat: wip - resultify sweep fn

aa473b1

feat: wip - resultify sweep fn (don't break existing code)

c3176f0

feat: wip - resultify finish fn

a7dfeef

feat: wip - resultify interior point trait fn

6ec9e0c

feat: wip - resultify iterator in try_sweep fn

38d6754

feat: wip - resultify handle_event in sweep fn

a8ea43e

feat: wip - resultify index_of fn and co

2d508f7

feat: wip - resultify insert/remove_active fn and co

f7025b9

feat: wip - resultify unreachable code paths

ade26e3

feat: wip - extend boolop trait with more falliable methods

f0ae1b0

feat: wip - rename error variant

1026412

chore: update CHANGES.md

1fef6bb

feat: wip - implement first test + fix some unwraps

d146728

feat: wip - add more test cases

45b35c2

feat: wip - add use case test case

8b9c23a

chore: remove unused impl of ActiveSet trait

810e230

RobWalt force-pushed the feat/falliable-boolops branch from 776367b to 810e230 Compare July 20, 2023 07:17

michaelkirk reviewed Jul 25, 2023

View reviewed changes

RobWalt mentioned this pull request Sep 26, 2023

Draft: feat: implement a safe wrapper for boolops #1073

Closed

7 tasks

RobWalt closed this Sep 29, 2023

wlinna mentioned this pull request Apr 27, 2024

Panic in BooleanOps::union for Polygon<f32> #1103

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Falliable Boolean Operations #1032

Falliable Boolean Operations #1032

RobWalt commented Jul 10, 2023 •

edited

Loading

rmanoka commented Jul 17, 2023

RobWalt commented Jul 18, 2023 •

edited

Loading

rmanoka commented Jul 18, 2023

michaelkirk left a comment

michaelkirk Jul 18, 2023

RobWalt Jul 19, 2023

michaelkirk Jul 18, 2023

RobWalt Jul 19, 2023 •

edited

Loading

michaelkirk Jul 18, 2023 •

edited

Loading

RobWalt Jul 19, 2023

michaelkirk Jul 18, 2023

RobWalt Jul 19, 2023

RobWalt commented Jul 19, 2023

michaelkirk Jul 25, 2023

Falliable Boolean Operations #1032

Falliable Boolean Operations #1032

Conversation

RobWalt commented Jul 10, 2023 • edited Loading

Some things to discuss

Error handling

Breaking changes

Todos

rmanoka commented Jul 17, 2023

RobWalt commented Jul 18, 2023 • edited Loading

rmanoka commented Jul 18, 2023

michaelkirk left a comment

Choose a reason for hiding this comment

michaelkirk Jul 18, 2023

Choose a reason for hiding this comment

RobWalt Jul 19, 2023

Choose a reason for hiding this comment

michaelkirk Jul 18, 2023

Choose a reason for hiding this comment

RobWalt Jul 19, 2023 • edited Loading

Choose a reason for hiding this comment

michaelkirk Jul 18, 2023 • edited Loading

Choose a reason for hiding this comment

RobWalt Jul 19, 2023

Choose a reason for hiding this comment

michaelkirk Jul 18, 2023

Choose a reason for hiding this comment

RobWalt Jul 19, 2023

Choose a reason for hiding this comment

RobWalt commented Jul 19, 2023

michaelkirk Jul 25, 2023

Choose a reason for hiding this comment

RobWalt commented Jul 10, 2023 •

edited

Loading

RobWalt commented Jul 18, 2023 •

edited

Loading

RobWalt Jul 19, 2023 •

edited

Loading

michaelkirk Jul 18, 2023 •

edited

Loading