Fixed DefinitionWatcher receiving changes after destruction

src/definition/DefinitionNode.cpp

@@ -134,29 +134,6 @@ void DefinitionNode::generateInitDiffs(vector<const DefinitionDiff *> *diffs) co
     // TODO add children diffs in cascade ?
 }
 
-void DefinitionNode::addWatcher(DefinitionWatcher *watcher, bool init_diff) {
-    if (root && root->diffs) {
-        if (init_diff) {
-            vector<const DefinitionDiff *> 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<int>(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;
 }

src/definition/DefinitionNode.h

@@ -82,25 +82,26 @@ class DEFINITIONSHARED_EXPORT DefinitionNode {
      */
     virtual void generateInitDiffs(vector<const DefinitionDiff *> *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.
      *

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<DiffManager *> 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<const DefinitionDiff *> diffs;
+            node->generateInitDiffs(&diffs);
+
+            for (auto diff : diffs) {
+                nodeChanged(node, diff, node);
+                delete diff;
+            }
+        }
+        diff_manager->addWatcher(node, this);
+        registered_to.insert(diff_manager);
+    }
 }

src/definition/DefinitionWatcher.h

@@ -4,6 +4,7 @@
 #include "definition_global.h"
 
 #include <string>
+#include <set>
 
 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<DiffManager *> registered_to;
 };
 }
 }

src/definition/DiffManager.cpp

@@ -3,6 +3,7 @@
 #include <algorithm>
 #include <map>
 #include <vector>
+#include <mutex>
 #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<const DefinitionDiff *> 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();
 }

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.
      */

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()

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()

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);
     }
 }

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<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);
+    }
 }
 
 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);
-    }
-}

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;

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);
 

src/interface/modeler/BaseModelerTool.cpp

@@ -14,19 +14,30 @@ class BaseModelerTool::pimpl {
     vector<unique_ptr<FloatPropertyBind>> 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<IntNode *>(ui->getScenery()->findByPath(path));
     if (node) {
         impl->int_bindings.push_back(make_unique<IntPropertyBind>(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<FloatPropertyBind>(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;

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.
      *

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 {

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 {

src/interface/modeler/IntPropertyBind.h

@@ -27,8 +27,8 @@ class IntPropertyBind : public QObject, public DefinitionWatcher {
 
   private:
     IntNode *node;
-    string property;
     QObject *item;
+    string property;
 };
 }
 }

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;

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();

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);
 }

src/interface/modeler/WaterModeler.h

@@ -1,27 +1,21 @@
 #ifndef WATERMODELER_H
 #define WATERMODELER_H
 
-#include <QObject>
-
 #include "modeler_global.h"
 
+#include "BaseModelerTool.h"
+#include <QObject>
+
 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;
 };
 }
 }

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() {

src/render/opengl/OpenGLPart.h

@@ -3,6 +3,8 @@
 
 #include "opengl_global.h"
 
+#include "DefinitionWatcher.h"
+
 #include <map>
 #include <vector>
 
@@ -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();

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() {

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();

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() {

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();

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() {

src/render/opengl/OpenGLVegetation.h

@@ -4,7 +4,6 @@
 #include "opengl_global.h"
 
 #include "OpenGLPart.h"
-#include "DefinitionWatcher.h"
 
 #include <vector>
 
@@ -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();

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() {

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();

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() {

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;

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<int, string>(depth, relpath));
+            DefinitionNode::onChildChanged(depth, relpath);
+        }
+        vector<pair<int, string>> 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);
+}

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);
+}

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<const FloatDiff *>(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);

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;