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 Mapillary vector tiles and switch to v4 #8372

Merged
merged 10 commits into from
Jun 30, 2021

Conversation

nickplesha
Copy link
Contributor

@nickplesha nickplesha commented Mar 1, 2021

Edit: This PR has now been updated to use the new (v4) Mapillary API.

The old API will be shutting down soon so, in order to keep the Mapillary integration working, it would be great if this PR could be merged as soon as possible.


Hello, this PR updates the Mapillary integration and now it uses our vector tiles as the data source for images, sequences and map features (traffic signs and points).

With this change the loading of Mapillary data should be much faster (you should observe an improvement in loading speed of 1000%+ in very densely mapped areas).

Changes

Switched to using vector tiles instead of the Mapillary API
Updated the data and cache structure to match new data source
Fixed a bug with clicking on a Mapillary image marker
Updated how Mapillay JS is initialized (version 3.0.0 introduced some breaking changes)
Removed user filter for Mapillary images (vector tiles don't have usernames right now)
Cleaned up the code and used a bit more modern JS where possible
You can test a preview of the changes here: https://id-mapillary-tiles.netlify.app/

Check the old PR for previous discussion: #8367 as this is a new branch with all the changes and bug fixes in a single commit, based on the latest develop

Let me know if you have any question or suggestions.
Thanks!

@nickplesha
Copy link
Contributor Author

@quincylvania This PR also prepares the Mapillary integration for the upcoming changes to our API. Here's a blog post with more details: https://blog.mapillary.com/update/2021/03/03/preparing-for-the-new-mapillary-api.html

The plan is to create another PR which would just update the URLs to the new API (when it's available) and this should hopefully make for a smooth transition.

@tordans
Copy link
Collaborator

tordans commented Mar 8, 2021

@nickplesha the speed improvements sound great!

However, I was really happy to finally see a username-filter added only recently #5307 and it would be a sad thing to see it go … 😿

Looking at https://blog.mapillary.com/update/2021/03/03/preparing-for-the-new-mapillary-api.html it sounds like, that usernames will be filterable soon again, so you would be able to re-add them soon?

Filtering will happen on the client side of the vector tiles (for example filtering image point data by a date range, a set of usernames, or spatial overlap with a polygon)

Or does this part reference the fact, that filtering need to be done in iD now, so it still could be there but needs to be reworked?

@nickplesha
Copy link
Contributor Author

@tordans Yes, the new vector tiles will have all the data necessary to enable filtering like we have in the existing API (v3). I've created this PR using our current vector tiles and will update to use the latest tiles as soon as they are available.

@nickplesha nickplesha changed the title Use Mapillary vector tiles for images, sequences and map features Use Mapillary vector tiles and switch to v4 Jun 15, 2021
@nickplesha
Copy link
Contributor Author

Updated PR with the switch to the v4 Mapillary API and vector tiles.

@mbrzakovic mbrzakovic self-requested a review June 23, 2021 12:51
Copy link
Collaborator

@mbrzakovic mbrzakovic left a comment

Choose a reason for hiding this comment

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

First of all, great job on this @nickplesha!
The code is clean, and I found it easy to understand.
I had few curious comments which are non-blockers for this PR and if needed they can be addressed later on.

modules/svg/mapillary_images.js Show resolved Hide resolved
Object.values(_mlyCache.map_features.inflight).forEach(abortRequest);
Object.values(_mlyCache.points.inflight).forEach(abortRequest);
Object.values(_mlyCache.sequences.inflight).forEach(abortRequest);
Object.values(_mlyCache.requests.inflight).forEach(function(request) { request.abort(); });
}

_mlyCache = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I was looking into memory consumption while browsing around with mapillary features turned on, and observed that allocated memory is never cleaned up...
Perhaps we can consider the following:

  • Clear cache periodically [time based, or check on each tileLoad if num of loaded tiles are above some threshold).
    We can discuss which policy would be the best for choosing the tiles to delete.. I can give an example on top of my mind: Clear tiles that are most distant from current projection view.
  • Clear cache when 'Photo Overlays' check boxes are toggled off. (or throttle/delay that operation for clicky users)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggestion one makes sense, we can set a threshold and clean up when it's reached. Ideally we would clean up the oldest loaded data first but we don't store a timestamp right now. I'll see if a simple solution can be found.

Suggestion two makes total sense and we can remove the cached data when the related layer is turned off.

Copy link
Member

Choose a reason for hiding this comment

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

The caches get cleared when the service.reset() is called - this happens when the user saves their edits. (all services work like this).

modules/services/mapillary.js Show resolved Hide resolved
modules/services/mapillary.js Show resolved Hide resolved
@nickplesha
Copy link
Contributor Author

Fixed merge conflicts.

@nickplesha
Copy link
Contributor Author

@mbrzakovic Thanks for reviewing the code!

I was wondering if there are plans make a new release of iD soon? I'm asking as the currently deployed version still uses Mapillary API v3 which we plan to shut down on August 18th.

@mbrzakovic
Copy link
Collaborator

@mbrzakovic Thanks for reviewing the code!

I was wondering if there are plans make a new release of iD soon? I'm asking as the currently deployed version still uses Mapillary API v3 which we plan to shut down on August 18th.

Yes, I was going to ask you what is the eol date for v3, thanks for the info!
I can't say anything official yet but you can expect the answer/announcement in the next week or two.
Its good to know that we have time on this and hopefully we won't get too close to the deadline.

@mbrzakovic
Copy link
Collaborator

Hey @nickplesha,

I was doing some local testing and found that translation for some mapillary map features are missing.
Can you repro the attached img?
If so, I think its due to Mapillary v4 using new set of feature objects and iD/data/core.yaml[mapillary_map_features] section is out-of-date.
Where can the list of feature object names be found? Curious on how often does Mapillary update this list?

image

@nickplesha
Copy link
Contributor Author

@mbrzakovic I think it was from the map feature titles, but we don't use them at all so I've removed them and I think this should fix those errors.

@mbrzakovic
Copy link
Collaborator

@mbrzakovic I think it was from the map feature titles, but we don't use them at all so I've removed them and I think this should fix those errors.

@nickplesha yes this fixes the error, unfortunately the change also disables the tooltips (hover over the mapillary features).
Can you please look closer into the code to see if we could have the solution that would eliminate the error and keep the tooltip.
From the quick look it seems to me that we would require full list of possible feature object types labels (e.g.: object--sign--advertisement) so that we could include them into core.yaml [translation files] and enable tooltips for multiple languages.

@nickplesha
Copy link
Contributor Author

@mbrzakovic OK, so that's what's it for, I haven't noticed the tooltips before. :) I brought them back now, and I've updated core.yaml to include all of our map feature classes and their names.

@mbrzakovic
Copy link
Collaborator

@mbrzakovic OK, so that's what's it for, I haven't noticed the tooltips before. :) I brought them back now, and I've updated core.yaml to include all of our map feature classes and their names.

@nickplesha Yes, thanks.
Sorry but when I go to urban areas with console on I am still seeing few more missing:
mapillary_map_features.construction.flat.driveway
mapillary_map_features.construction.barrier.temporary
mapillary_map_features.object.traffic_sign.direction_front
mapillary_map_features.object.traffic_sign.front
mapillary_map_features.object.traffic_sign.back
mapillary_map_features.object.sign.other

As mentioned above, I am curious on how often does Mapillary update this list?

@nickplesha
Copy link
Contributor Author

@mbrzakovic Thanks, I've added those classes. The last time we updated the list was two years ago and I don't anticipate significant changes in the future.

@mbrzakovic mbrzakovic merged commit 01daef3 into openstreetmap:develop Jun 30, 2021
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