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

Add Line type #118

Merged
merged 16 commits into from
Jun 4, 2017
Merged

Add Line type #118

merged 16 commits into from
Jun 4, 2017

Conversation

mbattifarano
Copy link
Member

Adds a Line type to represent a line segment by its two endpoints.

This PR is the first step to implementing Union/Intersection traits as outlined in #80 (See also #81). Additionally, it addresses #99.

Implements the following (with converse operations where applicable) for Line:

  • Length
  • Area
  • BoundingBox
  • Centroid
  • Rotate
  • Contains (Point, Line, LineString)
  • Intersects (Point, Line, LineString, Polygon)
  • Distance (Point)

I wanted to get this up sooner rather than later, and it's already on the large side so there are a few things left unimplemented (operations on Multi* geometries for example). Additionally, several operations on LineString (Length for example) could be nicely expressed as operations on an Line iterator. As a result, there is some redundancy that could be resolved in subsequent PRs.

@mbattifarano mbattifarano changed the title Add Line type Add Line type May 24, 2017
@frewsxcv
Copy link
Member

frewsxcv commented Jun 1, 2017

sorry for the delay! this seems alright with me. my main hesitation with this is 7a4e398, but i think i have hesitations about the Geometry enum in general. anyone else have thoughts? @urschrei?

@mbattifarano
Copy link
Member Author

@frewsxcv I added Line to Geometry in this PR purely for the sake of consistency. I'm happy to revert the commit, or leave it as is and deal with the fate of the enum separately.

@frewsxcv
Copy link
Member

frewsxcv commented Jun 2, 2017

let's wait a couple and see if anyone else has anything to say, otherwise I'll ditch the Geometry enum change and merge this in

@urschrei
Copy link
Member

urschrei commented Jun 3, 2017

Sorry for the delay! I agree that having it in the enum could lead to confusion. Related: Are any of the public / private visibility refinements that are landing in 1.19 useful here?

@frewsxcv
Copy link
Member

frewsxcv commented Jun 4, 2017

Related: Are any of the public / private visibility refinements that are landing in 1.19 useful here?

Not that I know of, if you find anything, please please open an issue :)

@frewsxcv frewsxcv merged commit c5bc209 into georust:master Jun 4, 2017
@frewsxcv
Copy link
Member

frewsxcv commented Jun 4, 2017

fyi, I rebased and merged without the Geometry enum change. thanks again for the contribution!

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

Successfully merging this pull request may close these issues.

3 participants