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

Compute centroid of relations with multiple outer members #103

Closed

Conversation

timonmasberg
Copy link

@timonmasberg timonmasberg commented Jul 22, 2021

Adds the functionality to calculate a centroid for relations that have multiple outer members that are non-closed ways.


Here's the reason for this change 🚀

Should completely implement #96
Also, this PR moves some utility functions and the interactions with LevelDB into files.


Here's what actually got changed 👏

  • Outsource LevelDB functionality
  • Create a file for utility functions
  • Add function to compute relation centroid

Here's how others can test the changes 👀

Run the relation_centroid tests or run the program with a pbf that includes relations with multiple outer ways and check the centroid.

@missinglink
Copy link
Member

👍 for reformatting/cleaning up the code, this was one of the first projects I wrote in Go ~6 years ago.

@timonmasberg
Copy link
Author

Sorry for the delay, uni stuff... I committed relation_centroid which includes the functionality of merging open outer ways together and calculating the geo centroid of it. Since there can be multiple outer areas, it connects where either the first coordinates matches the last or the other way around and also checks if for clockwise ways. So in the end there might be one complete polygon or multiple ones (and maybe in some cases broken ones, meaning first coords != last cords, they will be ignored).

Please also check the small changes I made for Relations in the pbf2json file.

Some things that are not covered:

  • If there are outer areas that use the same way twice, it can only be used once since it gets merged with any connecting way and therefor will not be available for the second outer way. I have never seen this, neither do I know if this would be a problem because the exact way might be a member twice.

Some things that need to be clarified:

  • Do you think we should label centroids calculated with the outer area, as we do it with the admin_centre and with entrances?
  • How do we handle relations in which not every member is present in the pbf, e.g. for Hamburg some members of a relation are in Hamburg and some are outside, the relation itself is given but only with members in Hamburg resulting in non-calculable bounds and centroids. Currently, it just prints a non-fatal warning.

__
Also, I think it would be nice to stick to the geo library structs Point, Bounds, PointSets etc. until we have to print them, it makes stuff a bit easier than mixing them up with maps and then always converting them. Maybe I will refactor functions like findMemberWayLatLons to return Point Structures (what are your thoughts on this?). This would also make everything more testable.

@timonmasberg
Copy link
Author

👍 for reformatting/cleaning up the code, this was one of the first projects I wrote in Go ~6 years ago.

Same for me, this is my first experience with go. So please be gentle :)

@timonmasberg timonmasberg changed the title WIP: Compute centroid of relations with multiple outer members Compute centroid of relations with multiple outer members Aug 9, 2021
@missinglink
Copy link
Member

missinglink commented Aug 10, 2021

Nice work, ok, let's look at getting this merged-in.
Since it's got quite big I think it might be prudent to split it up and merge some of the unrelated parts separately.

I've split out moving the 'cache' functions to their own file in #104

That should clear up the diff a bit, are you comfortable to rebase it?
If not you can add a merge commit and we can squash it out later.

There will be some minor conflicts with the casing of the function names, I've left them as lowercase which is a Golang convention to indicate they are private to the scope of the module (as opposed to Capitalized which indicates a public API), in this project it's not such a big deal since everything is in the main module, but it's a good best-practise.

So you can probably just remove your copy of cache.go and revert the casing of the function names and it should work fine.

git origin add pelias https://github.com/pelias/pbf2json.git
git fetch pelias

git rebase pelias/master
# or 
# git merge pelias/master

Let me know if you need a hand doing that, which will leave us with a cleaner diff with just your awesome centroid changes.

[edit] or what might be easier is just removing this one commit.

@timonmasberg
Copy link
Author

timonmasberg commented Aug 15, 2021

this should do the job. @missinglink

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.

2 participants