From c74b6cbdda3cd4075f75a9bb1fdb68485c306283 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C3=ABl=20Lemaire?= Date: Thu, 21 Jan 2016 23:02:22 +0100 Subject: [PATCH] Fixed DefinitionWatcher receiving changes after destruction --- src/definition/DefinitionNode.cpp | 36 ++++++--------- src/definition/DefinitionNode.h | 29 ++++++------ src/definition/DefinitionWatcher.cpp | 35 ++++++++++++-- src/definition/DefinitionWatcher.h | 21 +++++++-- src/definition/DiffManager.cpp | 19 +++++++- src/definition/DiffManager.h | 5 ++ src/definition/FloatNode.cpp | 1 + src/definition/IntNode.cpp | 1 + src/definition/VegetationDefinition.cpp | 2 +- src/definition/WaterDefinition.cpp | 51 ++++++++++----------- src/definition/WaterDefinition.h | 8 ++-- src/interface/modeler/AtmosphereModeler.h | 2 +- src/interface/modeler/BaseModelerTool.cpp | 17 +++++-- src/interface/modeler/BaseModelerTool.h | 7 ++- src/interface/modeler/FloatPropertyBind.cpp | 5 +- src/interface/modeler/IntPropertyBind.cpp | 5 +- src/interface/modeler/IntPropertyBind.h | 2 +- src/interface/modeler/MainModelerWindow.cpp | 5 ++ src/interface/modeler/ModelerCameras.cpp | 3 +- src/interface/modeler/WaterModeler.cpp | 17 ++----- src/interface/modeler/WaterModeler.h | 14 ++---- src/render/opengl/OpenGLPart.cpp | 4 +- src/render/opengl/OpenGLPart.h | 4 +- src/render/opengl/OpenGLSkybox.cpp | 8 ++-- src/render/opengl/OpenGLSkybox.h | 3 +- src/render/opengl/OpenGLTerrain.cpp | 1 + src/render/opengl/OpenGLTerrain.h | 3 +- src/render/opengl/OpenGLVegetation.cpp | 2 +- src/render/opengl/OpenGLVegetation.h | 3 +- src/render/opengl/OpenGLWater.cpp | 6 +-- src/render/opengl/OpenGLWater.h | 3 +- src/render/software/MoonRenderer.cpp | 4 +- src/render/software/SoftwareRenderer.cpp | 2 + src/tests/DefinitionNode_Test.cpp | 44 +++++++++++------- src/tests/DefinitionWatcher_Test.cpp | 33 +++++++++++++ src/tests/DiffManager_Test.cpp | 18 +++++--- src/tests/TestToolDefinition.h | 6 ++- 37 files changed, 275 insertions(+), 154 deletions(-) create mode 100644 src/tests/DefinitionWatcher_Test.cpp diff --git a/src/definition/DefinitionNode.cpp b/src/definition/DefinitionNode.cpp index a45ae43..ecd9783 100644 --- a/src/definition/DefinitionNode.cpp +++ b/src/definition/DefinitionNode.cpp @@ -134,29 +134,6 @@ void DefinitionNode::generateInitDiffs(vector *diffs) co // TODO add children diffs in cascade ? } -void DefinitionNode::addWatcher(DefinitionWatcher *watcher, bool init_diff) { - if (root && root->diffs) { - if (init_diff) { - vector diffs; - generateInitDiffs(&diffs); - - for (auto diff : diffs) { - watcher->nodeChanged(this, diff, this); - delete diff; - } - } - root->diffs->addWatcher(this, watcher); - } -} - -unsigned long DefinitionNode::getWatcherCount() const { - if (root && root->diffs) { - return root->diffs->getWatcherCount(this); - } else { - return 0; - } -} - void DefinitionNode::save(PackStream *stream) const { int children_count = static_cast(children.size()); stream->write(&children_count); @@ -222,6 +199,7 @@ void DefinitionNode::copy(DefinitionNode *destination) const { } void DefinitionNode::validate() { + // TODO This should be deprecated in favor of onChildChange for (auto child : children) { child->validate(); } @@ -255,6 +233,18 @@ DefinitionNode *DefinitionNode::findChildByName(const string &name) const { return NULL; } +void DefinitionNode::tellChanged() { + if (parent) { + parent->onChildChanged(0, name); + } +} + +void DefinitionNode::onChildChanged(int depth, const string &relpath) { + if (parent) { + parent->onChildChanged(depth + 1, name + "/" + relpath); + } +} + int DefinitionNode::getStreamSize() const { return -1; } diff --git a/src/definition/DefinitionNode.h b/src/definition/DefinitionNode.h index f157fef..53c13b0 100644 --- a/src/definition/DefinitionNode.h +++ b/src/definition/DefinitionNode.h @@ -82,25 +82,26 @@ class DEFINITIONSHARED_EXPORT DefinitionNode { */ virtual void generateInitDiffs(vector *diffs) const; - /** - * Add a watcher over this node. - * - * The watcher will receive DefinitionDiff objects when this node changes. - * - * If 'init_diff' is set to true, a first diff (or several) will be be pushed immediately to initialize the state. - */ - void addWatcher(DefinitionWatcher *watcher, bool init_diff = false); - - /** - * Get the current number of watchers. - */ - unsigned long getWatcherCount() const; - protected: void addChild(DefinitionNode *child); void removeChild(DefinitionNode *child); virtual DefinitionNode *findChildByName(const string &name) const; + /** + * The subclass should call this method when the direct value of the node (not a child node) changed. + */ + void tellChanged(); + + /** + * Called when a child changed in the definition tree. + * + * The base implementation propagates the event up the definition tree. + * + * "depth" is the depth level the node is in (0 for a direct child, 1 for a grandchild...). + * "relpath" is the relative path of the changed node. + */ + virtual void onChildChanged(int depth, const string &relpath); + /** * Get the size in bytes this child will consume when serialized to a stream. * diff --git a/src/definition/DefinitionWatcher.cpp b/src/definition/DefinitionWatcher.cpp index 2577986..d22efc6 100644 --- a/src/definition/DefinitionWatcher.cpp +++ b/src/definition/DefinitionWatcher.cpp @@ -3,13 +3,26 @@ #include "IntDiff.h" #include "FloatDiff.h" #include "DefinitionNode.h" +#include "DiffManager.h" #include "Logs.h" -DefinitionWatcher::DefinitionWatcher() { +DefinitionWatcher::DefinitionWatcher(const string &name) : name(name) { } DefinitionWatcher::~DefinitionWatcher() { - // FIXME watcher is not removed from the diff manager ! + if (registered_to.size() > 0) { + Logs::error("Definition") + << "Watcher still registered at destruction, forcing potentially harmful unregister : " << name << endl; + unregister(); + } +} + +void DefinitionWatcher::unregister() { + set copy_registered_to = registered_to; + registered_to.clear(); + for (auto diff_manager : copy_registered_to) { + diff_manager->removeWatcher(this); + } } void DefinitionWatcher::nodeChanged(const DefinitionNode *node, const DefinitionDiff *diff, const DefinitionNode *) { @@ -30,15 +43,27 @@ void DefinitionWatcher::intNodeChanged(const string &, int, int) { void DefinitionWatcher::floatNodeChanged(const string &, double, double) { } -void DefinitionWatcher::startWatching(const DefinitionNode *root, const string &path, bool init_diff) { +void DefinitionWatcher::startWatchingPath(const DefinitionNode *root, const string &path, bool init_diff) { DefinitionNode *node = root->findByPath(path); if (node) { - node->addWatcher(this, init_diff); + startWatching(node, init_diff); } else { Logs::warning("Definition") << "Node not found for watching : " << path << endl; } } void DefinitionWatcher::startWatching(const DefinitionNode *node, bool init_diff) { - startWatching(node->getRoot(), node->getPath(), init_diff); + if (auto diff_manager = node->getRoot()->getDiffManager()) { + if (init_diff) { + vector diffs; + node->generateInitDiffs(&diffs); + + for (auto diff : diffs) { + nodeChanged(node, diff, node); + delete diff; + } + } + diff_manager->addWatcher(node, this); + registered_to.insert(diff_manager); + } } diff --git a/src/definition/DefinitionWatcher.h b/src/definition/DefinitionWatcher.h index 206352a..90fd237 100644 --- a/src/definition/DefinitionWatcher.h +++ b/src/definition/DefinitionWatcher.h @@ -4,6 +4,7 @@ #include "definition_global.h" #include +#include namespace paysages { namespace definition { @@ -15,9 +16,19 @@ namespace definition { */ class DEFINITIONSHARED_EXPORT DefinitionWatcher { public: - DefinitionWatcher(); + DefinitionWatcher(const string &name); virtual ~DefinitionWatcher(); + /** + * Unregister from all watched nodes. + * + * This MUST be called before destructor. + * + * A watcher should also make sure it is not unregistered as a direct result of a nodeChanged call, + * or this may cause a deadlock. + */ + void unregister(); + /** * Abstract method called when a node changed. * @@ -29,12 +40,10 @@ class DEFINITIONSHARED_EXPORT DefinitionWatcher { /** * Start watching a path in a definition tree. */ - void startWatching(const DefinitionNode *root, const string &path, bool init_diff = true); + void startWatchingPath(const DefinitionNode *root, const string &path, bool init_diff = true); /** * Start watching a node. - * - * Overloaded for convenience. */ void startWatching(const DefinitionNode *node, bool init_diff = true); @@ -47,6 +56,10 @@ class DEFINITIONSHARED_EXPORT DefinitionWatcher { * Abstract convenience to receive float node changes. */ virtual void floatNodeChanged(const string &path, double new_value, double old_value); + + private: + string name; + set registered_to; }; } } diff --git a/src/definition/DiffManager.cpp b/src/definition/DiffManager.cpp index 84398f6..973c130 100644 --- a/src/definition/DiffManager.cpp +++ b/src/definition/DiffManager.cpp @@ -3,6 +3,7 @@ #include #include #include +#include #include "DefinitionNode.h" #include "DefinitionDiff.h" #include "DefinitionWatcher.h" @@ -13,6 +14,7 @@ class DiffManager::pimpl { pimpl(DefinitionNode *tree) : tree(tree), undone(0) { } + recursive_mutex lock; DefinitionNode *tree; unsigned long undone; vector diffs; @@ -35,10 +37,21 @@ unsigned long DiffManager::getDiffCount(int include_undone) const { } void DiffManager::addWatcher(const DefinitionNode *node, DefinitionWatcher *watcher) { + impl->lock.lock(); auto &watchers = impl->watchers[node]; if (find(watchers.begin(), watchers.end(), watcher) == watchers.end()) { watchers.push_back(watcher); } + impl->lock.unlock(); +} + +void DiffManager::removeWatcher(DefinitionWatcher *watcher) { + impl->lock.lock(); + for (auto &item : impl->watchers) { + auto &node_watchers = item.second; + node_watchers.erase(remove(node_watchers.begin(), node_watchers.end(), watcher), node_watchers.end()); + } + impl->lock.unlock(); } unsigned long DiffManager::getWatcherCount(const DefinitionNode *node) { @@ -102,12 +115,16 @@ void DiffManager::redo() { } void DiffManager::publishToWatchers(const DefinitionNode *node, const DefinitionDiff *diff) { - // TODO Parent node signaling should be aggregated (to not receive many nodeChanged calls) + impl->lock.lock(); + const DefinitionNode *cnode = node; do { for (auto watcher : impl->watchers[cnode]) { watcher->nodeChanged(node, diff, cnode); } + // TODO Parent node signaling should be aggregated (to not receive many nodeChanged calls) cnode = cnode->getParent(); } while (cnode); + + impl->lock.unlock(); } diff --git a/src/definition/DiffManager.h b/src/definition/DiffManager.h index 5d6a504..0bb8cf3 100644 --- a/src/definition/DiffManager.h +++ b/src/definition/DiffManager.h @@ -30,6 +30,11 @@ class DEFINITIONSHARED_EXPORT DiffManager { */ void addWatcher(const DefinitionNode *node, DefinitionWatcher *watcher); + /** + * Remove a watcher from all nodes. + */ + void removeWatcher(DefinitionWatcher *watcher); + /** * Get the number of watchers registered for a given node. */ diff --git a/src/definition/FloatNode.cpp b/src/definition/FloatNode.cpp index 70f7a28..c216440 100644 --- a/src/definition/FloatNode.cpp +++ b/src/definition/FloatNode.cpp @@ -61,6 +61,7 @@ bool FloatNode::applyDiff(const DefinitionDiff *diff, bool backward) { if (value == previous) { value = next; + tellChanged(); return true; } else { Logs::error("Definition") << "Can't apply float diff " << previous << " => " << next << " to " << getName() diff --git a/src/definition/IntNode.cpp b/src/definition/IntNode.cpp index e676ced..9fb2e1a 100644 --- a/src/definition/IntNode.cpp +++ b/src/definition/IntNode.cpp @@ -61,6 +61,7 @@ bool IntNode::applyDiff(const DefinitionDiff *diff, bool backward) { if (value == previous) { value = next; + tellChanged(); return true; } else { Logs::error("Definition") << "Can't apply int diff " << previous << " => " << next << " to " << getName() diff --git a/src/definition/VegetationDefinition.cpp b/src/definition/VegetationDefinition.cpp index 7a4e002..bfeaf8c 100644 --- a/src/definition/VegetationDefinition.cpp +++ b/src/definition/VegetationDefinition.cpp @@ -32,6 +32,6 @@ void VegetationDefinition::applyPreset(VegetationPreset preset, RandomGenerator if (preset == VEGETATION_PRESET_TEMPERATE) { layer.applyPreset(VegetationLayerDefinition::VEGETATION_BASIC_TREES, random); layer.setName("Basic tree"); - //addLayer(layer); + // addLayer(layer); } } diff --git a/src/definition/WaterDefinition.cpp b/src/definition/WaterDefinition.cpp index ac6eb6c..0f13b5d 100644 --- a/src/definition/WaterDefinition.cpp +++ b/src/definition/WaterDefinition.cpp @@ -26,8 +26,6 @@ WaterDefinition::WaterDefinition(DefinitionNode *parent) : DefinitionNode(parent detail_height = 0.0; turbulence = 0.0; foam_coverage = 0.0; - - model->addWatcher(this, true); } WaterDefinition::~WaterDefinition() { @@ -79,18 +77,19 @@ void WaterDefinition::load(PackStream *stream) { void WaterDefinition::copy(DefinitionNode *_destination) const { DefinitionNode::copy(_destination); - WaterDefinition *destination = (WaterDefinition *)_destination; - *destination->material = *material; - destination->transparency_depth = transparency_depth; - destination->transparency = transparency; - destination->lighting_depth = lighting_depth; - destination->scaling = scaling; - destination->waves_height = waves_height; - destination->detail_height = detail_height; - destination->turbulence = turbulence; - destination->foam_coverage = foam_coverage; - *destination->foam_material = *foam_material; - noise_state->copy(destination->noise_state); + if (auto destination = static_cast(_destination)) { + *destination->material = *material; + destination->transparency_depth = transparency_depth; + destination->transparency = transparency; + destination->lighting_depth = lighting_depth; + destination->scaling = scaling; + destination->waves_height = waves_height; + destination->detail_height = detail_height; + destination->turbulence = turbulence; + destination->foam_coverage = foam_coverage; + *destination->foam_material = *foam_material; + noise_state->copy(destination->noise_state); + } } void WaterDefinition::validate() { @@ -109,8 +108,18 @@ void WaterDefinition::validate() { foam_material->validate(); } -void WaterDefinition::nodeChanged(const DefinitionNode *node, const DefinitionDiff *, const DefinitionNode *) { - if (node == model) { +void WaterDefinition::applyPreset(WaterPreset preset, RandomGenerator &random) { + noise_state->randomizeOffsets(random); + + if (preset == WATER_PRESET_LAKE) { + model->setValue(0); + } else if (preset == WATER_PRESET_SEA) { + model->setValue(1); + } +} + +void WaterDefinition::onChildChanged(int, const string &relpath) { + if (relpath == "model") { switch (model->getValue()) { case 1: transparency = 0.3; @@ -142,13 +151,3 @@ void WaterDefinition::nodeChanged(const DefinitionNode *node, const DefinitionDi } } } - -void WaterDefinition::applyPreset(WaterPreset preset, RandomGenerator &random) { - noise_state->randomizeOffsets(random); - - if (preset == WATER_PRESET_LAKE) { - model->setValue(0); - } else if (preset == WATER_PRESET_SEA) { - model->setValue(1); - } -} diff --git a/src/definition/WaterDefinition.h b/src/definition/WaterDefinition.h index a476e3c..2563d01 100644 --- a/src/definition/WaterDefinition.h +++ b/src/definition/WaterDefinition.h @@ -9,7 +9,7 @@ namespace paysages { namespace definition { -class DEFINITIONSHARED_EXPORT WaterDefinition : public DefinitionNode, public DefinitionWatcher { +class DEFINITIONSHARED_EXPORT WaterDefinition : public DefinitionNode { public: WaterDefinition(DefinitionNode *parent); virtual ~WaterDefinition(); @@ -36,12 +36,12 @@ class DEFINITIONSHARED_EXPORT WaterDefinition : public DefinitionNode, public De return depth_color; } - virtual void nodeChanged(const DefinitionNode *node, const DefinitionDiff *diff, - const DefinitionNode *parent) override; - typedef enum { WATER_PRESET_LAKE, WATER_PRESET_SEA } WaterPreset; void applyPreset(WaterPreset preset, RandomGenerator &random = RandomGeneratorDefault); + protected: + virtual void onChildChanged(int depth, const string &relpath) override; + public: double transparency; SurfaceMaterial *material; diff --git a/src/interface/modeler/AtmosphereModeler.h b/src/interface/modeler/AtmosphereModeler.h index e65781d..6656a09 100644 --- a/src/interface/modeler/AtmosphereModeler.h +++ b/src/interface/modeler/AtmosphereModeler.h @@ -8,7 +8,7 @@ namespace paysages { namespace modeler { -class AtmosphereModeler : protected BaseModelerTool { +class AtmosphereModeler : public BaseModelerTool { public: AtmosphereModeler(MainModelerWindow *ui); diff --git a/src/interface/modeler/BaseModelerTool.cpp b/src/interface/modeler/BaseModelerTool.cpp index 0de7811..51d006d 100644 --- a/src/interface/modeler/BaseModelerTool.cpp +++ b/src/interface/modeler/BaseModelerTool.cpp @@ -14,19 +14,30 @@ class BaseModelerTool::pimpl { vector> float_bindings; }; -BaseModelerTool::BaseModelerTool(MainModelerWindow *ui) : impl(new pimpl), ui(ui) { +BaseModelerTool::BaseModelerTool(MainModelerWindow *ui) + : DefinitionWatcher("BaseModelerTool"), impl(new pimpl), ui(ui) { } BaseModelerTool::~BaseModelerTool() { } +void BaseModelerTool::destroy() { + for (auto &binding : impl->int_bindings) { + binding->unregister(); + } + for (auto &binding : impl->float_bindings) { + binding->unregister(); + } + unregister(); +} + void BaseModelerTool::addIntBinding(const string &object, const string &property, const string &path, bool monitor) { auto node = static_cast(ui->getScenery()->findByPath(path)); if (node) { impl->int_bindings.push_back(make_unique(ui, object, property, node)); if (monitor) { - startWatching(ui->getScenery(), path, false); + startWatchingPath(ui->getScenery(), path, false); } } else { Logs::error("UI") << "Can't find int node for binding : " << path << endl; @@ -39,7 +50,7 @@ void BaseModelerTool::addFloatBinding(const string &object, const string &proper impl->float_bindings.push_back(make_unique(ui, object, property, node)); if (monitor) { - startWatching(ui->getScenery(), path, false); + startWatchingPath(ui->getScenery(), path, false); } } else { Logs::error("UI") << "Can't find float node for binding : " << path << endl; diff --git a/src/interface/modeler/BaseModelerTool.h b/src/interface/modeler/BaseModelerTool.h index 29152e8..7d96328 100644 --- a/src/interface/modeler/BaseModelerTool.h +++ b/src/interface/modeler/BaseModelerTool.h @@ -10,11 +10,16 @@ namespace paysages { namespace modeler { -class BaseModelerTool : public DefinitionWatcher { +class BaseModelerTool : protected DefinitionWatcher { public: BaseModelerTool(MainModelerWindow *ui); virtual ~BaseModelerTool(); + /** + * Call before destructor, to allow for proper release of resources. + */ + virtual void destroy(); + /** * Add an automated two-way binding between a QML int property and a scenery IntNode. * diff --git a/src/interface/modeler/FloatPropertyBind.cpp b/src/interface/modeler/FloatPropertyBind.cpp index 8805a69..2fda3f7 100644 --- a/src/interface/modeler/FloatPropertyBind.cpp +++ b/src/interface/modeler/FloatPropertyBind.cpp @@ -8,10 +8,11 @@ FloatPropertyBind::FloatPropertyBind(MainModelerWindow *window, const string &object_name, const string &property_name, FloatNode *node) - : QObject(window), node(node), property(property_name) { + : QObject(window), DefinitionWatcher("FloatPropertyBind " + object_name + "." + property_name), node(node), + property(property_name) { item = window->findQmlObject(object_name); if (item) { - node->addWatcher(this, true); + startWatching(node); string signal_name("2" + property_name + "Changed()"); connect(item, signal_name.c_str(), this, SLOT(propertyChanged())); } else { diff --git a/src/interface/modeler/IntPropertyBind.cpp b/src/interface/modeler/IntPropertyBind.cpp index c8d95bb..6b71d19 100644 --- a/src/interface/modeler/IntPropertyBind.cpp +++ b/src/interface/modeler/IntPropertyBind.cpp @@ -6,10 +6,11 @@ IntPropertyBind::IntPropertyBind(MainModelerWindow *window, const string &object_name, const string &property_name, IntNode *node) - : QObject(window), node(node), property(property_name) { + : QObject(window), DefinitionWatcher("IntPropertyBind " + object_name + "." + property_name), node(node), + property(property_name) { item = window->findQmlObject(object_name); if (item) { - node->addWatcher(this, true); + startWatching(node); string signal_name("2" + property_name + "Changed()"); connect(item, signal_name.c_str(), this, SLOT(propertyChanged())); } else { diff --git a/src/interface/modeler/IntPropertyBind.h b/src/interface/modeler/IntPropertyBind.h index 26fad36..d6abf70 100644 --- a/src/interface/modeler/IntPropertyBind.h +++ b/src/interface/modeler/IntPropertyBind.h @@ -27,8 +27,8 @@ class IntPropertyBind : public QObject, public DefinitionWatcher { private: IntNode *node; - string property; QObject *item; + string property; }; } } diff --git a/src/interface/modeler/MainModelerWindow.cpp b/src/interface/modeler/MainModelerWindow.cpp index ad190d0..0854146 100644 --- a/src/interface/modeler/MainModelerWindow.cpp +++ b/src/interface/modeler/MainModelerWindow.cpp @@ -49,6 +49,11 @@ MainModelerWindow::MainModelerWindow() { } MainModelerWindow::~MainModelerWindow() { + // TODO Should be done automatically for all tools + cameras->unregister(); + atmosphere->destroy(); + water->destroy(); + delete atmosphere; delete water; delete cameras; diff --git a/src/interface/modeler/ModelerCameras.cpp b/src/interface/modeler/ModelerCameras.cpp index c80935c..911dff4 100644 --- a/src/interface/modeler/ModelerCameras.cpp +++ b/src/interface/modeler/ModelerCameras.cpp @@ -9,7 +9,8 @@ #include "AtmosphereRenderer.h" #include "Timing.h" -ModelerCameras::ModelerCameras(MainModelerWindow *parent) : QObject(parent), parent(parent) { +ModelerCameras::ModelerCameras(MainModelerWindow *parent) + : QObject(parent), DefinitionWatcher("ModelerCameras"), parent(parent) { render = new CameraDefinition(); topdown = new CameraDefinition(); current = new CameraDefinition(); diff --git a/src/interface/modeler/WaterModeler.cpp b/src/interface/modeler/WaterModeler.cpp index db703c8..0903af1 100644 --- a/src/interface/modeler/WaterModeler.cpp +++ b/src/interface/modeler/WaterModeler.cpp @@ -9,24 +9,17 @@ #include "OpenGLRenderer.h" #include "OpenGLWater.h" -WaterModeler::WaterModeler(MainModelerWindow *ui) : ui(ui) { +WaterModeler::WaterModeler(MainModelerWindow *ui) : BaseModelerTool(ui) { QObject *toggle_water = ui->findQmlObject("camera_toggle_water"); if (toggle_water) { connect(toggle_water, SIGNAL(toggled(bool)), this, SLOT(enableRendering(bool))); } - prop_model = new IntPropertyBind(ui, "water_model", "value", ui->getScenery()->getWater()->propModel()); - prop_height = new FloatPropertyBind(ui, "water_height", "value", ui->getScenery()->getTerrain()->propWaterHeight()); - prop_reflexion = - new FloatPropertyBind(ui, "water_reflection", "value", ui->getScenery()->getWater()->propReflection()); -} - -WaterModeler::~WaterModeler() { - delete prop_model; - delete prop_height; - delete prop_reflexion; + addIntBinding("water_model", "value", "/water/model"); + addFloatBinding("water_height", "value", "/terrain/water_height"); + addFloatBinding("water_reflection", "value", "/water/reflection"); } void WaterModeler::enableRendering(bool enable) { - ui->getRenderer()->getWater()->setEnabled(enable); + getWindow()->getRenderer()->getWater()->setEnabled(enable); } diff --git a/src/interface/modeler/WaterModeler.h b/src/interface/modeler/WaterModeler.h index 95b38bb..99bd6b8 100644 --- a/src/interface/modeler/WaterModeler.h +++ b/src/interface/modeler/WaterModeler.h @@ -1,27 +1,21 @@ #ifndef WATERMODELER_H #define WATERMODELER_H -#include - #include "modeler_global.h" +#include "BaseModelerTool.h" +#include + namespace paysages { namespace modeler { -class WaterModeler : public QObject { +class WaterModeler : public QObject, public BaseModelerTool { Q_OBJECT public: WaterModeler(MainModelerWindow *ui); - ~WaterModeler(); public slots: void enableRendering(bool enable); - - private: - MainModelerWindow *ui; - IntPropertyBind *prop_model; - FloatPropertyBind *prop_height; - FloatPropertyBind *prop_reflexion; }; } } diff --git a/src/render/opengl/OpenGLPart.cpp b/src/render/opengl/OpenGLPart.cpp index fd08603..2ec5c9c 100644 --- a/src/render/opengl/OpenGLPart.cpp +++ b/src/render/opengl/OpenGLPart.cpp @@ -4,7 +4,8 @@ #include "OpenGLShaderProgram.h" #include "OpenGLVertexArray.h" -OpenGLPart::OpenGLPart(OpenGLRenderer *renderer, const string &name) : renderer(renderer), name(name) { +OpenGLPart::OpenGLPart(OpenGLRenderer *renderer, const string &name) + : DefinitionWatcher("OpenGLPart/" + name), renderer(renderer), name(name) { } OpenGLPart::~OpenGLPart() { @@ -25,6 +26,7 @@ void OpenGLPart::destroy() { for (auto array : arrays) { array->destroy(functions); } + unregister(); } void OpenGLPart::interrupt() { diff --git a/src/render/opengl/OpenGLPart.h b/src/render/opengl/OpenGLPart.h index 610845a..ad7faba 100644 --- a/src/render/opengl/OpenGLPart.h +++ b/src/render/opengl/OpenGLPart.h @@ -3,6 +3,8 @@ #include "opengl_global.h" +#include "DefinitionWatcher.h" + #include #include @@ -12,7 +14,7 @@ namespace opengl { /** * Class that can be inherited by scenery parts, to use OpenGL features. */ -class OPENGLSHARED_EXPORT OpenGLPart { +class OPENGLSHARED_EXPORT OpenGLPart : protected DefinitionWatcher { public: OpenGLPart(OpenGLRenderer *renderer, const string &name); virtual ~OpenGLPart(); diff --git a/src/render/opengl/OpenGLSkybox.cpp b/src/render/opengl/OpenGLSkybox.cpp index f55bf28..6ef1748 100644 --- a/src/render/opengl/OpenGLSkybox.cpp +++ b/src/render/opengl/OpenGLSkybox.cpp @@ -58,10 +58,10 @@ OpenGLSkybox::~OpenGLSkybox() { void OpenGLSkybox::initialize() { // Watch for definition changes Scenery *scenery = renderer->getScenery(); - startWatching(scenery, path_sun_phi); - startWatching(scenery, path_sun_theta); - startWatching(scenery, path_sun_radius); - startWatching(scenery, path_humidity); + startWatchingPath(scenery, path_sun_phi); + startWatchingPath(scenery, path_sun_theta); + startWatchingPath(scenery, path_sun_radius); + startWatchingPath(scenery, path_humidity); } void OpenGLSkybox::update() { diff --git a/src/render/opengl/OpenGLSkybox.h b/src/render/opengl/OpenGLSkybox.h index 1feed9a..005ad7e 100644 --- a/src/render/opengl/OpenGLSkybox.h +++ b/src/render/opengl/OpenGLSkybox.h @@ -4,12 +4,11 @@ #include "opengl_global.h" #include "OpenGLPart.h" -#include "DefinitionWatcher.h" namespace paysages { namespace opengl { -class OPENGLSHARED_EXPORT OpenGLSkybox : public OpenGLPart, public DefinitionWatcher { +class OPENGLSHARED_EXPORT OpenGLSkybox : public OpenGLPart { public: OpenGLSkybox(OpenGLRenderer *renderer); virtual ~OpenGLSkybox(); diff --git a/src/render/opengl/OpenGLTerrain.cpp b/src/render/opengl/OpenGLTerrain.cpp index cd9abfe..9d1fb3e 100644 --- a/src/render/opengl/OpenGLTerrain.cpp +++ b/src/render/opengl/OpenGLTerrain.cpp @@ -117,6 +117,7 @@ void OpenGLTerrain::destroy() { for (auto &chunk : pv->chunks) { chunk->destroy(functions); } + OpenGLPart::destroy(); } void OpenGLTerrain::pause() { diff --git a/src/render/opengl/OpenGLTerrain.h b/src/render/opengl/OpenGLTerrain.h index d324e8f..48385be 100644 --- a/src/render/opengl/OpenGLTerrain.h +++ b/src/render/opengl/OpenGLTerrain.h @@ -4,14 +4,13 @@ #include "opengl_global.h" #include "OpenGLPart.h" -#include "DefinitionWatcher.h" namespace paysages { namespace opengl { class OpenGLTerrainPV; -class OPENGLSHARED_EXPORT OpenGLTerrain : public OpenGLPart, public DefinitionWatcher { +class OPENGLSHARED_EXPORT OpenGLTerrain : public OpenGLPart { public: OpenGLTerrain(OpenGLRenderer *renderer); virtual ~OpenGLTerrain(); diff --git a/src/render/opengl/OpenGLVegetation.cpp b/src/render/opengl/OpenGLVegetation.cpp index c2a2ebe..6a3643b 100644 --- a/src/render/opengl/OpenGLVegetation.cpp +++ b/src/render/opengl/OpenGLVegetation.cpp @@ -46,7 +46,7 @@ OpenGLVegetation::OpenGLVegetation(OpenGLRenderer *renderer) : OpenGLPart(render enabled = true; // Watch scenery changes - renderer->getScenery()->getVegetation()->addWatcher(this, true); + startWatching(renderer->getScenery()->getVegetation()); } OpenGLVegetation::~OpenGLVegetation() { diff --git a/src/render/opengl/OpenGLVegetation.h b/src/render/opengl/OpenGLVegetation.h index c68bb05..db76de5 100644 --- a/src/render/opengl/OpenGLVegetation.h +++ b/src/render/opengl/OpenGLVegetation.h @@ -4,7 +4,6 @@ #include "opengl_global.h" #include "OpenGLPart.h" -#include "DefinitionWatcher.h" #include @@ -13,7 +12,7 @@ namespace opengl { class VegetationUpdater; -class OPENGLSHARED_EXPORT OpenGLVegetation : public OpenGLPart, public DefinitionWatcher { +class OPENGLSHARED_EXPORT OpenGLVegetation : public OpenGLPart { public: OpenGLVegetation(OpenGLRenderer *renderer); virtual ~OpenGLVegetation(); diff --git a/src/render/opengl/OpenGLWater.cpp b/src/render/opengl/OpenGLWater.cpp index 6419f0d..cbfa12e 100644 --- a/src/render/opengl/OpenGLWater.cpp +++ b/src/render/opengl/OpenGLWater.cpp @@ -43,9 +43,9 @@ OpenGLWater::~OpenGLWater() { void OpenGLWater::initialize() { // Watch for definition changes Scenery *scenery = renderer->getScenery(); - startWatching(scenery, path_height); - startWatching(scenery, path_reflection); - startWatching(scenery, path_model, false); + startWatchingPath(scenery, path_height); + startWatchingPath(scenery, path_reflection); + startWatchingPath(scenery, path_model, false); } void OpenGLWater::update() { diff --git a/src/render/opengl/OpenGLWater.h b/src/render/opengl/OpenGLWater.h index 6fcef3b..26617a4 100644 --- a/src/render/opengl/OpenGLWater.h +++ b/src/render/opengl/OpenGLWater.h @@ -4,12 +4,11 @@ #include "opengl_global.h" #include "OpenGLPart.h" -#include "DefinitionWatcher.h" namespace paysages { namespace opengl { -class OPENGLSHARED_EXPORT OpenGLWater : public OpenGLPart, public DefinitionWatcher { +class OPENGLSHARED_EXPORT OpenGLWater : public OpenGLPart { public: OpenGLWater(OpenGLRenderer *renderer); virtual ~OpenGLWater(); diff --git a/src/render/software/MoonRenderer.cpp b/src/render/software/MoonRenderer.cpp index f09dcad..6e60a34 100644 --- a/src/render/software/MoonRenderer.cpp +++ b/src/render/software/MoonRenderer.cpp @@ -19,8 +19,8 @@ class MoonRenderer::pimpl { NoiseFunctionSimplex noise; }; -MoonRenderer::MoonRenderer(CelestialBodyDefinition *moon_node) : impl(new pimpl()) { - startWatching(moon_node->getRoot(), moon_node->getPath()); +MoonRenderer::MoonRenderer(CelestialBodyDefinition *moon_node) : DefinitionWatcher("MoonRenderer"), impl(new pimpl()) { + startWatchingPath(moon_node->getRoot(), moon_node->getPath()); } MoonRenderer::~MoonRenderer() { diff --git a/src/render/software/SoftwareRenderer.cpp b/src/render/software/SoftwareRenderer.cpp index ee59d17..13f5d03 100644 --- a/src/render/software/SoftwareRenderer.cpp +++ b/src/render/software/SoftwareRenderer.cpp @@ -55,6 +55,8 @@ SoftwareRenderer::SoftwareRenderer(Scenery *scenery) : scenery(scenery) { } SoftwareRenderer::~SoftwareRenderer() { + moon_renderer->unregister(); + delete render_camera; delete fluid_medium; diff --git a/src/tests/DefinitionNode_Test.cpp b/src/tests/DefinitionNode_Test.cpp index 3bd8a47..bd79349 100644 --- a/src/tests/DefinitionNode_Test.cpp +++ b/src/tests/DefinitionNode_Test.cpp @@ -1,6 +1,7 @@ #include "BaseTestCase.h" #include "DefinitionNode.h" +#include "IntNode.h" #include "FloatNode.h" #include "PackStream.h" #include "DefinitionWatcher.h" @@ -25,22 +26,6 @@ TEST(DefinitionNode, getPath) { EXPECT_EQ("/branch/leaf", leaf.getPath()); } -TEST(DefinitionNode, addWatcher) { - class FakeWatcher : public DefinitionWatcher { - virtual void nodeChanged(const DefinitionNode *, const DefinitionDiff *, const DefinitionNode *) override { - } - }; - - DefinitionNode root(NULL, "root"); - FakeWatcher watcher; - - EXPECT_EQ(0u, root.getWatcherCount()); - root.addWatcher(&watcher); - EXPECT_EQ(1u, root.getWatcherCount()); - root.addWatcher(&watcher); - EXPECT_EQ(1u, root.getWatcherCount()); -} - TEST(DefinitionNode, findByPath) { DefinitionNode root(NULL, "root"); DefinitionNode branch(&root, "branch"); @@ -132,3 +117,30 @@ TEST(DefinitionNode, saveLoadCopy) { delete before; delete after; } + +TEST(DefinitionNode, onChildChange) { + class CapturingNode : public DefinitionNode { + public: + CapturingNode(DefinitionNode *parent, const string &name) : DefinitionNode(parent, name) { + } + virtual void onChildChanged(int depth, const string &relpath) override { + changes.push_back(pair(depth, relpath)); + DefinitionNode::onChildChanged(depth, relpath); + } + vector> changes; + }; + + CapturingNode root(NULL, "root"); + CapturingNode branch(&root, "branch"); + IntNode leaf(&branch, "leaf"); + + leaf.setValue(5.0); + + ASSERT_EQ(1u, root.changes.size()); + EXPECT_EQ(1, root.changes[0].first); + EXPECT_EQ("branch/leaf", root.changes[0].second); + + ASSERT_EQ(1u, branch.changes.size()); + EXPECT_EQ(0, branch.changes[0].first); + EXPECT_EQ("leaf", branch.changes[0].second); +} diff --git a/src/tests/DefinitionWatcher_Test.cpp b/src/tests/DefinitionWatcher_Test.cpp new file mode 100644 index 0000000..b033731 --- /dev/null +++ b/src/tests/DefinitionWatcher_Test.cpp @@ -0,0 +1,33 @@ +#include "BaseTestCase.h" +#include "DefinitionWatcher.h" + +#include "IntNode.h" + +TEST(DefinitionWatcher, destructor) { + class FakeWatcher : public DefinitionWatcher { + public: + FakeWatcher(DefinitionNode *node, int &counter) : DefinitionWatcher("FakeWatcher"), counter(counter) { + startWatching(node, false); + } + + virtual void nodeChanged(const DefinitionNode *, const DefinitionDiff *, const DefinitionNode *) override { + counter++; + } + + int &counter; + }; + + IntNode root(NULL, "test", 5); + int counter = 0; + auto watcher = new FakeWatcher(&root, counter); + + EXPECT_EQ(0, counter); + + root.setValue(3); + EXPECT_EQ(1, counter); + + // When the watcher is destroyed, it should stop receiving diffs + delete watcher; + root.setValue(8); + EXPECT_EQ(1, counter); +} diff --git a/src/tests/DiffManager_Test.cpp b/src/tests/DiffManager_Test.cpp index ffb80dc..f532b1d 100644 --- a/src/tests/DiffManager_Test.cpp +++ b/src/tests/DiffManager_Test.cpp @@ -111,18 +111,23 @@ TEST(DiffManager, undoBranch) { EXPECT_DOUBLE_EQ(4.4, leaf.getValue()); } +namespace { class TestWatcher : public DefinitionWatcher { public: TestWatcher(DefinitionNode *expected_node, double expected_old_value, double expected_new_value) - : DefinitionWatcher(), expected_node(expected_node), expected_old_value(expected_old_value), + : DefinitionWatcher("TestWatcher"), expected_node(expected_node), expected_old_value(expected_old_value), expected_new_value(expected_new_value) { calls = 0; } + void start(bool init_diffs) { + startWatching(expected_node, init_diffs); + } + virtual void nodeChanged(const DefinitionNode *node, const DefinitionDiff *diff, const DefinitionNode *) override { EXPECT_EQ(expected_node, node); ASSERT_EQ("float", diff->getTypeName()); - const FloatDiff *float_diff = (const FloatDiff *)diff; + auto float_diff = static_cast(diff); EXPECT_EQ(expected_old_value, float_diff->getOldValue()); EXPECT_EQ(expected_new_value, float_diff->getNewValue()); calls++; @@ -133,6 +138,7 @@ class TestWatcher : public DefinitionWatcher { double expected_old_value; double expected_new_value; }; +} TEST(DiffManager, addWatcher) { FloatNode node(NULL, "node"); @@ -141,7 +147,7 @@ TEST(DiffManager, addWatcher) { node.setValue(1.3); EXPECT_EQ(0, watcher.calls); - node.addWatcher(&watcher); + watcher.start(false); EXPECT_EQ(0, watcher.calls); node.setValue(-4.0); @@ -152,7 +158,7 @@ TEST(DiffManager, addWatcherWithInitDiffs) { FloatNode node(NULL, "node", 1.3); TestWatcher watcher(&node, 1.3, 1.3); - node.addWatcher(&watcher, true); + watcher.start(true); EXPECT_EQ(1, watcher.calls); } @@ -163,12 +169,12 @@ TEST(DiffManager, publishToWatcher) { EXPECT_EQ(0u, watcher.changes.size()); - root.addWatcher(&watcher, true); + watcher.start(&root); ASSERT_EQ(1u, watcher.changes.size()); EXPECT_EQ(&root, watcher.changes[0].node); EXPECT_EQ(&root, watcher.changes[0].parent); - node.addWatcher(&watcher, true); + watcher.start(&node); ASSERT_EQ(2u, watcher.changes.size()); EXPECT_EQ(&node, watcher.changes[1].node); EXPECT_EQ(&node, watcher.changes[1].parent); diff --git a/src/tests/TestToolDefinition.h b/src/tests/TestToolDefinition.h index 8af53f3..6b91f68 100644 --- a/src/tests/TestToolDefinition.h +++ b/src/tests/TestToolDefinition.h @@ -11,7 +11,7 @@ namespace { */ class RecordingDefinitionWatcher : public DefinitionWatcher { public: - RecordingDefinitionWatcher() { + RecordingDefinitionWatcher() : DefinitionWatcher("RecordingDefinitionWatcher") { } virtual void nodeChanged(const DefinitionNode *node, const DefinitionDiff *diff, @@ -23,6 +23,10 @@ class RecordingDefinitionWatcher : public DefinitionWatcher { changes.push_back(change); } + virtual void start(DefinitionNode *node, bool init_diffs = true) { + startWatching(node, init_diffs); + } + typedef struct { const DefinitionNode *node; const DefinitionDiff *diff;