introduce Coord/Point trait

[michaelkirk]

One way to dip our toe into #838 would be a coord/point trait.

I've done a little POC here: https://github.com/georust/geo/tree/mkirk/affine-coord-trait

I did this as a means of integrating the new AffineOps into ABStreet here: https://github.com/a-b-street/abstreet/compare/master...michaelkirk:abstreet:mkirk/apply-affine?expand=1#diff-b2aff24a689eccbe772abd984783d3a508924c1e7d197cee9ba8cede6a36ee89R124

Disclaimer: I don't know that @dabreegster would actually wants to use this integration in ABStreet, I was just looking for a real-world application to test-bed it in.

One reason to use a Coord/Point trait as a starting point for trait-based geoms in geo is that there are already places internally that could immediately benefit, even if this crate never goes whole hog on trait-based geometries.

For example, there's a bunch of places in our API's where it's not clear wether we should pass in a Point or a Coordinate. Using a trait that encompasses both would clean that up, while also allowing people to pass in their own point type.

(maybe ultimately we should resurrect #15, but that could be pretty disruptive)

[rmanoka]

Is the std. AsRef trait not useful here? The first example specifically talks about accepting things that can be internally converted so. I guess the concrete geo-types structs are needed anyway as an anchor for all our ser/de crates (wkt, geojson, etc). Just trying to clarify, I don't mind the new traits per se.

[dabreegster]

The demo integration in A/B Street is very nice, I'd love to see this go forward! I do want to raise one possible design consideration. It's not relevant for geom::Tessellation in your integration, but if we tried to apply it to geom::Polygon later, it would come up.

What if the trait implementer wants to control the scalar values in the point more tightly? Specifically, something like rounding an f64 to a certain number of decimal places. With from_xy that could be done, but x_mut and y_mut mean code inside of geo can modify the caller's internal representation, with no opportunity to do that rounding afterwards (except by the caller themselves calling an "internal cleanup" method after calling a geo operation).

This might be a niche/weird use case not worth considering, but still wanted to bring it up

[michaelkirk]

What if the trait implementer wants to control the scalar values in the point more tightly? Specifically, something like rounding an f64 to a certain number of decimal places. With from_xy that could be done, but x_mut and y_mut mean code inside of geo can modify the caller's internal representation, with no opportunity to do that rounding afterwards (except by the caller themselves calling an "internal cleanup" method after calling a geo operation).

Yeah that would be tricky.

In practice is this precision model something that you maintain through all your intermediate calculations? IIRC it was only used in order to have a smaller serialization of game state. So would it be reasonable to only truncate the precision then during serialization?

I think conceptually what you have there is no longer really an f64 as it has extra semantics (e.g. "do f64 division and then round the result").

Another option would be to to formalize those semantics with your own GeoFloat, which would be backed by something like i32. Some of the intermediate computations you're doing with these numbers might be slower, so I'm not sure if it's feasible.

e.g.

struct MyFloat(i32);
impl GeoFloat for MyFloat { 
    fn to_f64(&self) -> f64 {
       // or whatever your precision model is
        self.0 as f64 / 10000.0
    }
    fn from_f64(v: f64) -> Self {
       // or whatever your precision model is
       Self((v * 10000.0) as i32)
    }
}

impl Div for MyFloat {
    fn div(other: Self) -> Self {
        // just an example of an implementation, you may want something different
        MyFloat(self.0 / other.0)
    }
}

let point: geo::Point<MyFloat> = geo::Point::new(MyFloat::from_f64(1.0), MyFloat::from_f64(2.0))

[dabreegster]

In practice is this precision model something that you maintain through all your intermediate calculations? IIRC it was only used in order to have a smaller serialization of game state. So would it be reasonable to only truncate the precision then during serialization?

The chain of reasoning was:

1. If we serialize an f64 with serde and then read it back in, sometimes the result is slightly different
2. Some workflows might import a map in-memory and immediately run a simulation. Others serialize the map, read it back in, and then run a simulation. The slight differences in f64s cause different simulation outcomes, and that needs to be totally deterministic (for debugging sanity and for meaningfully running A/B tests and isolating the effects of deliberately changing one variable).
3. We don't need all the precision anyway. So just round to some decimal places. Also save serialization space by encoding as an i32, since the rounding restricts to that range anyway.

Thinking back more, another reason to round is to avoid epsilon comparisons everywhere. Due to floating point error, two operations on a line-string might return a distance along of it of 1.2 and 1.200000000001. I want PartialEq to treat those the same and not have to remember to do epsilon checks everywhere. Rounding takes care of that nicely. I guess the alternative would be to write custom PartialEq implementations that have the epsilon comparion though.

If we just did this rounding when we serialize, then the two workflows in 2) would still differ. As soon as we create some geometry object, we need to round. The rounding right now happens inside of geom::Pt2D, geom::Distance, etc. Some operations might internally use a full regular f64 for some math, but they return one of these structures and wind up rounding.

The GeoFloat approach makes sense to me, but I agree it could be a perf hit, since CPUs are optimized for floating point ops anyway. An alternative after calling a geo op directly on Pt2D would be to call a cleanup routine that does the rounding. If we did this integration for affine ops, that approach would be fine -- we could do it at the end of affine_transform_mut for example.

I don't want to distract from this PR anymore. The example integration would need some tweaking, but I like it. If anyone has prior experience or opinions about how to deal with f64 precision and not worry about epsilon checks and serialization idempotency, I'd love to follow a more well-thought-out approach!

[urschrei]

@dabreegster Is this round-trip rounding issue present when you use serde with features = ["float_roundtrip"]?

[dabreegster]

@dabreegster Is this round-trip rounding issue present when you use serde with features = ["float_roundtrip"]?

Ah, seemingly not! This feature (for JSON output -- bincode is fine already) might not've been around when I last dug into this. I might consider trying this + baking in epsilon checks to PartialEq instead of weird rounding. That might be a much less strange way of doing things.

[urschrei]

We should probably have a look at enabling that feature in geojson, although there is a perf cost: https://github.com/serde-rs/json/blob/6346bb30037f31ea69b9a4eb029875e23521681c/Cargo.toml#L60-L62

[kylebarron]

I hadn't seen this 🙂. This does raise a question of whether geometry traits should offer mutable methods