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

Coordinate vs. Point #15

Open
Turbo87 opened this issue Oct 22, 2015 · 21 comments
Open

Coordinate vs. Point #15

Turbo87 opened this issue Oct 22, 2015 · 21 comments

Comments

@Turbo87
Copy link
Member

Turbo87 commented Oct 22, 2015

what is the purpose of having a Coordinate and a Point type? it seems to me that having Point should be sufficient.

@calvinmetcalf
Copy link
Member

a multipoint would be made up of multiple coordinates, not multiple points, same for lines

@Turbo87
Copy link
Member Author

Turbo87 commented Oct 22, 2015

hmm... I don't understand yet. What is the difference between the two? The name MultiPoint indicates already that it should consist of multiple points, doesn't it?

@calvinmetcalf
Copy link
Member

so in geojson a coordinate is simply an array of 2 (or more) values [x, y], a point is a geometry type object of the form

{
  "type": "Point",
   "coordiantes": [x, y]
}

and a multipoint

{
  "type": "MultiPoint",
   "coordiantes": [[x, y], [x, y]]
}

A Coordinate is a primitive type and a point is a geometry type

@Turbo87
Copy link
Member Author

Turbo87 commented Oct 22, 2015

Okay, I understand how this could be modeled around the GeoJSON representation. On the other hand GeoJSON is limited in that it only has objects and no other/custom types like e.g. Point or MultiPoint. The type attribute of the objects is essentially the type of object as the name suggests, but since Rust has a propery type system we don't need this here.

The current definition of MultiPoint is actually pub struct MultiPoint(pub Vec<Point>); so essentially this is already a list of points, not a list of coordinates.

I guess my main question is: why not merge Coordinate into Point? What would be the disadvantage?

I think the API could probably be simplified quite a bit if the two types are merged into one.

@calvinmetcalf
Copy link
Member

oh woops I missed that multipoint is made up of points nood coords

Turbo87 added a commit to Turbo87/rust-geo that referenced this issue Oct 22, 2015
This simplifies the Point API and removes an unnecessary abstraction level.

Resolves georust#15
@frewsxcv
Copy link
Member

Relevant discussion also happening in #16

@Turbo87
Copy link
Member Author

Turbo87 commented Feb 22, 2017

@frewsxcv I've thought about this a bit more and maybe it would make more sense to have a Coordinate trait (x() and y()) that Point would implement. That way we could also use another Point implementation with additional z and m properties as coordinates for e.g. a polygon.

@frewsxcv
Copy link
Member

frewsxcv commented Feb 26, 2017

So going back to earlier discussions in this thread, we should clarify Point vs. Polygon Coordinate. Right now for example, we represent LineString as a Vec<Point<T>>. I think this should be Vec<Coordinate<T>>. This matches what other formats libraries do:

Thoughts anyone? Anyone know counter-examples? Otherwise, I think it makes sense to align ourselves with other libraries instead of doing our own thing.

EDIT: typos

@jdroenner
Copy link
Member

I think it is the right thing to separate between points and coordinates. This could allow to have 2D and 3D coordinates. Additionally a geometry (Point, Line, ...) might have properties similar to geojson and/or a coordinate system which would be redundant for each coordinate.

@urschrei
Copy link
Member

+1 to going with what the other libs (JTS and GEOS, in particular), are doing.

@Turbo87
Copy link
Member Author

Turbo87 commented Feb 27, 2017

So going back to earlier discussions in this thread, we should clarify Point vs. Polygon Coordinate.

for that discussion we should clarify what the difference between a Point and a Coordinate is. I'm not seeing any definition of that yet and I would like to propose again to make Coordinate a trait that Point can implement.

using WKT and GeoJSON as examples is not useful IMHO because those are untyped data structures, while we are dealing with a typed language.

@jdroenner
Copy link
Member

At some point rust-geo will switch to traits for all geometry types #67. So if Coordinate is also a trait one can implement Point and Coordinate for the same type. I would still like to see geometries to have a spatial reference system. If each Coordinate of a (possibly large) Polygon carries the SRS this would be a large overhead. Also one could add some kind of Map to geometries to represent properties.

Here is the definition of JTS coordinates:

