geo icon indicating copy to clipboard operation
geo copied to clipboard

Panic: could not compare segments

Open hallahan opened this issue 2 years ago • 1 comments

When processing OSM data, sometimes we have bad geometry, and using the library for interior_point causes a panic. Normally, feeding bad geometry to a geometry library should return an error, not a panic, as this is a standard thing to expect.

I have to recover from the panic with panic::catch_unwind, which is not ideal.

https://github.com/georust/geo/blob/3b0d5738f54bd8964f7d1f573bd63dc114587dc4/geo/src/algorithm/sweep/active.rs#L54

[2022-12-04T19:14:40.661064000Z geo::algorithm::sweep::active] could not compare segments:
        Active(Segment{ LPt((444882789.06824327, 332473624.5), (444882817.0, 332473624.5))
        of Line { start: Coordinate { x: 444881047.0, y: 332473624.5 }, end: Coordinate { x: 444882817.0, y: 332473624.5 } }
         [NON/NON] })
        Active(Segment{ LPt((444882200.0, 332474331.0), (444882789.06824327, 332473624.5))
        of Line { start: Coordinate { x: 444882817.0, y: 332473591.0 }, end: Coordinate { x: 444882200.0, y: 332474331.0 } }
        [1st] [NON/NON] })
thread '<unnamed>' panicked at 'unable to compare active segments!', /Users/n/.cargo/registry/src/github.com-1ecc6299db9ec823/geo-0.23.0/src/algorithm/sweep/active.rs:54:13

hallahan avatar Dec 04 '22 20:12 hallahan

The comments on Eq and Ord for Active<T> state that they "Assert total ordering" but they use partial_cmp and thus must do something if the partial_cmp is not total and returns None. This is not ideal and the implementations should require Ord instead of PartialOrd on T or implement partial ordering instead.

softdevca avatar Mar 01 '23 17:03 softdevca