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

Imagery Offset Database #4166

Closed
wants to merge 5 commits into from
Closed

Imagery Offset Database #4166

wants to merge 5 commits into from

Conversation

kepta
Copy link
Collaborator

@kepta kepta commented Jul 20, 2017

@bhousel would need help in figuring out certain things.

  • What do we keep the padding of imagery points ?
  • Where should we invoke the check for imagery offset ? Should it happen automatically or be shown in the imagery adjustment UI, like click to apply saved imagery offset.

(closes #1124 )

@bhousel bhousel changed the title Imagery Imagery Offset Database Jul 20, 2017
@bhousel
Copy link
Member

bhousel commented Jul 20, 2017

This is a great start - just a reminder to use 4 space indent, like we do everywhere else in iD. (we should probably have an .editorconfig file in our project root that enforces this.)

What do we keep the padding of imagery points ?

Ideally we don't want to pad the lookup and store in a location cache - it would be better to cache the results of each tile url that we lookup. I'm not sure whether the service could handle the load of all iD users asking for offset for every tile. What do you think @Zverik ?

Where should we invoke the check for imagery offset ? Should it happen automatically or be shown in the imagery adjustment UI, like click to apply saved imagery offset.

Ideally automatically. We store the user supplied offset here in background_source. We could add a bool variable there to say whether the offset is automatically fetched, and we could add a corresponding checkbox on the Background ui screen.

@bhousel
Copy link
Member

bhousel commented Jul 20, 2017

@kepta As a next step, can you add a test here: https://github.com/openstreetmap/iD/tree/master/test/spec/services

@Zverik
Copy link
Contributor

Zverik commented Jul 20, 2017

Thanks for the pull request. Automatically checking the offset for every tile is an overkill and will quickly put the server offline. I suggest doing that when opening the editor, and after that when a user scrolls the map more than a kilometer (at least 500 m) off the last offset checking point.

@kepta
Copy link
Collaborator Author

kepta commented Jul 24, 2017

My bad forgot to check the indentation, I have fixed it in the latest commit. @bhousel should I rebase and squash the commits, to make it look better? (Side Note: My laptop broke, so lost all the configurations :'( )

I have added some simple tests and also added a padding of 1km to each imagery pos as per @Zverik 's recommendation. I am still not sure about the radius to search in.

Next actions would be, to change the way search works. Currently the backend gives the whole list of imagery's within the supplied radius. We would like to cache all of it but return back only the imagery offset matching with the active imagery.

The backend gives the data_url of the imagery, I guess that can be used to map the active imagery to the one in the cache?.

@bhousel
Copy link
Member

bhousel commented Jul 25, 2017

My bad forgot to check the indentation, I have fixed it in the latest commit. @bhousel should I rebase and squash the commits, to make it look better?

No problem. I don't care much about how the commit history looks.

I have added some simple tests and also added a padding of 1km to each imagery pos as per @Zverik 's recommendation. I am still not sure about the radius to search in.

You could probably use a large radius, since we're caching the results (so that would mean fewer calls) - and the results packets are small, and there probably aren't too many.

I guess try it with a few different search radii and see?

@kepta
Copy link
Collaborator Author

kepta commented Jul 25, 2017

I guess try it with a few different search radii and see?

That makes sense, I will go for an increased radius. @bhousel Any thoughts on how to match the active imagery with the one in cache?

@bhousel
Copy link
Member

bhousel commented Jul 25, 2017

That makes sense, I will go for an increased radius. @bhousel Any thoughts on how to match the active imagery with the one in cache?

Yes, I think we can do this similarly to how I sample imagery vintage in the Background panel.

The background layer is a bunch of <img> elements, and one of them which is closest to the center of the screen will be classed with .tile-center.

Here is some code that you can stuff into the background panel. There is already a getVintage function, so this would be how you might write a getOffset function.

var tile = d3.select('.layer-background img.tile-center');   // tile near viewport center
if (tile.empty()) return;

var d = tile.datum();   // the data for that tile
if (!d || d.length < 4) return;

var zoom = d[2];
var url = d[3];
var center = context.map().center();

// service.getOffset will work somewhat like our nominatim service reverse():
// 1. use magic to turn the url, center, and zoom level into an identifier, per
//     https://wiki.openstreetmap.org/wiki/Imagery_Offset_Database/API#TMS
// 2. query the offset database with those parameters (or call callback w/cached result)
// 3. cache result and call the callback
service.getOffset(url, center, zoom, function(err, result) {
   // console.log something?
   // or update a div someplace with the numbers
});

inflight = {};
offsetCache = rbush();
},
getImageryID: function(url, imageryType) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Zverik could I get your review for this piece of code, I tried to convert the Java code you wrote in Javascript.

If you could also point me to some unit test cases for this function in your code base ..

Copy link
Contributor

Choose a reason for hiding this comment

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

JavaScript probably contains more (or better) files for handling URLs. I'm wondering if it makes sense to write this differently, particularly when parsing the parameters part of the URL

@kepta
Copy link
Collaborator Author

kepta commented Jul 31, 2017

@bhousel do we support WMS type of imageries in iD, since I couldn't find one in imagery.json?

@bhousel
Copy link
Member

bhousel commented Jul 31, 2017

do we support WMS type of imageries in iD, since I couldn't find one in imagery.json?

Not yet.. that's #1141

// if (url.indexOf("scanex_irs") > )
if (
imageryType === 'TMS' &&
url.match(/.+tiles\.mapbox\.com\/v[3-9]\/openstreetmap\.map.*/)
Copy link
Contributor

Choose a reason for hiding this comment

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

A sed pattern (s/foo/bar/) can use any character to delimit, and it's normal to use something other than / when dealing with URLs, e.g. s@foo@bar@. This avoids excessive escaping. Is this possible with Javascript's regex implementation?

@@ -185,7 +185,7 @@ export function uiBackground(context) {
function update() {
backgroundList.call(drawList, 'radio', clickSetSource, function(d) { return !d.overlay; });
overlayList.call(drawList, 'checkbox', clickSetOverlay, function(d) { return d.overlay; });

Copy link
Contributor

Choose a reason for hiding this comment

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

Stray whitespace

var center = context.map().center();
console.log(d);
searchOffset.search(center,url, console.log);
// zoom = (d && d.length >= 3 && d[2]) || Math.floor(context
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this commented code be removed?


export function uiPanelBackground(context) {
var background = context.background();
var currSource = null;
var currZoom = '';
var currVintage = '';

debugger;
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems out of place

) {
return;
}
console.log('here')
Copy link
Contributor

Choose a reason for hiding this comment

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

Left over debugging statement?

var kv = param.split('=');
console.log(kv);
kv[0] = kv[0].toLowerCase();
// WMS: if this is WMS, remove all parameters except map and layers
Copy link
Contributor

Choose a reason for hiding this comment

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

Not needed for iD

url = url.slice(0, questionMarkIndex);
}

var removeWMSParams = [
Copy link
Contributor

Choose a reason for hiding this comment

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

Not needed for iD

inflight = {};
offsetCache = rbush();
},
getImageryID: function(url, imageryType) {
Copy link
Contributor

Choose a reason for hiding this comment

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

JavaScript probably contains more (or better) files for handling URLs. I'm wondering if it makes sense to write this differently, particularly when parsing the parameters part of the URL

@pnorman
Copy link
Contributor

pnorman commented Aug 1, 2017

do we support WMS type of imageries in iD, since I couldn't find one in imagery.json?

Not yet.. that's #1141

I wouldn't worry about adding any WMS code here. #1141 is probably a long way off, and there's no way to currently test any WMS code you write.

@bhousel
Copy link
Member

bhousel commented Aug 2, 2017

I wouldn't worry about adding any WMS code here. #1141 is probably a long way off, and there's no way to currently test any WMS code you write.

I'd prefer to leave it in actually. This isn't WMS code, but rather code for generating an identifier to query the the Offset Database.

@kepta
Copy link
Collaborator Author

kepta commented Aug 2, 2017

Hey @Zverik , do you have test cases for this code

@bhousel
Copy link
Member

bhousel commented Jan 29, 2020

stale

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.

4 participants