Panic in BooleanOps::union for Polygon<f32> about geo HOT 6 OPEN

@urschrei added a test for this in bd5f57a. I added both variations (f32 and f64) and they both succeeded.

from geo.

@RobWalt Could you add this to your draft Bool ops PR as a test case?

from geo.

I get lots of panics with Polygon<f64>::union. It always seems to be one of these:

• segment not found in active-vec-set: 2
• assembly segments must be eulierian

What I'm doing is that I'm trying to turn a triangle mesh into a MultiPolygon by calling union sequentially.

I'm using geo 0.28.

I think turning boolean_op into a fallible function would be a good solution until all the issues have been ironed out. It seems that something like that has been attempted in at least two PRs before, and neither of them were merged

1. #1032
2. #1073

If I understand correctly, the first one was closed because the same author opened another PR with a different idea. But since that didn't pan out (?), why not reopen the first one? The panics are easy to hit and it'd be so much better if we could recover from errors.

EDIT: I've notice that there's also a Stitch PR and Spade Boolops PR

from geo.

I think turning boolean_op into a fallible function would be a good solution until all the issues have been ironed out.

Yeah I thought so as well. The main problem with the current implementation, panicking implementation of boolops is, that it relies on an implementation of PartialOrd. Within this traits implementation there is panicking code. We don't really have control over this since partial_cmp has an un-generic function signature which doesn't allow falliability https://doc.rust-lang.org/stable/std/cmp/trait.PartialOrd.html#tymethod.partial_cmp . It might still be possible to fix this by rolling our own PartialOrd but no-one wanted to touch that can of worm yet I guess.

I'm still working on the alternative implementation of boolops which should work without panics but it'll still take a while since we're currently still stuck reviewing the stitch PR.

from geo.

Hmm.. but even if we those parts that depend on PartialOrd, wouldn't it help quite a lot of if code like this

            debug_assert!(num_segments % 2 == 0, "assembly segments must be eulierian");

and this

    pub fn index_of(&self, segment: &T) -> usize {
        self.data
            .binary_search(Active::active_ref(segment))
            .expect("segment not found in active-vec-set")
}

.. returned Errors or Options instead?

Btw, I switched to SpadeBoolops::union and so far it seems to work well enough.

from geo.

Oh I just noticed I didn't press send here. Sorry for the wait.

Indeed I did try out to use errors in every scenario possible before. The thing is that during testing it didn't really make a difference since the panic in the PartialOrd was hit anyways.

from geo.