Skip to content
This repository was archived by the owner on Aug 8, 2023. It is now read-only.

Styleable MapSnapshotter#16268

Merged
alexshalamov merged 13 commits intomasterfrom
alexshalamov_styleable_snapshotter
Mar 5, 2020
Merged

Styleable MapSnapshotter#16268
alexshalamov merged 13 commits intomasterfrom
alexshalamov_styleable_snapshotter

Conversation

@alexshalamov
Copy link
Copy Markdown
Contributor

@alexshalamov alexshalamov commented Mar 4, 2020

Styleable MapSnapshotter

This PR provides means of modifying style of a MapSnapshotter. The new API enables several use-cases, such as: adding route overlays, removing extra information (layers) from a base style, adding custom images that are missing from a style.

In a follow-up PR, new constructor for a MapSnapshotter could be added that takes existing Map as a parameter, so that snapshotter could re-use already created / parsed style.

Changes:

  • Instead of running map and renderer on a separate thread, Map is running on a client thread, while orchestration and rendering is done on a separate thread. This allows us to expose Style object from the map, yet, offload most expensive tasks to a background thread.
  • Simple observer interface is added. Bare minimum that is required to add / remove layers once style is loaded.
  • Simplified MapSnapshotter constructor
  • Actors and threading information is removed from public header.
  • Added MapSnapshotter::cancel method
  • Added examples to glfw app (H key saves snapshot, J key saves snapshot with an extrusion overlay)
  • Changed usage AnnotationManager from reference to weak_ptr, as it is owned by a Map, yet, acessed by a renderer.

Adaptation for iOS / macOS / Android platforms

iOS / macOS adaptation: https://github.com/mapbox/mapbox-gl-native-ios/compare/alexshalamov_styleable_snapshotter
Android adaptation: in this PR

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • document any changes to public APIs
  • tagged @mapbox/maps-android @mapbox/maps-ios @mapbox/core-sdk if this PR adds or updates a public API
  • apply needs changelog label if a changelog is needed (remove label when added)

/cc @mapbox/maps-android @mapbox/maps-ios @mapbox/core-sdk

@alexshalamov alexshalamov self-assigned this Mar 4, 2020
@alexshalamov alexshalamov added the needs changelog Indicates PR needs a changelog entry prior to merging. label Mar 4, 2020
@alexshalamov alexshalamov marked this pull request as ready for review March 4, 2020 12:37
Copy link
Copy Markdown
Contributor

@pozdnyakov pozdnyakov left a comment

Choose a reason for hiding this comment

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

Looks great overall!

Comment thread platform/android/src/snapshotter/map_snapshotter.hpp
Comment thread platform/default/include/mbgl/map/map_snapshotter.hpp Outdated
Comment thread platform/default/src/mbgl/map/map_snapshotter.cpp Outdated
Comment thread platform/default/src/mbgl/map/map_snapshotter.cpp Outdated
Comment thread platform/default/src/mbgl/map/map_snapshotter.cpp Outdated
Comment thread platform/default/src/mbgl/map/map_snapshotter.cpp Outdated
Comment thread platform/default/src/mbgl/map/map_snapshotter.cpp Outdated
@zugaldia
Copy link
Copy Markdown
Member

zugaldia commented Mar 4, 2020

The new API enables several use-cases, such as: adding route overlays, removing extra information (layers) from a base style, adding custom images that are missing from a style.

You'll find this interesting @mapbox/navigation-ios @mapbox/navigation-android .

@alexshalamov alexshalamov force-pushed the alexshalamov_styleable_snapshotter branch from a06a12d to 3faae80 Compare March 4, 2020 19:44
@alexshalamov alexshalamov removed the needs changelog Indicates PR needs a changelog entry prior to merging. label Mar 4, 2020
@alexshalamov alexshalamov force-pushed the alexshalamov_styleable_snapshotter branch 2 times, most recently from 7e91d87 to cbca768 Compare March 4, 2020 20:03
Map and renderer / orchestrator should be able to run on a separate threads,
however, legacy AnnotationManager is shared between Map and Renderer, therefore
is not a thread safe. Until AnnotationManager is deprecated and removed from a
code-base, use it only via weak pointers.
@alexshalamov alexshalamov force-pushed the alexshalamov_styleable_snapshotter branch from cbca768 to 1229932 Compare March 4, 2020 20:08
@alexshalamov alexshalamov force-pushed the alexshalamov_styleable_snapshotter branch 2 times, most recently from 6c2e5b5 to ecc5376 Compare March 4, 2020 22:33
The clusterProperties contain only few elements at most.
@alexshalamov alexshalamov force-pushed the alexshalamov_styleable_snapshotter branch from 5c90442 to 341ce4b Compare March 5, 2020 12:01
Copy link
Copy Markdown
Contributor

@pozdnyakov pozdnyakov left a comment

Choose a reason for hiding this comment

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

lgtm

snapshotter->setStyleJSON(jni::Make<std::string>(_env, styleJSON));
} else {
style = std::make_pair(false, jni::Make<std::string>(_env, styleURL));
snapshotter->setStyleJSON(jni::Make<std::string>(_env, styleURL));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should this be a call to setStyleURL() rather than setStyleJSON()?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah, this is taken care of in #16286.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants