Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce trait for accessing geometries #67

Closed
pka opened this issue Sep 18, 2016 · 27 comments
Closed

Introduce trait for accessing geometries #67

pka opened this issue Sep 18, 2016 · 27 comments

Comments

@pka
Copy link
Member

pka commented Sep 18, 2016

In the course of refactoring rust-postgis to use rust-geo geometries, I'm looking at needed conversions between different georust geometries. One goal would be to store a rust-gdal geometry with rust-postgis as efficient as possible.

One possible solution would be using iterators returning a Point trait instead of accessing the structs directly. If rust-gdal would implement this trait, storing a geometry with rust-postgis could be done without any conversion. And if the geo algorithms would use this trait, they could be applied directly on geometries implementing this trait. It should also solve the ownership problem in #21.

A simplified trait could be

pub trait PointType {
    fn x(&self) -> f64;
    fn y(&self) -> f64;
    //...
}

When this trait is implemented for geo::Point and geo::LineString implements IntoIterator with Item=&PointType, you can iterate like

let xsum = linestring.into_iter().fold(0., |sum, p| sum + p.x());
@frewsxcv
Copy link
Member

frewsxcv commented Sep 22, 2016

Sorry, didn't see this until today. I'll need a day or two to think this through but I think this is a good idea. I'm not sure how this might work for something like LineStringType. I'll try you respond again tomorrow after I've played around with this idea.

@pka
Copy link
Member Author

pka commented Sep 22, 2016

My current experiments look like this:

pub trait PointType {
    fn x(&self) -> f64;
    fn y(&self) -> f64;
    //...
}

pub trait LineStringType<'a> {
    fn points(self) -> PointRefIter<'a>;
}

pub trait MultiLineStringType<'a> {
    fn lines(self) -> LineStringRefIter<'a>;
}

I got the point iterator working, but not yet the line iterator.

let sumx = linestring.points().fold(0., |sum, p| sum + p.x());

@frewsxcv
Copy link
Member

I played around with this idea today: https://is.gd/tqkzBp

I think this is a good idea. If you have any feedback for my link above, let me know. Commented out section was something I tried, but I couldn't get the typechecker to behave. Will have to do some more thinking.

@pka
Copy link
Member Author

pka commented Sep 30, 2016

I made some progress implementing the following traits:

pub trait Point {
    fn x(&self) -> f64;
    fn y(&self) -> f64;
    //...
}

/// Iterator for points of a line
pub struct Points<'a, T: 'a + Point>
{
    pub iter: Iter<'a, T>
}

pub trait LineString<'a> {
    type PointType: Point;

    fn points(&'a self) -> Points<'a, Self::PointType>;
}

/// Iterator for lines of multi-lines
pub struct LineStrings<'a, T: 'a + LineString<'a>>
{
    pub iter: Iter<'a, T>
}

pub trait MultiLineString<'a> {
    type LineStringType: LineString<'a>;

    fn lines(self) -> LineStrings<'a, Self::LineStringType>;
}

Full code is here

@pka
Copy link
Member Author

pka commented Oct 13, 2016

Nicer traits which don't depend on std::slice::Iter

pub trait Point {
    fn x(&self) -> f64;
    fn y(&self) -> f64;
    //...
}

/// Iterator for points of line or multi-point geometry
pub trait Points<'a> {
    type ItemType: 'a + Point;
    type Iter: Iterator<Item=&'a Self::ItemType>;
    fn points(&'a self) -> Self::Iter;
}

pub trait LineString<'a>: Points<'a> {
}

/// Iterator for lines of multi-lines
pub trait Lines<'a> {
    type ItemType: 'a + LineString<'a>;
    type Iter: Iterator<Item=&'a Self::ItemType>;
    fn lines(&'a self) -> Self::Iter;
}

pub trait MultiLineString<'a>: Lines<'a> {
}

Full code on playground

@frewsxcv
Copy link
Member

@pka I really appreciate the effort you're putting into this! ✨ In my opinion, I do think this is the right path for rust-geo; supplying a set of traits like these matches Rust's principle of "abstraction without overhead".

