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.

Sign up for GitHub

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

Jump to bottom

ClosestPoint for Triangle, Rect, GeometryCollection types & Geometry enum #675

Merged
merged 6 commits into from
Nov 19, 2021
Merged

ClosestPoint for Triangle, Rect, GeometryCollection types & Geometry enum #675

merged 6 commits into from
Nov 19, 2021

Conversation

pkopparla
Copy link
Contributor

@pkopparla pkopparla commented Nov 2, 2021 •
edited
Loading

  • I agree to follow the project's code of conduct.
  • I added an entry to CHANGES.md if knowledge of this change could be valuable to users.

This pull request addresses issue #544
The ClosestPoint method was implemented for the remaining members of the Geometry enum ie., Triangle, Rect and GeometryCollection and then implemented for the enum itself.

@pkopparla pkopparla changed the title ClosestPoint for Tri, Rect, GC & Geometry ClosestPoint for Triangle, Rect, GeometryCollection types & Geometry enum Nov 2, 2021
@michaelkirk
Copy link
Member

bors try

michaelkirk
michaelkirk requested changes Nov 2, 2021
View reviewed changes
@michaelkirk
Copy link
Member

A couple other housekeeping things:

  • please update CHANGES.md
  • please run cargo fmt

@michaelkirk michaelkirk mentioned this pull request Nov 6, 2021
Merged
2 tasks
@lnicola
Copy link
Member

lnicola commented Nov 7, 2021 •
edited
Loading

There's still a merge conflict on the changelog.

@pkopparla
Copy link
Contributor Author

pkopparla commented Nov 7, 2021 •
edited
Loading

Thanks @lnicola . This implementation is still not finalized, I will resolve conflicts before it is ready to be merged.

bors bot added a commit that referenced this pull request Nov 13, 2021
@bors @michaelkirk
Merge #679
b5d1a7f 
679: Fix ClosestPoint when internal for Polygons r=frewsxcv a=michaelkirk

- [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.
---

While discussing #675, I noticed our current implementation of ClosestPoint for Polygon seems incorrect.

Previously, we were only considering the boundary of the polygon. But presumably we should behave like JTS and [PostGIS](https://postgis.net/docs/ST_ClosestPoint.html) which consider interior points to have a distance 0.



Co-authored-by: Michael Kirk <michael.code@endoftheworl.de>
@pkopparla
Copy link
Contributor Author

@michaelkirk I've changed the implementation to use to_lines now, how about this?

@michaelkirk
Copy link
Member

The implementation LGTM!

If you can fix up the conflict in the changelog, I think this should be good to merge.

@pkopparla
Copy link
Contributor Author

I think that should've fixed the changelog now. Thanks for reviewing @michaelkirk !

@lnicola
Copy link
Member

lnicola commented Nov 18, 2021

Still needs a rebase, I think.

@pkopparla
Copy link
Contributor Author

@lnicola I rebased it on georust/master before the last commit, would you mind explaining what I should now?

@lnicola
Copy link
Member

lnicola commented Nov 18, 2021

bors try

@bors
Copy link
Contributor

bors bot commented Nov 18, 2021

try

Already running a review

@urschrei
Copy link
Member

bors try-

@urschrei
Copy link
Member

bors try

bors bot added a commit that referenced this pull request Nov 18, 2021
@bors
Try #675:
93e8867
@lnicola
Copy link
Member

lnicola commented Nov 18, 2021

bors retry

?

@bors
Copy link
Contributor

bors bot commented Nov 18, 2021

try

Already running a review

@bors
Copy link
Contributor

bors bot commented Nov 18, 2021

try

Build failed:

@urschrei
Copy link
Member

Oh ffs. This is because cargo shipping with rust < 1.56.0 doesn't know what to do with the 2021 feature, isn't it?

@urschrei
Copy link
Member

pdqselect 0.1.1 (an rstar dep) has switched to 2021, which looks to be the cause here.

@urschrei
Copy link
Member

This isn't fixable in the PR: it'll require someone to pin pdqselect 0.1.0 in rstar and push out a point release, I think. We can't even fix it in our CI because there's only one stable 2021 release.

@lnicola
Copy link
Member

lnicola commented Nov 18, 2021

Does pdqselect even have a public repository?

@urschrei
Copy link
Member

Does pdqselect even have a public repository?

Not that I know of. I had to go through docs.rs to see the source: https://docs.rs/crate/pdqselect/0.1.1/source/

urschrei added a commit to georust/rstar that referenced this pull request Nov 18, 2021
@urschrei
Pin pdqselect to 0.1.0
675b62a 
pdqselect 0.1.1 has switched to the 2021 edition, which is breaking
some downstream crates which rely on older rust version in their CI.

See discussion in georust/geo#675
@urschrei
Copy link
Member

bors try

bors bot added a commit that referenced this pull request Nov 18, 2021
@bors
Try #675:
e0dfe14
@bors
Copy link
Contributor

bors bot commented Nov 18, 2021

try

Build failed:

@urschrei
Copy link
Member

🤷‍♂️

@urschrei
Copy link
Member

bors try

bors bot added a commit that referenced this pull request Nov 18, 2021
@bors
Try #675:
c73e40c
@bors
Copy link
Contributor

bors bot commented Nov 18, 2021

try

Build failed:

@urschrei
Copy link
Member

Sorry everyone, it's a "real" error now – looks like something iterator-related landed for arrays in 1.52

@pkopparla
Copy link
Contributor Author

Thank you @urschrei and @lnicola for figuring out that problem. I've fixed the error based on the compiler help message, hopefully it will work now.

@lnicola
Copy link
Member

lnicola commented Nov 19, 2021

bors try

bors bot added a commit that referenced this pull request Nov 19, 2021
@bors
Try #675:
9c09bf6
@bors
Copy link
Contributor

bors bot commented Nov 19, 2021

try

Build succeeded:

@lnicola
Copy link
Member

lnicola commented Nov 19, 2021

bors r+

would you mind explaining what I should now?

Sorry, I was confused by this GitHub message:

image

@bors
Copy link
Contributor

bors bot commented Nov 19, 2021

Build succeeded:

@bors bors bot merged commit 13f073a into georust:master Nov 19, 2021
@pkopparla pkopparla deleted the geom_enum_closest branch November 19, 2021 08:55
@michaelkirk
Copy link
Member

Thanks for seeing it through @pkopparla!

@callpraths callpraths mentioned this pull request Mar 5, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaelkirk michaelkirk michaelkirk requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pkopparla @michaelkirk @lnicola @urschrei