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

Vector tile drawing for .mvt and .pbf format #5072

Merged
merged 97 commits into from
Jul 9, 2018

Conversation

vershwal
Copy link
Collaborator

Related #3742.

@vershwal vershwal changed the title Vectoe tile drawing for .mvt and .pbf format Vector tile drawing for .mvt and .pbf format Jun 11, 2018
@vershwal vershwal mentioned this pull request Jun 11, 2018
@geohacker geohacker requested a review from bhousel June 13, 2018 03:20
@geohacker
Copy link
Member

Nice work @vershwal! 👏

Let's outline what this PR contains for the time being - this add .mvt support by converting the tile to geojson and rendering them on the map. We'll use this internally to integrate a vector tile URL or an mbtiles file in the next sprint.

@vershwal
Copy link
Collaborator Author

if a user uploads some unknown format or broken file, some kind of error message would be incredibly helpful to find out what's going on.

Ya, this sounds good! Will do that. Thanks.

jgravois and others added 24 commits June 29, 2018 09:20
Apparently still needed to build the project on Windows
use 22 as maxZoom for Esri imagery (selectively)
The capacity defines if there is one swing on the object or more. A lot of playgrounds have one structure with two swings on it.
Add capacity to playground:swing-preset
Replace JSONP with XHR where possible
(closes openstreetmap#5121)

This commit also refactors some of the logic to make it simpler and more efficient
(removes lodash _transform too) and updates the comment that was out of date.
I am currently unable to bring-up entrance by typing in "entrance" on a vertex, this might help?
Attempt to improve entrance searchability
@bhousel bhousel merged commit 69e8309 into openstreetmap:master Jul 9, 2018
@bhousel
Copy link
Member

bhousel commented Jul 9, 2018

Thanks @vershwal this is a great start!

Quick update on this - I changed a few things below and merged the code:

  • I replaced d3_fetch (a newer API which isn't supported on IE11) with d3_request (traditional XHR)
  • There are still some todos in the code
  • I commented out the item from the map data panel.

Next action for me:

  • We really should just combine the "gpx" and "mvt" layers into a single module which handles all overlay data. They each currently register a drag-and-drop handler and these handlers are conflicting with each other. Also they have misleading names. And it's just redundant and confusing to have two things. So I'm going to combine them 😅

Next action for @vershwal :
So the code was merged but is commented out and not ready yet. I didn't want your PR to diverge too much from master. The vershwal:check branch was not really current when you started the work, so that's why the merge above did some weird things to the PR. Going forward I'd like to work more disciplined like this:

  • set main iD repo as your origin remote (you are a collaborator so this is ok)
    git remote set-url origin git@github.com:openstreetmap/iD.git
  • pull from master frequently, but avoid developing on that branch or pushing to it
    git checkout master && git pull origin master
  • create feature branches off of master before working on something
    git checkout -b new_thing
  • try to check in a little bit of work every day, and push to your feature branch.
    git push origin new_thing

We can chat on Slack more if you have questions!

@vershwal
Copy link
Collaborator Author

Hey @bhousel !! Thanks for the merge and comments.
I generally try to follow all these steps. Tere were a lot of changes in the master branch during my work in this PR so there were many merge conflicts and things got complicated.
I'll take care of this for the next time.
Thanks.

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.

10 participants