Even though I am interested in helping out with this, I'm writing just to inform that I probably won't be able to make assist in any progress with this specific issue over the next couple weeks (busy week with work and preparing a workshop for a conference). Just thought I'd mention instead of just leaving you hanging.

@pka
Copy link
Member Author

pka commented Oct 26, 2016

I've ended up with the following traits:

pub trait Point {
    fn x(&self) -> f64;
    fn y(&self) -> f64;
    fn opt_z(&self) -> Option<f64> {
        None
    }
    fn opt_m(&self) -> Option<f64> {
        None
    }
}

pub trait LineString<'a> {
    type ItemType: 'a + Point;
    type Iter: Iterator<Item=&'a Self::ItemType>;
    fn points(&'a self) -> Self::Iter;
}

pub trait Polygon<'a> {
    type ItemType: 'a + LineString<'a>;
    type Iter: Iterator<Item=&'a Self::ItemType>;
    fn rings(&'a self) -> Self::Iter;
}

pub trait MultiPoint<'a> {
    type ItemType: 'a + Point;
    type Iter: Iterator<Item=&'a Self::ItemType>;
    fn points(&'a self) -> Self::Iter;
}

pub trait MultiLineString<'a> {
    type ItemType: 'a + LineString<'a>;
    type Iter: Iterator<Item=&'a Self::ItemType>;
    fn lines(&'a self) -> Self::Iter;
}

pub trait MultiPolygon<'a> {
    type ItemType: 'a + Polygon<'a>;
    type Iter: Iterator<Item=&'a Self::ItemType>;
    fn polygons(&'a self) -> Self::Iter;
}

They are implemented in rust-postgis allowing to store any geometry type implementing these traits.

@jdroenner
Copy link
Member

jdroenner commented Oct 27, 2016

I really like the idea of having traits for accessing geometries :)
Is it really necessary to have opt_m and opt_z ? And will they always be f64? As Point is a trait could this be handled by generics?

@pka
Copy link
Member Author

pka commented Oct 28, 2016

Hi @jdroenner

  • With XYZM attributes the OGC SF standard is covered. In rust-postgis opt_m and opt_z are needed to store e.g. 3D points in the DB
  • My point in concentrating on f64 was simplicity. If you want to implement algorithms like an intersection, you have to solve the numerical problems only for one numeric type. You can still implement the trait for f32 or even i32 points (e.g. screen coordinates like in MB vector tiles), but all operations are done with f64.
    It was difficult enough (at least for me) to find traits which are implementable for rust-postgis and rust-gdal, so I didn't try to make them generic.

@jdroenner
Copy link
Member

jdroenner commented Oct 29, 2016

Hi @pka
i understand that it is not easy to find a generic trait which fits all needs. Maybe we could split the Point into multiple traits? Something like this:

pub trait Point2D {
    fn x(&self) -> f64;
    fn y(&self) -> f64;
}

pub trait Point3D : Point2D {
    fn z(&self) -> Option<f64> {
        None
    }
}

pub trait Measurement<M> {
    fn m(&self) -> Option<M> {
        None
    }
}

pub trait LineString<'a> {
    type ItemType: 'a + Point2D;
    type Iter: Iterator<Item=&'a Self::ItemType>;
    fn points(&'a self) -> Self::Iter;
}

This still allows to have XYZM Points in rust-postgis e.g. pub trait PointXYZM: Point3D + Measurement<f64> {} but is more explicit. One would know what kind of Point to expect...
Maybe this could also be usefull to implement algorithms for 2D and 3D cases.

@pka
Copy link
Member Author

pka commented Oct 29, 2016

Looks nice for points, but I don't see how you could access Z values of 3D line points and I'm afraid that I had to write separate WKB conversion routines for each point type.

@jdroenner
Copy link
Member

I guess you are right. If we treat Point2D and Point3D as different types than some things have to be handled separate.

Accessing the z values of 3D line points shouldn't be a problem. If you have something like this:

impl Point2D for (f64, f64, f64) {
    fn x(&self) -> f64 {
        self.0
    }
    fn y(&self) -> f64 {
        self.1
    }
}

