Panic due to integer overflow in Line::delta logic, and thus Line::slope

[jayvdb]

The docstrings dont clarify whether Line::delta should be absolute or not. Either way, it should not panic.

let coord_1 = geo_types::Coord::<u64> { x: 4188713892826324325, y: 13985072956178380357 };
let coord_2 =  geo_types::Coord::<u64> { x: 9749381878712502956, y: 1128913892127837453 };
let line = geo_types::Line::new(coord_1, coord_2);
let delta = line.delta(); // broken
let _ = line.slope(); // also fails

results in

thread 'impls::geo::tests::test_triangle' panicked at 'attempt to subtract with overflow', /rustc/2c8cc343237b8f7d5a3c3703e3a87f2eb2c54a74/library/core/src/ops/arith.rs:219:1
stack backtrace:
   0: rust_begin_unwind
             at /rustc/2c8cc343237b8f7d5a3c3703e3a87f2eb2c54a74/library/std/src/panicking.rs:575:5
   1: core::panicking::panic_fmt
             at /rustc/2c8cc343237b8f7d5a3c3703e3a87f2eb2c54a74/library/core/src/panicking.rs:64:14
   2: core::panicking::panic
             at /rustc/2c8cc343237b8f7d5a3c3703e3a87f2eb2c54a74/library/core/src/panicking.rs:114:5
   3: <u64 as core::ops::arith::Sub>::sub
             at /rustc/2c8cc343237b8f7d5a3c3703e3a87f2eb2c54a74/library/core/src/ops/arith.rs:212:45
   4: <geo_types::geometry::coord::Coord<T> as core::ops::arith::Sub>::sub
             at /home/jayvdb/.cargo/registry/src/github.com-1ecc6299db9ec823/geo-types-0.7.9/src/geometry/coord.rs:180:16
   5: geo_types::geometry::line::Line<T>::delta
             at /home/jayvdb/.cargo/registry/src/github.com-1ecc6299db9ec823/geo-types-0.7.9/src/geometry/line.rs:44:9
   6: tests::test_triangle
             at ./src/impls/geo/mod.rs:131:21
   7: tests::test_triangle::{{closure}}
             at ./src/impls/geo/mod.rs:124:24

I note that the "Add, Div, Mul, Sub" operations in Coord look like they will also cause overflows for integer types. Looks like that is unavoidable due to their definitions in std::ops.

[frewsxcv]

We can introduce checked_delta and checked_slope methods that mirror the checked_* methods on integers in the standard library. Would that help @jayvdb?

[jayvdb]

tbh, what I need is a way to get an accurate slope, in order to determine if three coords are a triangle. Needed for https://github.com/cksac/fake-rs/pull/130 i.e. use f64 even in the T is an unsigned int.

Could I contribute a Line::abs_slope ? e.g. something like

fn abs_slope<T>(a: geo_types::Coord<T>, b: geo_types::Coord<T>) -> f64
where
    T: CoordNum,
{
    let (min_x, max_x) = if a.x > b.x { (b.x, a.x) } else { (a.x, b.x) };
    let (min_y, max_y) = if a.y > b.y { (b.y, a.y) } else { (a.y, b.y) };
    let delta_x: f64 = cast(max_x - min_x).unwrap();
    let delta_y: f64 = cast(max_y - min_y).unwrap();
    delta_y / delta_x
}

[urschrei]

determine if three coords are a triangle

Any three points that aren't collinear form a triangle, right? You could use https://docs.rs/robust/0.2.3/robust/fn.orient2d.html and check for a non-zero value.

Or if you prefer, you can access robust's orient2d function via geo: https://docs.rs/geo/latest/geo/algorithm/kernels/trait.Kernel.html#method.orient2d

[jayvdb]

Seems odd the geo-types package doesnt have a way to determine of a geo_types::Triangle is a triangle. I'd rather not have another dependency just for that.

It looks like orient2d is going to be a perf hit - its algorithm does lots of additional work when the points are approaching collinear. In my case that isnt desirable as in my use-case it is desirable to discard the points when they are cheaply determined to be approximately collinear, as it is less work to pick a different random point to construct the triangle.

[urschrei]

Seems odd the geo-types package doesnt have a way to determine of a geo_types::Triangle is a triangle. I'd rather not have another dependency just for that.

geo-types provides types, not algorithms. The robust crate is minimal, and is provided as a separate crate because its predicates are widely useful to users outside the geo ecosystem.

its algorithm does lots of additional work when the points are approaching collinear

I would characterise it differently: Jonathan Shewchuk's incredibly widely-used algorithm provides an accurate answer to a difficult problem, trading performance for correctness when appropriate.

You say it "looks like" it's going to be a perf hit. Have you determined this empirically?

As a more general comment, I'm not inclined to accept a PR for less robust functionality already provided by a crate we maintain, though it's not solely up to me.

[jayvdb]

geo-types provides algorithms like slope. I am asking to have an 'absolute slope' added.

Have you determined this empirically?

No I havent. Will do.