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

use a tilemap for esri imagery instead of tacking on blankTile=false #5029

Merged
merged 4 commits into from
Jun 29, 2018

Conversation

jgravois
Copy link

@jgravois jgravois commented May 7, 2018

greetings! 👋

currently OSM editors are requesting roughly 35 million Esri imagery tiles a month 🎉 and making roughly the same number of requests for tiles that don't exist 🚫.

this PR still needs work, but it introduces support for the 'tilemap' option i described in #4327 (comment)

the gist of it is that we can take advantage of a service that indicates whether or not tiles exist in a particular area of interest.

by asking whether tiles are present at zoom level 20 in the current extent of the map as soon as the edit session is instantiated, we'd forgo a lot of unnecessary requests for tiles.


// make the request and introspect the response from the tilemap server
fetch(tilemapUrl)
.then(response => response.json())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a bit of error handling might be good here... (url unavailable, cannot parse result, etc.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bigger issue here is that we'll have to polyfill fetch / promises for IE11..
(But we need to do this anyway for D3 v5 / #4929)

Copy link
Author

@jgravois jgravois May 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't have my heart set on using fetch, i just didn't know what you were using for generic requests to a CORS enabled web servers.

i've pushed up another commit which uses d3_json and aborts if an error is encountered. at this point i could use a hand triggering the function when editing is initially activated instead of waiting until the metadata context menu is opened.

@bhousel
Copy link
Member

bhousel commented May 11, 2018

Cool! Thanks @jgravois - what is missing from this now? It probably would work if you switch the const/let back to old-school var.. (again strict ES5 for IE11/Phantom 🙄)

@@ -312,6 +307,9 @@ rendererBackgroundSource.Esri = function(data) {

if (inflight[tileId]) return;

// instead of calling fetchTilemap when the metadata window is open, we should call it when editing is first activated
fetchTilemap(center, esri);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

switch the const/let back to old-school var

👍

what is missing from this now?

right now fetchTilemap() is only called when the background metadata window is opened w/ ⌘ + Shift + B.

i'm not sure how/where, but the method needs to be called as soon as users cross the zoom threshold to put iD into 'edit mode' (when Esri imagery is being used in the background anyway).

@jgravois
Copy link
Author

  • const > var
  • found existing hook trigger requests to check the esri tilemap.

this is ready for review.

@bhousel
Copy link
Member

bhousel commented Jun 15, 2018

Thanks @jgravois! I tried this out and ran into a few issues:

  1. It looks like the tilemap url isn't put together correctly:

screenshot 2018-06-15 09 21 11

I also get a eslint warning around this code
309:13 warning 'x' is assigned a value but never used no-unused-vars

Would it make more sense to calculate z/x/y first and then call esri.url, and then append a /tilemap? Rather than calling esri.url first, then calculating x/y and using regex to replace parts of the url? (just guessing)

  1. I think esri.fetchTilemap might need to use jsonpRequest instead of d3_json (similar to how the esri.getMetadata function works).
    I'm getting blocked by CORS.

screenshot 2018-06-15 09 27 58

@jgravois
Copy link
Author

jgravois commented Jun 15, 2018

thanks for taking the time to check this out @bhousel!

you were definitely on the right track with the eslint warning. i made a silly mistake when i removed my template literal to please uglify after i finishing my own testing.

clarity.maptiles.arcgis.com definitely returns a CORS response header when the request is formed properly.

@bhousel
Copy link
Member

bhousel commented Jun 15, 2018

Looks better now @jgravois - should there be code that puts the max zoom back to a higher number if the tileMap supports it? AFAICT it will just stay at [0,19] forever if it gets a negative result from the tilemap query..

@jgravois
Copy link
Author

my thought was that once a user is zoomed in tight enough to trigger a tilemap request and encounters missing tiles it'd be appropriate to downsample from there on out.

that said, I didn't really consider the scenario where they zoom back out beyond 16 and move somewhere entirely different.

I'm AFK for a couple days but happy to take another look when I get back.

@bhousel
Copy link
Member

bhousel commented Jun 15, 2018

my thought was that once a user is zoomed in tight enough to trigger a tilemap request and encounters missing tiles it'd be appropriate to downsample from there on out.

that said, I didn't really consider the scenario where they zoom back out beyond 16 and move somewhere entirely different.

Hmmm - I tested this by accident today by opening the iD walkthrough, which uses the Esri Clarity layer around Three Rivers MI, and it put the maxZoom to 19.

I'll see if I can find a way to have iD check the tilemap sometimes.

@bhousel bhousel merged commit eaa9d8d into openstreetmap:master Jun 29, 2018
@bhousel
Copy link
Member

bhousel commented Jun 29, 2018

I merged this 🎉
I ended up moving the code to call fetchTilemap into the main background() drawing function. But it will only do this if the user is already at > z18 and it will not fetch again unless the user moves more than 1km away. This way the user can move around to different places, and iD can adjust the Esri max zoom to 19 or 20 as needed.

@jgravois
Copy link
Author

awesome @bhousel. thank you 👏!

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