impl Point3D for (f64, f64, f64) {
    fn z(&self) -> Option<f64> {
        Some(self.2)
    }
}

impl <'a, T> LineString<'a> for Vec<T> where T: 'a + Point2D {
    type ItemType = T;
    type Iter = std::slice::Iter<'a, Self::ItemType>;
    fn points(&'a self) -> Self::Iter {
        self.iter()
    }
}

A line could be created like this: let line = vec!((1.0, 2.0, 3.0), (4.0, 5.0, 6.0));. This allows to use the entries as Point3D and provides access to the Z values.

@pka
Copy link
Member Author

pka commented Oct 30, 2016

I'm with you, that these traits are implementable for 3D point types. But wouldn't we need additional line traits like

pub trait LineString3D<'a> {
    type ItemType: 'a + Point3D;
    type Iter: Iterator<Item=&'a Self::ItemType>;
    fn points(&'a self) -> Self::Iter;
}

for accessing Z values?
In your LineString implementation ItemType has type Point2D, so the compiler won't let us call line.points().last().unwrap().z()
Don't get me wrong. I'm all for more elegant traits, but I'm still sceptical whether there is a practical implementation.

@jdroenner
Copy link
Member

We can use the associated type ItemType of the LineString trait to define the type of the points:

pub trait LineString<'a> {
    type ItemType: 'a + Point2D;
    type Iter: Iterator<Item=&'a Self::ItemType>;
    fn points(&'a self) -> Self::Iter;
}

pub fn do_something<'a, L>(line: &'a L)  -> f64 where L: LineString<'a>, L::ItemType: Point3D {
    line.points().last().unwrap().z().unwrap()
}

@frewsxcv
Copy link
Member

frewsxcv commented Nov 1, 2016

My main concern with offering opt_z (and opt_m) is that users might expect rust-geo's set of algorithms to consider these extra dimensions when performing the calculations (e.g. three dimensional XYZ distance instead of two dimensional XY distance). I haven't thought too hard about the Z and M dimensions; I don't normally use them so I'm neutral on the matter. If necessary, we can ship with just x() and y() and always extend the traits in the future to support z and m.


Taking a step back and looking at the bigger picture here, I'm still convinced this is the right way to go with this crate. Similar to rust-num, which offers abstract numeric traits with numeric operations, rust-geo will offer abstract geographic traits with geographic operations, with no extra abstractions.

My last hesitation here is UX related. If we transition this entire crate to be trait based, this will require the user of rust-geo to have concrete structures (enum or struct) that implement the various traits that rust-geo will offer. Is there any value in rust-geo having some of these concrete structures inside rust-geo alongside the traits? For example, if we have a pub trait Point, we'll also have a basic pub struct ConcretePoint which implements the trait Point. Right now, I'm leaning towards not offering these associated concrete structures.

@groteworld
Copy link
Member

groteworld commented Nov 1, 2016

To me I feel like the structs would be offered by surrounding format libraries (wkt, gdal, geojson, etc.). Yet, there may be a lot of overlap there. I agree for now, a common struct is not absolutely necessary.

Great work @pka

@jdroenner
Copy link
Member

Maybe we can create a "basic-geometries" crate at some point which a format library can use if it doesn't need a special implementation...

Just to be clear: i really like the traits @pka created :)
@frewsxcv what do you think about having explicit Point2D and Point3D traits? This way all 2D Methods are usable with 3D objects but it should be clear that only methods for Point3D or with ItemType: Point3D can consider the Z coordinate.

Also what do you (all) think about a Measurement or Payload trait? One of the advantages of such a trait could be to also have Polygons or LineStrings with a payload. GeoJson objects usually have a properties map which could be accessible via such a trait.

@pka
Copy link
Member Author

pka commented Nov 1, 2016

@jdroenner your proposed solution using an associated ItemType could solve my reservations. I still think that I have to implement the "to_ewkb" methods for each point trait, which could end up in a lot of code.

