From fe24a0f48d3bd1cfbc323543820ba67928ab8a18 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C3=ABl=20Lemaire?= Date: Mon, 18 Jan 2016 23:06:50 +0100 Subject: [PATCH] Added pimpl on DiffManager --- src/definition/DefinitionNode.cpp | 5 +- src/definition/DefinitionNode.h | 4 +- src/definition/DefinitionWatcher.cpp | 4 ++ src/definition/DefinitionWatcher.h | 7 +++ src/definition/DiffManager.cpp | 63 ++++++++++++------- src/definition/DiffManager.h | 15 ++--- src/definition/VegetationDefinition.cpp | 6 +- src/render/opengl/OpenGLVegetation.cpp | 2 +- .../opengl/OpenGLVegetationImpostor.cpp | 3 +- src/tests/DefinitionNode_Test.cpp | 22 +++---- src/tests/Layers_Test.cpp | 14 ++--- 11 files changed, 85 insertions(+), 60 deletions(-) diff --git a/src/definition/DefinitionNode.cpp b/src/definition/DefinitionNode.cpp index 2b6855c..a45ae43 100644 --- a/src/definition/DefinitionNode.cpp +++ b/src/definition/DefinitionNode.cpp @@ -149,7 +149,7 @@ void DefinitionNode::addWatcher(DefinitionWatcher *watcher, bool init_diff) { } } -int DefinitionNode::getWatcherCount() const { +unsigned long DefinitionNode::getWatcherCount() const { if (root && root->diffs) { return root->diffs->getWatcherCount(this); } else { @@ -265,7 +265,8 @@ void DefinitionNode::addDiff(const DefinitionDiff *diff) { if (root && root->diffs) { root->diffs->addDiff(this, diff); } else { - // TODO Apply diff ? + // No diff manager available, apply it directly and delete it + applyDiff(diff); delete diff; } } diff --git a/src/definition/DefinitionNode.h b/src/definition/DefinitionNode.h index c2eff42..f157fef 100644 --- a/src/definition/DefinitionNode.h +++ b/src/definition/DefinitionNode.h @@ -43,7 +43,7 @@ class DEFINITIONSHARED_EXPORT DefinitionNode { inline DiffManager *getDiffManager() const { return diffs; } - inline int getChildrenCount() const { + inline unsigned long getChildrenCount() const { return children.size(); } @@ -94,7 +94,7 @@ class DEFINITIONSHARED_EXPORT DefinitionNode { /** * Get the current number of watchers. */ - int getWatcherCount() const; + unsigned long getWatcherCount() const; protected: void addChild(DefinitionNode *child); diff --git a/src/definition/DefinitionWatcher.cpp b/src/definition/DefinitionWatcher.cpp index 37f9379..2577986 100644 --- a/src/definition/DefinitionWatcher.cpp +++ b/src/definition/DefinitionWatcher.cpp @@ -38,3 +38,7 @@ void DefinitionWatcher::startWatching(const DefinitionNode *root, const string & 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); +} diff --git a/src/definition/DefinitionWatcher.h b/src/definition/DefinitionWatcher.h index bbb32ca..206352a 100644 --- a/src/definition/DefinitionWatcher.h +++ b/src/definition/DefinitionWatcher.h @@ -31,6 +31,13 @@ class DEFINITIONSHARED_EXPORT DefinitionWatcher { */ void startWatching(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); + /** * Abstract convenience to receive integer node changes. */ diff --git a/src/definition/DiffManager.cpp b/src/definition/DiffManager.cpp index a88af0b..84398f6 100644 --- a/src/definition/DiffManager.cpp +++ b/src/definition/DiffManager.cpp @@ -1,42 +1,59 @@ #include "DiffManager.h" #include -#include +#include +#include #include "DefinitionNode.h" #include "DefinitionDiff.h" #include "DefinitionWatcher.h" #include "Logs.h" -DiffManager::DiffManager(DefinitionNode *tree) : tree(tree) { - undone = 0; +class DiffManager::pimpl { + public: + pimpl(DefinitionNode *tree) : tree(tree), undone(0) { + } + + DefinitionNode *tree; + unsigned long undone; + vector diffs; + map> watchers; +}; + +DiffManager::DiffManager(DefinitionNode *tree) : impl(new pimpl(tree)) { } DiffManager::~DiffManager() { - for (auto diff : diffs) { + // TODO smart pointers + for (auto diff : impl->diffs) { delete diff; } - diffs.clear(); + impl->diffs.clear(); +} + +unsigned long DiffManager::getDiffCount(int include_undone) const { + return include_undone ? impl->diffs.size() : impl->diffs.size() - impl->undone; } void DiffManager::addWatcher(const DefinitionNode *node, DefinitionWatcher *watcher) { - if (find(watchers[node].begin(), watchers[node].end(), watcher) == watchers[node].end()) { - watchers[node].push_back(watcher); + auto &watchers = impl->watchers[node]; + if (find(watchers.begin(), watchers.end(), watcher) == watchers.end()) { + watchers.push_back(watcher); } } -int DiffManager::getWatcherCount(const DefinitionNode *node) { - return watchers[node].size(); +unsigned long DiffManager::getWatcherCount(const DefinitionNode *node) { + return impl->watchers[node].size(); } void DiffManager::addDiff(DefinitionNode *node, const DefinitionDiff *diff) { - while (undone > 0) { + while (impl->undone > 0) { // truncate diffs ahead - delete diffs.back(); - diffs.pop_back(); - undone--; + delete impl->diffs.back(); + impl->diffs.pop_back(); + impl->undone--; } - diffs.push_back(diff); + impl->diffs.push_back(diff); // TODO Delayed commit (with merge of consecutive diffs) @@ -46,13 +63,13 @@ void DiffManager::addDiff(DefinitionNode *node, const DefinitionDiff *diff) { } void DiffManager::undo() { - if (undone < (int)diffs.size()) { - const DefinitionDiff *diff = diffs[diffs.size() - undone - 1]; + if (impl->undone < impl->diffs.size()) { + const DefinitionDiff *diff = impl->diffs[impl->diffs.size() - impl->undone - 1]; // Obtain the node by path and reverse apply diff on it - DefinitionNode *node = tree->findByPath(diff->getPath()); + DefinitionNode *node = impl->tree->findByPath(diff->getPath()); if (node) { - undone++; + impl->undone++; unique_ptr reversed(diff->newReversed()); @@ -67,13 +84,13 @@ void DiffManager::undo() { } void DiffManager::redo() { - if (undone > 0) { - const DefinitionDiff *diff = diffs[diffs.size() - undone]; + if (impl->undone > 0) { + const DefinitionDiff *diff = impl->diffs[impl->diffs.size() - impl->undone]; // Obtain the node by path and re-apply diff on it - DefinitionNode *node = tree->findByPath(diff->getPath()); + DefinitionNode *node = impl->tree->findByPath(diff->getPath()); if (node) { - undone--; + impl->undone--; Logs::debug("Definition") << "Node redo : " << node->getPath() << endl; node->applyDiff(diff); @@ -88,7 +105,7 @@ void DiffManager::publishToWatchers(const DefinitionNode *node, const Definition // TODO Parent node signaling should be aggregated (to not receive many nodeChanged calls) const DefinitionNode *cnode = node; do { - for (auto watcher : watchers[cnode]) { + for (auto watcher : impl->watchers[cnode]) { watcher->nodeChanged(node, diff, cnode); } cnode = cnode->getParent(); diff --git a/src/definition/DiffManager.h b/src/definition/DiffManager.h index 876fa00..5d6a504 100644 --- a/src/definition/DiffManager.h +++ b/src/definition/DiffManager.h @@ -3,8 +3,7 @@ #include "definition_global.h" -#include -#include +#include namespace paysages { namespace definition { @@ -22,9 +21,7 @@ class DEFINITIONSHARED_EXPORT DiffManager { /** * Get the total number of diff stored. */ - inline int getDiffCount(int include_undone = true) { - return include_undone ? diffs.size() : diffs.size() - undone; - } + unsigned long getDiffCount(int include_undone = true) const; /** * Add a watcher for a specific node. @@ -36,7 +33,7 @@ class DEFINITIONSHARED_EXPORT DiffManager { /** * Get the number of watchers registered for a given node. */ - int getWatcherCount(const DefinitionNode *node); + unsigned long getWatcherCount(const DefinitionNode *node); /** * Add a new diff of a node to the change flow. @@ -63,10 +60,8 @@ class DEFINITIONSHARED_EXPORT DiffManager { void publishToWatchers(const DefinitionNode *node, const DefinitionDiff *diff); private: - DefinitionNode *tree; - int undone; - vector diffs; - map> watchers; + class pimpl; + unique_ptr impl; }; } } diff --git a/src/definition/VegetationDefinition.cpp b/src/definition/VegetationDefinition.cpp index 2ed1d6f..7a4e002 100644 --- a/src/definition/VegetationDefinition.cpp +++ b/src/definition/VegetationDefinition.cpp @@ -29,9 +29,9 @@ void VegetationDefinition::applyPreset(VegetationPreset preset, RandomGenerator clear(); - /*if (preset == VEGETATION_PRESET_TEMPERATE) { + if (preset == VEGETATION_PRESET_TEMPERATE) { layer.applyPreset(VegetationLayerDefinition::VEGETATION_BASIC_TREES, random); layer.setName("Basic tree"); - addLayer(layer); - }*/ + //addLayer(layer); + } } diff --git a/src/render/opengl/OpenGLVegetation.cpp b/src/render/opengl/OpenGLVegetation.cpp index f5b1b46..c2a2ebe 100644 --- a/src/render/opengl/OpenGLVegetation.cpp +++ b/src/render/opengl/OpenGLVegetation.cpp @@ -118,7 +118,7 @@ void OpenGLVegetation::acquireLayers(vector &layers) { layers_lock->release(); } -void OpenGLVegetation::releaseLayers(const vector &layers) { +void OpenGLVegetation::releaseLayers(const vector &) { // TODO Reference count } diff --git a/src/render/opengl/OpenGLVegetationImpostor.cpp b/src/render/opengl/OpenGLVegetationImpostor.cpp index c3e5b69..158eaa0 100644 --- a/src/render/opengl/OpenGLVegetationImpostor.cpp +++ b/src/render/opengl/OpenGLVegetationImpostor.cpp @@ -67,7 +67,8 @@ void OpenGLVegetationImpostor::render(OpenGLShaderProgram *program, const OpenGL } void OpenGLVegetationImpostor::prepareTexture(const VegetationModelDefinition &model, const Scenery &environment, - bool *interrupt) { + bool *) { + // TODO interrupt Scenery scenery; environment.getAtmosphere()->copy(scenery.getAtmosphere()); SoftwareRenderer renderer(&scenery); diff --git a/src/tests/DefinitionNode_Test.cpp b/src/tests/DefinitionNode_Test.cpp index 30d6c3a..3bd8a47 100644 --- a/src/tests/DefinitionNode_Test.cpp +++ b/src/tests/DefinitionNode_Test.cpp @@ -25,20 +25,20 @@ TEST(DefinitionNode, getPath) { EXPECT_EQ("/branch/leaf", leaf.getPath()); } -class FakeWatcher : public DefinitionWatcher { - virtual void nodeChanged(const DefinitionNode *, const DefinitionDiff *, const DefinitionNode *) override { - } -}; - 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(0, root.getWatcherCount()); + EXPECT_EQ(0u, root.getWatcherCount()); root.addWatcher(&watcher); - EXPECT_EQ(1, root.getWatcherCount()); + EXPECT_EQ(1u, root.getWatcherCount()); root.addWatcher(&watcher); - EXPECT_EQ(1, root.getWatcherCount()); + EXPECT_EQ(1u, root.getWatcherCount()); } TEST(DefinitionNode, findByPath) { @@ -65,15 +65,15 @@ TEST(DefinitionNode, attachDetach) { EXPECT_EQ(root, child1->getParent()); EXPECT_EQ(root, child2->getParent()); - EXPECT_EQ(2, root->getChildrenCount()); + EXPECT_EQ(2u, root->getChildrenCount()); delete child1; - EXPECT_EQ(1, root->getChildrenCount()); + EXPECT_EQ(1u, root->getChildrenCount()); delete child2; - EXPECT_EQ(0, root->getChildrenCount()); + EXPECT_EQ(0u, root->getChildrenCount()); delete root; } diff --git a/src/tests/Layers_Test.cpp b/src/tests/Layers_Test.cpp index bda6a23..0b8699a 100644 --- a/src/tests/Layers_Test.cpp +++ b/src/tests/Layers_Test.cpp @@ -127,25 +127,25 @@ TEST(Layers, undoRedo) { Layers layers(NULL, "layers", _construc3); EXPECT_EQ(0, layers.getLayerCount()); - EXPECT_EQ(0, layers.getDiffManager()->getDiffCount()); + EXPECT_EQ(0u, layers.getDiffManager()->getDiffCount()); layers.addLayer("testlayer"); ASSERT_EQ(1, layers.getLayerCount()); - TestLayer *layer = (TestLayer *)layers.getLayer(0); + auto layer = static_cast(layers.getLayer(0)); - EXPECT_EQ(1, layers.getDiffManager()->getDiffCount()); + EXPECT_EQ(1u, layers.getDiffManager()->getDiffCount()); checkLayerChild(layers, 0, "val", 5); layer->val->setValue(8); - EXPECT_EQ(2, layers.getDiffManager()->getDiffCount()); + EXPECT_EQ(2u, layers.getDiffManager()->getDiffCount()); checkLayerChild(layers, 0, "val", 8); layers.removeLayer(0); - EXPECT_EQ(3, layers.getDiffManager()->getDiffCount()); + EXPECT_EQ(3u, layers.getDiffManager()->getDiffCount()); EXPECT_EQ(0, layers.getLayerCount()); // Start undoing @@ -156,7 +156,7 @@ TEST(Layers, undoRedo) { layers.getDiffManager()->undo(); EXPECT_EQ(0, layers.getLayerCount()); - EXPECT_EQ(3, layers.getDiffManager()->getDiffCount()); + EXPECT_EQ(3u, layers.getDiffManager()->getDiffCount()); // Start redoing layers.getDiffManager()->redo(); @@ -166,7 +166,7 @@ TEST(Layers, undoRedo) { layers.getDiffManager()->redo(); EXPECT_EQ(0, layers.getLayerCount()); - EXPECT_EQ(3, layers.getDiffManager()->getDiffCount()); + EXPECT_EQ(3u, layers.getDiffManager()->getDiffCount()); // TODO Test swapping }