A lightweight class used to store coordinates on the 2-dimensional Cartesian plane.
It is distinct from {Point}, which is a subclass of {Geometry}.
Unlike objects of type {Point} (which contain additional information such as an envelope, a precision model, and spatial reference system information), a Coordinate only contains ordinate values and accessor methods.

@Turbo87
Copy link
Member Author

Turbo87 commented Feb 28, 2017

@jdroenner thanks for that quote. I think that definition makes sense and using traits for everything seems like a good idea too.

@frewsxcv what's the status of #85?

@pka
Copy link
Member

pka commented Feb 28, 2017

After implementing geometry traits for rust-postgis I've changed may mind and I'm also for a distinction between Coordinates and Points. Not necessarily within the traits, but in a concrete implementation. The problem implementing traits without Coordinate objects hit me exactly at the line and polygon implementation with additional coordinate system information. When using only Points, the CRS information is stored redundant in each vertex.

frewsxcv added a commit that referenced this issue Jun 3, 2018
Relevant conversation in #15.

This is a breaking change for `geo-types`.
frewsxcv added a commit that referenced this issue Jun 3, 2018
Relevant conversation in #15.

This is a breaking change for `geo-types`.
frewsxcv added a commit that referenced this issue Jun 4, 2018
Relevant conversation in #15.

This is a breaking change for `geo-types`.
bors bot added a commit that referenced this issue Jun 23, 2018
244: Migrate Line/LineString to be a series of Coordinates (not Points). r=urschrei a=frewsxcv

Relevant conversation in #15.

This is a breaking change for `geo-types`.

I'm _not_ planning to publish a release if this merges. I have a couple other breaking changes I'd like to consider before bumping the Rust geo ecosystem.

Co-authored-by: Corey Farwell <[email protected]>
@michaelkirk
Copy link
Member

I believe this has been addressed - all geometries are backed by Coordinates now, not points.

@michaelkirk
Copy link
Member

Every year or so, I come back to this issue and stare into the void.

For the record, I think it'd be nice to consolidate the Point and Coord structs. They offer no practical difference and the difference frequently trips people up.

A more practical approach might be #901

@michaelkirk
Copy link
Member

michaelkirk commented Sep 23, 2024

Staring into this void again. I'm going to re-open this issue.

A more practical approach might be #901

One thing that a coordinate trait would not solve are algorithms that need to return a coordinate. We have a lot of algorithms which return a coordinate (or point!). So we ultimately need at least one concrete type (but I contend that we don't need both concrete types!)

Other than the fact that this would be a very big breaking change, does anyone actually think it's a bad idea to get rid of the Coord structure and just use Point everywhere?

Edit: just to clarify, this is not an argument against a point/coord trait - we could (probably should) still do that. Here I am orthogonally arguing for merging the existing concrete Point and Coord types.

@michaelkirk michaelkirk reopened this Sep 23, 2024
@kylebarron
Copy link
Member

GEOS only has a point, right? I don't think it has a coordinate concept, and they get by fine

@urschrei
Copy link
Member

I have always been on board with a single 1D type.

@michaelkirk
Copy link
Member

michaelkirk commented Sep 28, 2024

Sheesh, how would we go about implementing this without blowing up the entire codebase.

I think in any case it would be a large change. I don't think any of the changes will be conceptually difficult, it's just a ton of very mechanical changes.

One way to make the change a little more piece-meal could be to unify the coords/point accessors - to wit, getting rid of the public field accessors on Coord, and merging those changes first:

- coord.x + coord.y
+ coord.x() + coord.y()

- Coord { x: 1, y: 2 }
+ coord!(x: 1, y: 2)
+ Coord::new(x: 1, y: 2)

- coord.x += 7 
+ coord.x_mut() += 7 

Maybe this PR is already controversial enough though 😆

It's not so much that I'm opposed to doing the work, but more so that the bigger it is, the less likely it will be reviewed, and the more ripe it will be for merge conflicts.

@kylebarron
Copy link
Member

One way to make the change a little more piece-meal could be to unify the coords/point accessors - to wit, getting rid of the public field accessors on Coord, and merging those changes first:

agreed that seems like a way to minimize the per-PR diff

@kylebarron kylebarron mentioned this issue Oct 10, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants