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

[core] Add Layer::serialize() method #16231

Merged
merged 8 commits into from
Feb 26, 2020
Merged

Conversation

alexshalamov
Copy link
Contributor

@alexshalamov alexshalamov commented Feb 25, 2020

New method allows serialization of a layer into a Value type, including all modifications done via runtime style API. New method is also an enabler for Style object serialization (sources, layers, etc). PR also fixes several issues in universal style setter / getter code.

Related issues: #7600
Fixes: https://github.com/mapbox/mapbox-gl-native-team/issues/194

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/core-sdk

@alexshalamov alexshalamov added the needs changelog Indicates PR needs a changelog entry prior to merging. label Feb 25, 2020
@alexshalamov alexshalamov self-assigned this Feb 25, 2020
@alexshalamov alexshalamov force-pushed the alexshalamov_layer_serialize branch from b879b26 to b68f954 Compare February 25, 2020 17:07
@alexshalamov alexshalamov removed the needs changelog Indicates PR needs a changelog entry prior to merging. label Feb 25, 2020
@alexshalamov alexshalamov marked this pull request as ready for review February 25, 2020 17:31
include/mbgl/style/layer.hpp Outdated Show resolved Hide resolved
src/mbgl/style/layer.cpp Outdated Show resolved Hide resolved
src/mbgl/style/layer.cpp Outdated Show resolved Hide resolved
src/mbgl/style/layer.cpp Outdated Show resolved Hide resolved
src/mbgl/style/layers/layer.cpp.ejs Outdated Show resolved Hide resolved
src/mbgl/style/layers/layer.cpp.ejs Show resolved Hide resolved
@alexshalamov alexshalamov force-pushed the alexshalamov_layer_serialize branch from b68f954 to 88ccea8 Compare February 26, 2020 15:17
Copy link
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

@alexshalamov alexshalamov merged commit ff6c142 into master Feb 26, 2020
@alexshalamov alexshalamov deleted the alexshalamov_layer_serialize branch February 26, 2020 16:15
@chloekraw
Copy link
Contributor

This is great! cc/ @mapbox/maps-android @mapbox/maps-ios

New method is also an enabler for Style object serialization (sources

@alexshalamov does it already work if you want to serialize a runtime source or light object into a Value type, or are you saying these could be enabled in a follow-on pr?

@alexshalamov
Copy link
Contributor Author

alexshalamov commented Mar 3, 2020

does it already work if you want to serialize a runtime source or light object

Not without a big effort from a developer. If and when we would enable full Style object serialization, developers would be able to:

  1. map.loadStyleUrl(streets_base_style)
  2. Do modifications via runtime styling, map.getStyle().addLayer() or map.getStyle().getLayer().setLayerProperty()
  3. Serialize, and maybe store whole Style object. serializedStyle = map.getStyle().serialize();
  4. On a subsequent start, load serialized style, map.loadStyle(serialized);

@chloekraw If source and light serialization would be required, yes, we could try adding that functionality in a follow-up releases / PRs.

@chloekraw
Copy link
Contributor

@alexshalamov thanks! for now, could we open tickets to track the work? I think source would be useful, less sure about light.

@tobrun
Copy link
Member

tobrun commented Mar 3, 2020

@chloekraw If source and light serialization would be required

Serialization support for sources would be great, light isn't used that much. To me, in the long run, it would be great to be fully compatible with style-spec. This will enable using json across style API with the mbgl value conversion system. This will simplify the generated API downstream even more and make implementing additional APIs on top more flexible.

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