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

[core] Return source and layer ownership #7014

Merged
merged 5 commits into from
Nov 11, 2016
Merged

[core] Return source and layer ownership #7014

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3ec4c24
[core] Return source and layer ownership
boundsj Nov 10, 2016
8584922
[core] Remove whitespace
boundsj Nov 11, 2016
00c5210
[core] Make removing a CustomLayer triggers deinitialization
boundsj Nov 11, 2016
50d3792
[core] Make destructor default and guard against optional deinitializeFn
boundsj Nov 11, 2016
554abe9
[core] Deinitialize all custom layers when style is destroyed
boundsj Nov 11, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions include/mbgl/map/map.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,12 +159,12 @@ class Map : private util::noncopyable {
// Sources
style::Source* getSource(const std::string& sourceID);
void addSource(std::unique_ptr<style::Source>);
void removeSource(const std::string& sourceID);
std::unique_ptr<style::Source> removeSource(const std::string& sourceID);

// Layers
style::Layer* getLayer(const std::string& layerID);
void addLayer(std::unique_ptr<style::Layer>, const optional<std::string>& beforeLayerID = {});
void removeLayer(const std::string& layerID);
std::unique_ptr<style::Layer> removeLayer(const std::string& layerID);

// Add image, bound to the style
void addImage(const std::string&, std::unique_ptr<const SpriteImage>);
Expand Down
13 changes: 8 additions & 5 deletions src/mbgl/map/map.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -852,11 +852,12 @@ void Map::addSource(std::unique_ptr<style::Source> source) {
}
}

void Map::removeSource(const std::string& sourceID) {
std::unique_ptr<Source> Map::removeSource(const std::string& sourceID) {
if (impl->style) {
impl->styleMutated = true;
impl->style->removeSource(sourceID);
return impl->style->removeSource(sourceID);
}
return nullptr;
}

style::Layer* Map::getLayer(const std::string& layerID) {
Expand All @@ -881,18 +882,20 @@ void Map::addLayer(std::unique_ptr<Layer> layer, const optional<std::string>& be
impl->backend.deactivate();
}

void Map::removeLayer(const std::string& id) {
std::unique_ptr<Layer> Map::removeLayer(const std::string& id) {
if (!impl->style) {
return;
return nullptr;
}

impl->styleMutated = true;
impl->backend.activate();

impl->style->removeLayer(id);
auto removedLayer = impl->style->removeLayer(id);
impl->onUpdate(Update::Classes);

impl->backend.deactivate();
Copy link
Contributor

Choose a reason for hiding this comment

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

We will need to add special logic for CustomLayer. Currently, ~CustomLayer calls the CustomLayerDeinitializeFunction, which may make OpenGL calls. This is the reason that we call impl->backend.activate()/deactivate() -- the context needs to be active when initialization/deinitialization happens.

I think we need to preserve the behavior that removing a CustomLayer triggers deinitialization, even when returning ownership. Let's do this in Style::removeLayer, making it parallel Style::addLayer, which does initialization.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

00c5210


return removedLayer;
}

void Map::addImage(const std::string& name, std::unique_ptr<const SpriteImage> image) {
Expand Down
12 changes: 7 additions & 5 deletions src/mbgl/style/layers/custom_layer_impl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,7 @@ CustomLayer::Impl::Impl(const CustomLayer::Impl& other)
// Don't copy anything else.
}

CustomLayer::Impl::~Impl() {
if (deinitializeFn) {
deinitializeFn(context);
}
}
CustomLayer::Impl::~Impl() = default;

std::unique_ptr<Layer> CustomLayer::Impl::clone() const {
return std::make_unique<CustomLayer>(*this);
Expand All @@ -43,6 +39,12 @@ void CustomLayer::Impl::initialize() {
initializeFn(context);
}

void CustomLayer::Impl::deinitialize() {
if (deinitializeFn) {
deinitializeFn(context);
}
}

void CustomLayer::Impl::render(const TransformState& state) const {
assert(renderFn);

Expand Down
1 change: 1 addition & 0 deletions src/mbgl/style/layers/custom_layer_impl.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ class CustomLayer::Impl : public Layer::Impl {
~Impl() final;

void initialize();
void deinitialize();
void render(const TransformState&) const;

private:
Expand Down
20 changes: 17 additions & 3 deletions src/mbgl/style/style.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ void Style::addSource(std::unique_ptr<Source> source) {
sources.emplace_back(std::move(source));
}

void Style::removeSource(const std::string& id) {
std::unique_ptr<Source> Style::removeSource(const std::string& id) {
auto it = std::find_if(sources.begin(), sources.end(), [&](const auto& source) {
return source->getID() == id;
});
Expand All @@ -143,8 +143,11 @@ void Style::removeSource(const std::string& id) {
throw std::runtime_error("no such source");
}

auto source = std::move(*it);
sources.erase(it);
updateBatch.sourceIDs.erase(id);

return source;
}

std::vector<const Layer*> Style::getLayers() const {
Expand Down Expand Up @@ -185,11 +188,22 @@ Layer* Style::addLayer(std::unique_ptr<Layer> layer, optional<std::string> befor
return layers.emplace(before ? findLayer(*before) : layers.end(), std::move(layer))->get();
}

void Style::removeLayer(const std::string& id) {
auto it = findLayer(id);
std::unique_ptr<Layer> Style::removeLayer(const std::string& id) {
auto it = std::find_if(layers.begin(), layers.end(), [&](const auto& layer) {
return layer->baseImpl->id == id;
});

if (it == layers.end())
throw std::runtime_error("no such layer");

auto layer = std::move(*it);

if (CustomLayer* customLayer = layer->as<CustomLayer>()) {
customLayer->impl->deinitialize();
}

layers.erase(it);
return layer;
}

std::string Style::getName() const {
Expand Down
4 changes: 2 additions & 2 deletions src/mbgl/style/style.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,13 +66,13 @@ class Style : public GlyphAtlasObserver,

Source* getSource(const std::string& id) const;
void addSource(std::unique_ptr<Source>);
void removeSource(const std::string& sourceID);
std::unique_ptr<Source> removeSource(const std::string& sourceID);

std::vector<const Layer*> getLayers() const;
Layer* getLayer(const std::string& id) const;
Layer* addLayer(std::unique_ptr<Layer>,
optional<std::string> beforeLayerID = {});
void removeLayer(const std::string& layerID);
std::unique_ptr<Layer> removeLayer(const std::string& layerID);

std::string getName() const;
LatLng getDefaultLatLng() const;
Expand Down