Separate rendering part from specification part for layers #8797

ivovandongen · 2017-04-24T00:44:08Z

In an attempt to separate rendering on a different thread..

Open ends:

Generate parts of the code. Feels repetitious
~~Use of std::enable_shared_from_this eg here so we can do this~~
I've extracted Layer::Type in a separate header to re-use it for RenderLayer subclasses. This makes it public though. ~~Maybe we should just duplicate it, especially if we want this to be part of the public API.~~
Move rendering related source files to the /renderer package:
- paint_property_binder
- paint_property_statistics
- *property_evaluator
- ...
Thorough cleanup

jfirebaugh

Looks great overall! The most significant feedback I have is about ownership of the Layer::Impl -- I don't think we need to introduce shared pointers yet.

Public LayerType enum is fine. 👍 on moving rendering related source files.

jfirebaugh · 2017-04-24T22:47:02Z

include/mbgl/style/layer.hpp

@@ -117,14 +111,12 @@ class Layer : public mbgl::util::noncopyable {
 void setMaxZoom(float) const;

 // Private implementation
- const std::unique_ptr<Impl> baseImpl;
+ const std::shared_ptr<Impl> baseImpl;


If we go the route of using immutable objects, we may use shared pointers internally (albeit probably behind an Immutable<T> class template), but for now let's keep the Layer owning the Impl uniquely. RenderLayers should never outlive their associated Layer and can therefore have a regular non-owning const Impl& reference.

RenderLayers should never outlive their associated Layer

Just realized this isn't the case. RenderLayers copied into the worker may outlive the Layer. Need to think about this.

Per chat: good solution is probably to continue to pass copies of Layers to the worker, rather than RenderLayers. The worker can then create its own RenderLayers, allowing us to preserve the invariant "RenderLayers never outlive their associated Layer" and avoid (unintentionally) sharing a Layer::Impl between the main thread and worker.

@jfirebaugh If we copy layers in GeometryTile to pass to GeometryTileWorker::setLayers and then create RenderLayers from that in the GeometryTileWorker; we would be copying the associated Impl twice right? In that case, maybe we should revert the changes to GeometryTileWorker and group_by_layout.hpp to continue to work on Layers?

@jfirebaugh I Fixed up ownership as discussed above in 7f0ad32. I Left the pointers to the concrete Implementations in the subclasses as is (const style::FooLayer::Impl* const impl; as opposed to a const style::FooLayer::Impl&) to avoid modifying all call sites.

@jfirebaugh This does not work as hoped. Since the Layer::Impl subclasses no longer contain a reference to the cascaded/evaluated paint properties, the render layers created in the GeometryTileWorker start with a blank copy, disjointed from the copy in the render layers in the style.

Do we actually need the cascaded/evaluated paint properties? Evaluated paint properties can be generated from the cascaded paint properties plus tile zoom level. With regards to cascaded properties, cascading would mainly be needed in supporting the combination of data-driven paint properties with style classes, which I don't think we support.

@jfirebaugh I figured this must be the reason that all the property-function render tests are failing. It makes me think it's because there is some state now not copied over / held on to when creating the buckets (since that is the main change).

If I understand correctly; in the painter the paint property binders (created in the bucket from the copied paint properties) and the evaluated paint properties form the Layer::Impl come together and then in the program the current properties are bound to attributes and interpolated from the values the binders were originally created with right?

c397125 fixes the render tests.

It's true that the current paint properties are inspected at render time as well as the binder, but it's either one or the other:

If the current value is a constant or camera function, it's used and the value the binder was originally created from is ignored.

Otherwise, the value the binder was originally created from is used, and the current value is ignored.

jfirebaugh · 2017-04-24T22:49:13Z

src/mbgl/layout/symbol_layout.cpp

 {

- const SymbolLayer::Impl& leader = *layers.at(0)->as<SymbolLayer>()->impl;
+ const auto& leader = *layers.at(0)->as<RenderSymbolLayer>()->impl;


Don't use auto unless the type is written elsewhere in the same line (e.g. assigning from the result of std::make_unique) or is uninteresting (iterators). Here explicitly specifying SymbolLayer::Impl aids readability.

jfirebaugh · 2017-04-24T23:26:23Z

src/mbgl/style/layer_impl.hpp

@@ -42,6 +36,7 @@ class Layer::Impl {

 // Create a new layer with the specified `id` and `sourceID`. All other properties
 // are copied from this layer.
+ // TODO: evaluate if this still meets expectations


copy is only used in one place: StyleSourcedAnnotationImpl, which is code supporting a pre-runtime styling feature that we should remove. We can save that for a separate change though.

jfirebaugh · 2017-04-24T23:28:15Z

src/mbgl/style/style.cpp

+ }
+
+ // TODO
+// auto layer = std::move(*it);


This can be deleted. When a custom layer is removed, the custom layer deinitialize function only needs to be called once.

jfirebaugh · 2017-04-24T23:28:53Z

src/mbgl/style/layers/symbol_layer_impl.hpp


- std::unique_ptr<Bucket> createBucket(const BucketParameters&, const std::vector<const Layer*>&) const override;
- std::unique_ptr<SymbolLayout> createLayout(const BucketParameters&, const std::vector<const Layer*>&,
+ // TODO move to RenderSymbolLayer?


Yes, it should. This is the symbol layer equivalent of createBucket.

ivovandongen · 2017-04-25T20:52:57Z

@jfirebaugh I update the pr with your review comments. I refrained from moving more files to the renderer package for now as the ones I had my eye on are all referred to from paint_properties.hpp. Moving it to a different package would really help readability.

ivovandongen added Core The cross-platform C++ core, aka mbgl ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold labels Apr 24, 2017

ivovandongen self-assigned this Apr 24, 2017

ivovandongen force-pushed the render-layers branch from 9194b7a to 3ab7f47 Compare April 24, 2017 00:57

jfirebaugh reviewed Apr 24, 2017

View reviewed changes

ivovandongen force-pushed the render-layers branch 2 times, most recently from c397125 to 4fde1da Compare April 25, 2017 19:08

[core] split off render layers

2677a3a

ivovandongen force-pushed the render-layers branch from 4fde1da to 2677a3a Compare April 25, 2017 20:49

ivovandongen removed the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label Apr 25, 2017

ivovandongen mentioned this pull request Apr 25, 2017

Asynchronous rendering #8820

Closed

28 tasks

jfirebaugh mentioned this pull request Apr 26, 2017

Separate rendering part from specification part for sources #8821

Merged

jfirebaugh approved these changes Apr 26, 2017

View reviewed changes

ivovandongen merged commit 3c175ad into master Apr 26, 2017

ivovandongen deleted the render-layers branch April 26, 2017 00:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate rendering part from specification part for layers #8797

Separate rendering part from specification part for layers #8797

ivovandongen commented Apr 24, 2017 •

edited

Loading

jfirebaugh left a comment

jfirebaugh Apr 24, 2017

jfirebaugh Apr 24, 2017

jfirebaugh Apr 24, 2017

ivovandongen Apr 25, 2017

ivovandongen Apr 25, 2017

ivovandongen Apr 25, 2017

jfirebaugh Apr 25, 2017

ivovandongen Apr 25, 2017

jfirebaugh Apr 25, 2017 •

edited

Loading

jfirebaugh Apr 24, 2017

jfirebaugh Apr 24, 2017

jfirebaugh Apr 24, 2017

jfirebaugh Apr 24, 2017

ivovandongen commented Apr 25, 2017

Separate rendering part from specification part for layers #8797

Separate rendering part from specification part for layers #8797

Conversation

ivovandongen commented Apr 24, 2017 • edited Loading

jfirebaugh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfirebaugh Apr 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivovandongen commented Apr 25, 2017

ivovandongen commented Apr 24, 2017 •

edited

Loading

jfirebaugh Apr 25, 2017 •

edited

Loading