Added node watching on "container" nodes

src/definition/DefinitionNode.cpp

@@ -9,6 +9,20 @@
 #include <cassert>
 #include <algorithm>
 
+// Diff for abstract nodes
+class DefinitionNodeDiff : public DefinitionDiff {
+  public:
+    DefinitionNodeDiff(const DefinitionNode *node) : DefinitionDiff(node) {
+    }
+
+    DefinitionNodeDiff(const DefinitionDiff *diff) : DefinitionDiff(diff) {
+    }
+
+    virtual DefinitionDiff *newReversed() const override {
+        return new DefinitionNodeDiff(this);
+    }
+};
+
 DefinitionNode::DefinitionNode(DefinitionNode *parent, const string &name, const string &type_name)
     : parent(parent), type_name(type_name), name(name) {
     if (parent) {
@@ -115,7 +129,9 @@ bool DefinitionNode::applyDiff(const DefinitionDiff *diff, bool) {
     }
 }
 
-void DefinitionNode::generateInitDiffs(vector<const DefinitionDiff *> *) const {
+void DefinitionNode::generateInitDiffs(vector<const DefinitionDiff *> *diffs) const {
+    diffs->push_back(new DefinitionNodeDiff(this));
+    // TODO add children diffs in cascade ?
 }
 
 void DefinitionNode::addWatcher(DefinitionWatcher *watcher, bool init_diff) {
@@ -125,7 +141,7 @@ void DefinitionNode::addWatcher(DefinitionWatcher *watcher, bool init_diff) {
             generateInitDiffs(&diffs);
 
             for (auto diff : diffs) {
-                watcher->nodeChanged(this, diff);
+                watcher->nodeChanged(this, diff, this);
                 delete diff;
             }
         }

src/definition/DefinitionWatcher.cpp

@@ -9,9 +9,10 @@ DefinitionWatcher::DefinitionWatcher() {
 }
 
 DefinitionWatcher::~DefinitionWatcher() {
+    // FIXME watcher is not removed from the diff manager !
 }
 
-void DefinitionWatcher::nodeChanged(const DefinitionNode *node, const DefinitionDiff *diff) {
+void DefinitionWatcher::nodeChanged(const DefinitionNode *node, const DefinitionDiff *diff, const DefinitionNode *) {
     string type_name = node->getTypeName();
 
     if (type_name == "int") {

src/definition/DefinitionWatcher.h

@@ -20,8 +20,10 @@ class DEFINITIONSHARED_EXPORT DefinitionWatcher {
 
     /**
      * Abstract method called when a node changed.
+     *
+     * *parent* is the node that is watched (useful if *node* is a sub-node).
      */
-    virtual void nodeChanged(const DefinitionNode *node, const DefinitionDiff *diff);
+    virtual void nodeChanged(const DefinitionNode *node, const DefinitionDiff *diff, const DefinitionNode *parent);
 
   protected:
     /**

src/definition/DiffManager.cpp

@@ -39,11 +39,10 @@ void DiffManager::addDiff(DefinitionNode *node, const DefinitionDiff *diff) {
     diffs.push_back(diff);
 
     // TODO Delayed commit (with merge of consecutive diffs)
-    node->applyDiff(diff);
 
-    for (auto watcher : watchers[node]) {
+    Logs::debug("Definition") << "Node changed : " << node->getPath() << endl;
-        watcher->nodeChanged(node, diff);
+    node->applyDiff(diff);
-    }
+    publishToWatchers(node, diff);
 }
 
 void DiffManager::undo() {
@@ -55,12 +54,12 @@ void DiffManager::undo() {
         if (node) {
             undone++;
 
-            node->applyDiff(diff, true);
+            unique_ptr<DefinitionDiff> reversed(diff->newReversed());
 
-            for (auto watcher : watchers[node]) {
+            Logs::debug("Definition") << "Node undo : " << node->getPath() << endl;
-                unique_ptr<DefinitionDiff> reversed(diff->newReversed());
+            // TODO use reversed ?
-                watcher->nodeChanged(node, reversed.get());
+            node->applyDiff(diff, true);
-            }
+            publishToWatchers(node, reversed.get());
         } else {
             Logs::error("Definition") << "Can't find node to undo diff : " << diff->getPath() << endl;
         }
@@ -76,13 +75,22 @@ void DiffManager::redo() {
         if (node) {
             undone--;
 
+            Logs::debug("Definition") << "Node redo : " << node->getPath() << endl;
             node->applyDiff(diff);
-
+            publishToWatchers(node, diff);
-            for (auto watcher : watchers[node]) {
-                watcher->nodeChanged(node, diff);
-            }
         } else {
             Logs::error("Definition") << "Can't find node to redo diff : " << diff->getPath() << endl;
         }
     }
 }
+
+void DiffManager::publishToWatchers(const DefinitionNode *node, const DefinitionDiff *diff) {
+    // TODO Parent node signaling should be aggregated (to not receive many nodeChanged calls)
+    const DefinitionNode *cnode = node;
+    do {
+        for (auto watcher : watchers[cnode]) {
+            watcher->nodeChanged(node, diff, cnode);
+        }
+        cnode = cnode->getParent();
+    } while (cnode);
+}

src/definition/DiffManager.h

@@ -53,6 +53,15 @@ class DEFINITIONSHARED_EXPORT DiffManager {
      */
     void redo();
 
+protected:
+    /**
+     * Publish a diff to the registered watchers.
+     *
+     * The diff will be published to the watcher of the node, and to the watchers of all
+     * parents, up to the definition tree root.
+     */
+    void publishToWatchers(const DefinitionNode *node, const DefinitionDiff *diff);
+
   private:
     DefinitionNode *tree;
     int undone;

src/definition/WaterDefinition.cpp

@@ -109,7 +109,7 @@ void WaterDefinition::validate() {
     foam_material->validate();
 }
 
-void WaterDefinition::nodeChanged(const DefinitionNode *node, const DefinitionDiff *) {
+void WaterDefinition::nodeChanged(const DefinitionNode *node, const DefinitionDiff *, const DefinitionNode *) {
     if (node == model) {
         switch (model->getValue()) {
         case 1:

src/definition/WaterDefinition.h

@@ -36,7 +36,7 @@ class DEFINITIONSHARED_EXPORT WaterDefinition : public DefinitionNode, public De
         return depth_color;
     }
 
-    virtual void nodeChanged(const DefinitionNode *node, const DefinitionDiff *diff);
+    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);

src/interface/modeler/AtmosphereModeler.cpp

@@ -18,7 +18,7 @@ AtmosphereModeler::AtmosphereModeler(MainModelerWindow *ui) : BaseModelerTool(ui
     ui->setQmlProperty("atmosphere_daytime", "value", ui->getScenery()->getAtmosphere()->getDaytime());
 }
 
-void AtmosphereModeler::nodeChanged(const DefinitionNode *node, const DefinitionDiff *) {
+void AtmosphereModeler::nodeChanged(const DefinitionNode *node, const DefinitionDiff *, const DefinitionNode *) {
     if (node->getPath().find("/atmosphere/sun/") == 0) {
         getWindow()->getCamera()->startSunTool(1000);
     }

src/interface/modeler/AtmosphereModeler.h

@@ -12,7 +12,7 @@ class AtmosphereModeler : protected BaseModelerTool {
   public:
     AtmosphereModeler(MainModelerWindow *ui);
 
-    virtual void nodeChanged(const DefinitionNode *node, const DefinitionDiff *diff) override;
+    virtual void nodeChanged(const DefinitionNode *node, const DefinitionDiff *diff, const DefinitionNode *parent) override;
 };
 }
 }

src/interface/modeler/FloatPropertyBind.cpp

@@ -20,7 +20,7 @@ FloatPropertyBind::FloatPropertyBind(MainModelerWindow *window, const string &ob
     }
 }
 
-void FloatPropertyBind::nodeChanged(const DefinitionNode *, const DefinitionDiff *) {
+void FloatPropertyBind::nodeChanged(const DefinitionNode *, const DefinitionDiff *, const DefinitionNode *) {
     if (item) {
         item->setProperty(property.c_str(), node->getValue());
     }

src/interface/modeler/FloatPropertyBind.h

@@ -20,7 +20,7 @@ class FloatPropertyBind : public QObject, public DefinitionWatcher {
     FloatPropertyBind(MainModelerWindow *window, const string &object_name, const string &property_name,
                       FloatNode *node);
 
-    virtual void nodeChanged(const DefinitionNode *node, const DefinitionDiff *diff) override;
+    virtual void nodeChanged(const DefinitionNode *node, const DefinitionDiff *diff, const DefinitionNode *parent) override;
 
   private slots:
     void propertyChanged();

src/interface/modeler/IntPropertyBind.cpp

@@ -18,7 +18,7 @@ IntPropertyBind::IntPropertyBind(MainModelerWindow *window, const string &object
     }
 }
 
-void IntPropertyBind::nodeChanged(const DefinitionNode *, const DefinitionDiff *) {
+void IntPropertyBind::nodeChanged(const DefinitionNode *, const DefinitionDiff *, const DefinitionNode *) {
     if (item) {
         item->setProperty(property.c_str(), node->getValue());
     }

src/interface/modeler/IntPropertyBind.h

@@ -19,7 +19,7 @@ class IntPropertyBind : public QObject, public DefinitionWatcher {
   public:
     IntPropertyBind(MainModelerWindow *window, const string &object_name, const string &property_name, IntNode *node);
 
-    virtual void nodeChanged(const DefinitionNode *node, const DefinitionDiff *diff) override;
+    virtual void nodeChanged(const DefinitionNode *node, const DefinitionDiff *diff, const DefinitionNode *parent) override;
 
   private slots:
     void propertyChanged();

src/interface/modeler/ModelerCameras.cpp

@@ -89,7 +89,7 @@ void ModelerCameras::timerEvent(QTimerEvent *) {
     }
 }
 
-void ModelerCameras::nodeChanged(const DefinitionNode *node, const DefinitionDiff *) {
+void ModelerCameras::nodeChanged(const DefinitionNode *node, const DefinitionDiff *, const DefinitionNode *) {
     if (node->getPath().find("/atmosphere/sun/") == 0 and tool_mode == TOOL_SUN) {
         tool->setTarget(parent->getRenderer()->getAtmosphereRenderer()->getSunLocation());
     }

src/interface/modeler/ModelerCameras.h

@@ -50,7 +50,7 @@ class ModelerCameras : public QObject, public DefinitionWatcher {
   protected:
     virtual void timerEvent(QTimerEvent *event) override;
 
-    virtual void nodeChanged(const DefinitionNode *node, const DefinitionDiff *diff) override;
+    virtual void nodeChanged(const DefinitionNode *node, const DefinitionDiff *diff, const DefinitionNode *parent) override;
 
     /**
      * Validate current camera, pushing it to rendered scenery if needed.

src/render/opengl/OpenGLSkybox.cpp

@@ -78,7 +78,7 @@ void OpenGLSkybox::render() {
     program->draw(vertices);
 }
 
-void OpenGLSkybox::nodeChanged(const DefinitionNode *node, const DefinitionDiff *diff) {
+void OpenGLSkybox::nodeChanged(const DefinitionNode *node, const DefinitionDiff *diff, const DefinitionNode *parent) {
     OpenGLSharedState *state = renderer->getSharedState();
     AtmosphereDefinition *newdef = renderer->getScenery()->getAtmosphere();
 
@@ -92,7 +92,7 @@ void OpenGLSkybox::nodeChanged(const DefinitionNode *node, const DefinitionDiff
         state->set("dayTime", newdef->getDaytime());
     }
 
-    DefinitionWatcher::nodeChanged(node, diff);
+    DefinitionWatcher::nodeChanged(node, diff, parent);
 }
 
 void OpenGLSkybox::floatNodeChanged(const string &path, double new_value, double) {

src/render/opengl/OpenGLSkybox.h

@@ -18,7 +18,7 @@ class OPENGLSHARED_EXPORT OpenGLSkybox : public OpenGLPart, public DefinitionWat
     virtual void update() override;
     virtual void render() override;
 
-    virtual void nodeChanged(const DefinitionNode *node, const DefinitionDiff *diff) override;
+    virtual void nodeChanged(const DefinitionNode *node, const DefinitionDiff *diff, const DefinitionNode *parent) override;
     virtual void floatNodeChanged(const string &path, double new_value, double old_value) override;
 
   private:

src/render/opengl/OpenGLTerrain.cpp

@@ -165,7 +165,7 @@ void OpenGLTerrain::performChunksMaintenance() {
     }
 }
 
-void OpenGLTerrain::nodeChanged(const DefinitionNode *node, const DefinitionDiff *) {
+void OpenGLTerrain::nodeChanged(const DefinitionNode *node, const DefinitionDiff *, const DefinitionNode *) {
     if (node->getPath() == "/terrain/water_height") {
         resetTextures();
     } else if (node->getPath().find("/atmosphere") == 0) {

src/render/opengl/OpenGLTerrain.h

@@ -35,7 +35,7 @@ class OPENGLSHARED_EXPORT OpenGLTerrain : public OpenGLPart, public DefinitionWa
 
     void performChunksMaintenance();
 
-    virtual void nodeChanged(const DefinitionNode *node, const DefinitionDiff *diff) override;
+    virtual void nodeChanged(const DefinitionNode *node, const DefinitionDiff *diff, const DefinitionNode *) override;
 
   private:
     OpenGLShaderProgram *program;

src/render/opengl/OpenGLVegetation.cpp

@@ -88,7 +88,7 @@ void OpenGLVegetation::render() {
     }
 }
 
-void OpenGLVegetation::nodeChanged(const DefinitionNode *node, const DefinitionDiff *) {
+void OpenGLVegetation::nodeChanged(const DefinitionNode *node, const DefinitionDiff *, const DefinitionNode *) {
     if (node->getPath() == "/vegetation") {
         updateLayers();
     }

src/render/opengl/OpenGLVegetation.h

@@ -32,7 +32,7 @@ class OPENGLSHARED_EXPORT OpenGLVegetation : public OpenGLPart, public Definitio
     virtual void update() override;
     virtual void render() override;
 
-    virtual void nodeChanged(const DefinitionNode *node, const DefinitionDiff *diff) override;
+    virtual void nodeChanged(const DefinitionNode *node, const DefinitionDiff *diff, const DefinitionNode *parent) override;
 
     /**
      * Get the currently rendered scenery.

src/render/opengl/OpenGLWater.cpp

@@ -66,7 +66,7 @@ void OpenGLWater::render() {
     }
 }
 
-void OpenGLWater::nodeChanged(const DefinitionNode *node, const DefinitionDiff *) {
+void OpenGLWater::nodeChanged(const DefinitionNode *node, const DefinitionDiff *, const DefinitionNode *) {
     OpenGLSharedState *state = renderer->getSharedState();
 
     if (node->getPath() == path_height) {

src/render/opengl/OpenGLWater.h

@@ -18,7 +18,7 @@ class OPENGLSHARED_EXPORT OpenGLWater : public OpenGLPart, public DefinitionWatc
     virtual void update() override;
     virtual void render() override;
 
-    virtual void nodeChanged(const DefinitionNode *node, const DefinitionDiff *diff) override;
+    virtual void nodeChanged(const DefinitionNode *node, const DefinitionDiff *diff, const DefinitionNode *parent) override;
 
     /**
      * Enable or disable the water surface rendering.

src/render/software/MoonRenderer.cpp

@@ -18,17 +18,13 @@ class MoonRenderer::pimpl {
 
 MoonRenderer::MoonRenderer(CelestialBodyDefinition *moon_node) : impl(new pimpl()) {
     startWatching(moon_node->getRoot(), moon_node->getPath());
-
-    // FIXME should not be needed because the above watcher should watch the whole node
-    //   and call nodeChanged
-    moon_node->copy(&impl->definition);
 }
 
 MoonRenderer::~MoonRenderer() {
 }
 
-void MoonRenderer::nodeChanged(const DefinitionNode *node, const DefinitionDiff *) {
+void MoonRenderer::nodeChanged(const DefinitionNode *, const DefinitionDiff *, const DefinitionNode *parent) {
-    if (auto moon_node = static_cast<const CelestialBodyDefinition *>(node)) {
+    if (auto moon_node = static_cast<const CelestialBodyDefinition *>(parent)) {
         moon_node->copy(&impl->definition);
     }
 }

src/render/software/MoonRenderer.h

@@ -19,7 +19,7 @@ class SOFTWARESHARED_EXPORT MoonRenderer : public DefinitionWatcher, public Ligh
     MoonRenderer(CelestialBodyDefinition *moon_node);
     virtual ~MoonRenderer();
 
-    virtual void nodeChanged(const DefinitionNode *node, const DefinitionDiff *diff) override;
+    virtual void nodeChanged(const DefinitionNode *node, const DefinitionDiff *diff, const DefinitionNode *parent) override;
     virtual bool getLightsAt(vector<LightComponent> &result, const Vector3 &location) const override;
 
     /**

src/tests/DefinitionNode_Test.cpp

@@ -26,7 +26,7 @@ TEST(DefinitionNode, getPath) {
 }
 
 class FakeWatcher : public DefinitionWatcher {
-    virtual void nodeChanged(const DefinitionNode *, const DefinitionDiff *) override {
+    virtual void nodeChanged(const DefinitionNode *, const DefinitionDiff *, const DefinitionNode *) override {
     }
 };
 

src/tests/DiffManager_Test.cpp

@@ -1,5 +1,6 @@
 #include "BaseTestCase.h"
 
+#include "TestToolDefinition.h"
 #include "DiffManager.h"
 #include "DefinitionNode.h"
 #include "DefinitionWatcher.h"
@@ -118,7 +119,7 @@ class TestWatcher : public DefinitionWatcher {
         calls = 0;
     }
 
-    virtual void nodeChanged(const DefinitionNode *node, const DefinitionDiff *diff) override {
+    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;
@@ -154,3 +155,28 @@ TEST(DiffManager, addWatcherWithInitDiffs) {
     node.addWatcher(&watcher, true);
     EXPECT_EQ(1, watcher.calls);
 }
+
+TEST(DiffManager, publishToWatcher) {
+    DefinitionNode root(NULL, "root");
+    FloatNode node(&root, "node", 1.3);
+    RecordingDefinitionWatcher watcher;
+
+    EXPECT_EQ(0u, watcher.changes.size());
+
+    root.addWatcher(&watcher, true);
+    ASSERT_EQ(1u, watcher.changes.size());
+    EXPECT_EQ(&root, watcher.changes[0].node);
+    EXPECT_EQ(&root, watcher.changes[0].parent);
+
+    node.addWatcher(&watcher, true);
+    ASSERT_EQ(2u, watcher.changes.size());
+    EXPECT_EQ(&node, watcher.changes[1].node);
+    EXPECT_EQ(&node, watcher.changes[1].parent);
+
+    node.setValue(2.3);
+    ASSERT_EQ(4u, watcher.changes.size());
+    EXPECT_EQ(&node, watcher.changes[2].node);
+    EXPECT_EQ(&node, watcher.changes[2].parent);
+    EXPECT_EQ(&node, watcher.changes[3].node);
+    EXPECT_EQ(&root, watcher.changes[3].parent);
+}

src/tests/TestToolDefinition.h

@@ -0,0 +1,35 @@
+#ifndef TESTTOOLDEFINITION_H
+#define TESTTOOLDEFINITION_H
+
+#include "DefinitionWatcher.h"
+
+#include <vector>
+
+namespace {
+/**
+ * Definition watcher that registers all calls to nodeChanged.
+ */
+class RecordingDefinitionWatcher : public DefinitionWatcher {
+  public:
+    RecordingDefinitionWatcher() {
+    }
+
+    virtual void nodeChanged(const DefinitionNode *node, const DefinitionDiff *diff, const DefinitionNode *parent) override {
+        RecordedChange change;
+        change.node = node;
+        change.diff = diff;  // FIXME Referenced diff may get deleted by the diff manager
+        change.parent = parent;
+        changes.push_back(change);
+    }
+
+    typedef struct {
+        const DefinitionNode *node;
+        const DefinitionDiff *diff;
+        const DefinitionNode *parent;
+    } RecordedChange;
+    vector<RecordedChange> changes;
+};
+}
+
+
+#endif // TESTTOOLDEFINITION_H