Concrete structures in rust-geo: A next step would be to define geo factory traits. A rust-geo method like intersection would use these traits to generate the output geometry. Without factory traits, rust-geo needs concrete structures to produce results.

XYZM handling in rust-geo: In a trait based rust-geo I would at least expect that geographic operations keep my ZM values whenever possible.

@jdroenner
Copy link
Member

@pka maybe we can find a solution for to_ewkb that doesn't require to much code. Would it be possible to combine the de/encoding with rust-wkt and then reuse common parts?

Regarding the factory traits: I think @pka is right about this but i'm not really sure how a factory trait would look like...

@frewsxcv
Copy link
Member

Concrete structures in rust-geo: A next step would be to define geo factory traits. A rust-geo method like intersection would use these traits to generate the output geometry. Without factory traits, rust-geo needs concrete structures to produce results.

Maybe we could start out by having concrete structures in rust-geo, and once RFC 1522 stabilizes, we can do something like:

// Note: this struct is not `pub`
struct CentroidPoint<T: Float> {
    x: T,
    y: T,
}

pub trait Centroid<T: Float> {
    fn centroid(&self) -> Option<impl Point<T>>;
}

impl<T> Centroid<T> for Polygon<T>
    where T: Float
{
    fn centroid(&self) -> Option<impl Point<T>> {
        // do some maths and then return CentroidPoint
        CentroidPoint { x: 10., y: 10. }
    }
}

@frewsxcv
Copy link
Member

frewsxcv commented Jan 2, 2017

Hey all. I'm interested in moving forward with this.

@pka Do you have a local git branch with your changes you could open a PR with? I'd be interested in getting it ready for merging. If not, not a big deal, I'll make a branch and open a PR.

@jdroenner
Copy link
Member

I'm also still interested in this. One open point is the design of factory traits. What are the minimal requirements for such a trait?
I guess we need:

  • a function to create each geometry type
  • a way to clone/move the metadata/payload to a new geometry.

@frewsxcv
Copy link
Member

WIP PR: #85

@pka
Copy link
Member Author

pka commented Jan 15, 2017

Nice to see you PR. My implementation is in rust-postgis

frewsxcv added a commit that referenced this issue Jan 22, 2017
frewsxcv added a commit that referenced this issue Jan 24, 2017
@Farkal
Copy link

Farkal commented Apr 8, 2019

Any new on this ? The PR have been closed but we are still interesting for using geo with postgis and vectortile (based on postgis types) ? If you need some help I can try to help you 👨‍💻

nyurik pushed a commit to nyurik/geo that referenced this issue Mar 19, 2022
67: Minimal support for JTS extensions r=michaelkirk a=rmanoka

Refer discussion in georust#50 

+ support LINEARRING by parsing it as a LINESTRING
+ test LINEARRING parsing

- [x] I agree to follow the project's [code of conduct](https://github.com/georust/geo/blob/master/CODE_OF_CONDUCT.md).
- [x] I added an entry to `CHANGES.md` if knowledge of this change could be valuable to users.
---




Co-authored-by: Rajsekar Manokaran <[email protected]>
bors bot added a commit that referenced this issue Jun 21, 2023
1011: Initial geo-traits implementation r=frewsxcv a=kylebarron

- [x] I agree to follow the project's [code of conduct](https://github.com/georust/geo/blob/main/CODE_OF_CONDUCT.md).
- [ ] I added an entry to `CHANGES.md` if knowledge of this change could be valuable to users.
---

As discussed on the discord, in #838, and originally in #67, this is an initial bare-bones implementation of geometry access traits for discussion. As `@frewsxcv` suggested, for now this is a separate crate in the workspace to let us iterate on some ideas.

### Future work

#### Minimize cloning in the geo-types implementation

I got lost in lifetime errors so to get things to compile I added cloning to the geo-types implementations. I.e. for `MultiPoint` each point is cloned when accessed.

Co-authored-by: Kyle Barron <[email protected]>
@frewsxcv
Copy link
Member

Hello from four years in the future. There is an active branch being worked on with trait implementations for geometries #1011

@frewsxcv
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants