-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Port Esri wayback imagery from Rapid #11780
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
base: develop
Are you sure you want to change the base?
Conversation
a8a9009 to
852a88e
Compare
By sheer coincidence, Rapid implemented this feature independently shortly after OHM had implemented it in iD. Rapid’s implementation is more robust. In OHM iD’s implementation, a failure to load the Esri endpoint blocks the background layer list from loading at all, preventing the user from seeing any imagery. The Rapid team also convinced Esri to expose an endpoint for filtering out redundant layers given a particular location, which really cuts down on noise. The two teams agreed that it would be a good idea to bring Rapid’s implementation to OHM’s iD at some point: OpenHistoricalMap/issues#806. If this can happen through OSM’s iD, then it means less for OHM iD’s development team (headcount: 0.125 on a good day) to maintain. OHM wants to keep its fork as lightweight as possible because merges from upstream are very infrequent. As an OHM mapper, the only thing I’d miss from Rapid’s design is the ability to flip back and forth easily between two Esri Wayback layers. Perhaps an OSM mapper would feel the same if their use case is to compare Esri Wayback to another undated layer. As it is, the CtrlB shortcut does nothing if your last two layers are both Esri Wayback layers. A fix for that would really improve usability. Longer-term, a time slider control would optimize for the use case of flipping back and forth between two adjacent layers, which is probably the most common use case. It would be more efficient than a dropdown menu that you have to play guess-and-check with, especially on Windows.1 We could reuse the time slider that #10394 added to the Photo Overlays panel. Footnotes
|
| } | ||
|
|
||
| /** | ||
| * Get wayback release dates formatted for UI display |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dates are in ISO 8601 format, which may confuse some users. Ideally we’d format the date according to the user’s locale, as in the Photo Overlays panel. The short date format would be fine in this space-constrained control. But we’ll need to distinguish between the display format and the value, since the formatted dates won’t necessarily sort the same way.
iD/modules/ui/sections/photo_overlays.js
Line 298 in c0a7831
| date: new Date(now - Math.pow(dateSliderValue('from'), 1.45) * 10 * 365.25 * 86400 * 1000).toLocaleDateString(localizer.localeCode()) })); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, formatting according to the user's locale would definitely be ideal.
| const waybackInfo = parseWaybackId(requestedBackground); | ||
| if (waybackInfo.isWayback) { | ||
| const waybackSource = background.findSource(ESRI_WAYBACK_ID); | ||
| if (waybackSource) { | ||
| waybackSource.date(waybackInfo.date); | ||
| background.baseLayerSource(waybackSource); | ||
| } else { | ||
| background.baseLayerSource(getFallbackSource(background.findSource(requestedBackground))); | ||
| } | ||
| } else { | ||
| background.baseLayerSource(getFallbackSource(background.findSource(requestedBackground))); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading again, this can likely be…
| const waybackInfo = parseWaybackId(requestedBackground); | |
| if (waybackInfo.isWayback) { | |
| const waybackSource = background.findSource(ESRI_WAYBACK_ID); | |
| if (waybackSource) { | |
| waybackSource.date(waybackInfo.date); | |
| background.baseLayerSource(waybackSource); | |
| } else { | |
| background.baseLayerSource(getFallbackSource(background.findSource(requestedBackground))); | |
| } | |
| } else { | |
| background.baseLayerSource(getFallbackSource(background.findSource(requestedBackground))); | |
| } | |
| const waybackInfo = parseWaybackId(requestedBackground); | |
| const waybackSource = background.findSource(ESRI_WAYBACK_ID); | |
| if (waybackInfo.isWayback && waybackSource) { | |
| waybackSource.date(waybackInfo.date); | |
| background.baseLayerSource(waybackSource); | |
| } else { | |
| background.baseLayerSource(getFallbackSource(background.findSource(requestedBackground))); | |
| } |
FYI: parseWaybackId validates the URL param to be valid, especially the date given that it is essential for the service.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me to simplify those two branches.
| } | ||
| }); | ||
|
|
||
| // Only add EsriWayback if 'EsriWorldImagery' exists, inserting it right after it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "only add if" is not the goal here but given we should always be present, I did not think we need an else-branch here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess one could argue that if one uses a customized instance of iD with it own set of custom imagery sources (that does not include ESRI World Imagery), it does indeed make sense to not insert the "ESRI Wayback" layer.
Maybe it would be slightly more elegant to define "ESRI Wayback" in data/manual_imagery.json somehow (which can be more easily customized by potential forks of iD) instead of hardcoding it here and relying on the presence of another (technically independent) layer. Instead, here we could just make sure that it is sorted properly next to the other ESRI layer. Feel free to leave it as is, though, if you think it is not worth the effort.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PS: I think it would make more sense to include the Wayback entry after the regular ESRI World Imagery layer: It is more common to use the latest imagery while mapping compared to older versions of the imagery.
| "build": "run-s build:css build:data build:js", | ||
| "build:css": "node scripts/build_css.js", | ||
| "build:data": "shx mkdir -p dist/data && node scripts/build_data.js", | ||
| "build:data": "shx mkdir -p dist/data && npm run imagery && node scripts/build_data.js", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This way the npm run all also runs the imagery. Or what is the usual process?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
previously run run imagery was typically only needed while updating the editor-layer-index in the release process.
But I don't see why npm run all shouldn't run the imagery step as well (intuitively, I would have assumed that all should also include imagery). I would have however include it in the all command as "all": "run-s clean imagery build dist" (instead of in the build:data command), as the imagery script is not namespaced with the other build commands. And if we do that, we should also update the following build scripts that currently do npm run imagery and npm run all separately:
-
scripts/deploy.sh -
.github/workflows/staging.yml - perhaps simplify
RELEASING.md1
Footnotes
-
npm run allshould actually not be needed at that point, or more precisely: if there would be changes while doingnpm run all, they should not be committed together with thenpm run imageryresult. 🤔 maybe it would be best to do this in an extra step, i.e.…npm install editor-layer-indexnpm run imagerygit add . && git commit -m 'npm run build'npm run buildgit add . && git commit -m 'npm run build' # this should typically return that there is "nothing to commit"– feel free to not incorporate these changes in this PR; I can do that afterwards. ↩
tyrasd
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thank you.
I haven't looked at the concrete "business logic" parts of this PR yet, but as this is practically already in use for a while now in Rapid, I would assume that it is sufficiently well tested. 😉
What I found notices and found somewhat confusing is that for some regions, the displayed "wayback" dates do not really correspond to the actual vintage of the imagery. For example, at coords 46.46258/11.25749, some dates for 2025, 2024 and 2023 are offered, but all only contain 2022 imagery.
Interestingly, when viewing the same area on ESRI's wayback website, the newer 2025 and 2024 dates are not listed… Maybe there is some API parameter which can limit to only return "relevant" dates for a location, that we're not yet setting?
And, it is really not ideal that the wayback imagery vintage can be actually so much off from the "layer" name used in the dropdown box. Not sure if there's anything we can do about that, is there? Fetching the individual metadata layers is probably too slow… The only way I could think of would be to label the entries with our own numbering scheme instead of the pseudo-dates (e.g. A, B, … instead of 2025-12-18, 2025-01-30, …), then it would be clear to the user that they need to use the background panel to check for the real vintage of the layer.
Also, do you know what the values for the cryptic entries for Source and Description mean in the background panel?
Lastly, I noticed that there is quite a lot of network traffic to wayback.maptiles.arcgis.com, even when the ESRI wayback layer is not active. That's really not ideal, and I'm wondering whether there is a reason for that? I'd assume it might have to do with getting the list of available wayback layers for the respective location… Ideally, the list of available layers would only be fetched whenever the background layers list is actually rendered on the map (and while the data is not yet loaded, it should use a loading animation in place of the dropdown).
| fs.writeFileSync('dist/data/languages.min.json', JSON.stringify(languageInfo)); | ||
|
|
||
| // Save individual data files | ||
| // Note: data/imagery.json and data/imagery_esri_wayback.json should be generated first via 'npm run imagery' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd omit this comment: as the mentioned files are mirrored in the repo, it is not strictly required to generate them first. (Only if you want to update the actually stored data in those files, you need to execute the command.)
So, I think this would only unnecessarily add confusion to mention it here.
| minifyJSON('data/qa_data.json', 'dist/data/qa_data.min.json'), | ||
| minifyJSON('data/shortcuts.json', 'dist/data/shortcuts.min.json'), | ||
| minifyJSON('data/territory_languages.json', 'dist/data/territory_languages.min.json'), | ||
| minifyJSON('data/imagery_esri_wayback.json', 'dist/data/imagery_esri_wayback.min.json'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please put this up as (line 94, just below handling of data/imagery.json). That keeps it together with the related imagery data and the list sorted.
| const waybackInfo = parseWaybackId(requestedBackground); | ||
| if (waybackInfo.isWayback) { | ||
| const waybackSource = background.findSource(ESRI_WAYBACK_ID); | ||
| if (waybackSource) { | ||
| waybackSource.date(waybackInfo.date); | ||
| background.baseLayerSource(waybackSource); | ||
| } else { | ||
| background.baseLayerSource(getFallbackSource(background.findSource(requestedBackground))); | ||
| } | ||
| } else { | ||
| background.baseLayerSource(getFallbackSource(background.findSource(requestedBackground))); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me to simplify those two branches.
| } | ||
| }); | ||
|
|
||
| // Only add EsriWayback if 'EsriWorldImagery' exists, inserting it right after it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess one could argue that if one uses a customized instance of iD with it own set of custom imagery sources (that does not include ESRI World Imagery), it does indeed make sense to not insert the "ESRI Wayback" layer.
Maybe it would be slightly more elegant to define "ESRI Wayback" in data/manual_imagery.json somehow (which can be more easily customized by potential forks of iD) instead of hardcoding it here and relying on the presence of another (technically independent) layer. Instead, here we could just make sure that it is sorted properly next to the other ESRI layer. Feel free to leave it as is, though, if you think it is not worth the effort.
| } | ||
| }); | ||
|
|
||
| // Only add EsriWayback if 'EsriWorldImagery' exists, inserting it right after it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PS: I think it would make more sense to include the Wayback entry after the regular ESRI World Imagery layer: It is more common to use the latest imagery while mapping compared to older versions of the imagery.
| } | ||
|
|
||
| /** | ||
| * Get wayback release dates formatted for UI display |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, formatting according to the user's locale would definitely be ideal.
This is the date in the layer name, corresponding to the date on which Esri released the imagery layer. As Esri Wayback is a global mosaic, there’s necessarily a lag between capture and release. Additionally, the imagery in a given area may be reused across releases. This implementation implements the “Only versions with local changes” checkbox by default, which is supposed to weed out those reused layers. That’s why the list is so much shorter than it is in OHM’s iD. For example, at 39°16′57″N, 84°17′7″W, this branch lists only 13 Esri Wayback layers, compared to 179 in OHM’s iD. Are you seeing a lot of redundancy between layers that would suggest the layers are still duplicates?
I’m not sure a bespoke identifier scheme would be very useful. Why not append something, like “12/18/2025 Release”? That would at least set an upper bound on the capture date, assuming Esri doesn’t sometimes revert to older imagery. If we really can’t label the layers with anything reliable, then we could replace the dropdown menu with a slider, as I mentioned in #11780 (comment).
“Source: WV02” refers to the WorldView-2 satellite. “Description: Vantor” is the name of the vendor from whom Esri purchased the imagery. You may be more familiar with their previous name, Maxar.
Yes, this is actively filtering the list down to just the relevant layers for the current location. It has to do it separately for each visible tile by fetching the tilemap/ endpoint for the tile. We should find a way to defer this until the user first pops open the Background panel. We could even hide the dropdown until the user switches to Esri Wayback. (This would be consistent with a redesign to show a slider like in the Photo Options section.) Then it loads the actual tile images too, even if an unrelated layer like Bing is currently active. This seems like a bug to me. |
Inspired by the talk at osmlab/editor-layer-index#2805 I looked into migrating the Esri Wayback imagery setup from Rapid to iD.
I used the Wayback imagery before in Rapid and find them quite useful in some specific mapping task. I wrote about it a bit at https://en.osm.town/@tordans/112301870063364573 a while back.
The background imagery system in Rapid is quite a bit different from iD, but the general setup can be ported.
UI
background=EsriWayback_2025-01-30where the part after the_is the date from the dropdownCode
Differences to Rapid
I opted for this one instead of https://github.com/rapideditor/wayback-core / https://www.npmjs.com/package/@rapideditor/wayback-core since this is now archived and Esri has a few newer commits https://github.com/Esri/wayback-core/commits/main/ (vs https://github.com/rapideditor/wayback-core/commits/main/)
References to Rapid
Non-references
Testing
I tested it locally at http://127.0.0.1:8080/#disable_features=points,boundaries&map=17.27/52.45327/12.94761&locale=en&background=EsriWayback_2025-01-30 and compared it to Rapid https://rapideditor.org/edit#map=17.03/52.45219/12.94909&background=EsriWayback_2025-01-30&disable_features=points,boundaries
Use cases
backgroundworks and selects the right dateTodos: