From aa0fa37d844a3eecf607bcaf6c3dea8856b54f1f Mon Sep 17 00:00:00 2001 From: RobBuchanan <106311829+RobBuchananCompPhys@users.noreply.github.com> Date: Mon, 27 Jul 2026 13:15:25 +0100 Subject: [PATCH 1/6] refactor: Graph & IterableGraph TOML roundtrip (#2559) --- src/nodes/edge.cpp | 78 ++++++++++++++++++++++--------- src/nodes/edge.h | 9 ++++ src/nodes/iterableGraph.cpp | 72 +++++++++++++++++++++------- src/nodes/iterableGraph.h | 11 +++++ tests/nodes/loop.cpp | 93 ++++++++++++++++++++++--------------- tests/nodes/subGraph.cpp | 77 +++++++++++++++++++----------- tests/testing.cpp | 5 +- 7 files changed, 241 insertions(+), 104 deletions(-) diff --git a/src/nodes/edge.cpp b/src/nodes/edge.cpp index c767573136..986bb3c9a1 100644 --- a/src/nodes/edge.cpp +++ b/src/nodes/edge.cpp @@ -3,6 +3,7 @@ #include "nodes/edge.h" #include "nodes/graph.h" +#include "nodes/inputs.h" #include "nodes/loopBack.h" #include "nodes/outputs.h" @@ -31,6 +32,21 @@ class EdgeConstructor : public Edge // Create an edge from the supplied definition std::unique_ptr Edge::create(Graph *parent, const EdgeDefinition &definition) { + // Get target node + auto targetNode = parent->findNode(definition.targetNode); + if (!targetNode) + { + Messenger::error("Target node '{}' does not exist in the graph.\n", definition.targetNode); + return {}; + } + + // Disallow circular edges (mostly a check for Graph -> Graph connections) + if (targetNode == parent) + { + Messenger::error("Target node is graph '{}' and cannot be the owner of the edge.", definition.targetNode); + return {}; + } + // Get source node and output auto sourceNode = parent->findNode(definition.sourceNode); if (!sourceNode) @@ -38,11 +54,27 @@ std::unique_ptr Edge::create(Graph *parent, const EdgeDefinition &definiti Messenger::error("Source node '{}' does not exist in the graph.\n", definition.sourceNode); return {}; } + auto sourceOutput = sourceNode->findOutput(definition.sourceOutput); if (!sourceOutput) { - Messenger::error("Source node '{}' has no output parameter '{}'.\n", definition.sourceNode, definition.sourceOutput); - return {}; + // If the source node is a Graph's own Inputs node, we will create an edge on the fly - else, throw an error + if (!dynamic_cast(sourceNode)) + { + Messenger::error("Source node '{}' has no output parameter '{}'.\n", definition.sourceNode, + definition.sourceOutput); + return {}; + } + + // The target node is the parent Graph's own Inputs node, so create a parameter link from the mapped input to the + // targetInput + auto link = targetNode->findInput(definition.targetInput)->createParameterLink(definition.sourceOutput); + if (!parent->addProxyInput(link.inputParameter, link.outputParameter)) + { + Messenger::error("Failed to add mapped input '{}'.\n", definition.targetInput); + return {}; + } + sourceOutput = parent->proxyInputs().findOutput(definition.sourceOutput); } // Confirm that the source is actually an output @@ -53,21 +85,6 @@ std::unique_ptr Edge::create(Graph *parent, const EdgeDefinition &definiti return {}; } - // Get target node and input - auto targetNode = parent->findNode(definition.targetNode); - if (!targetNode) - { - Messenger::error("Target node '{}' does not exist in the graph.\n", definition.targetNode); - return {}; - } - - // Disallow circular edges (mostly a check for Graph -> Graph connections) - if (targetNode == parent) - { - Messenger::error("Target node is graph '{}' and cannot be the owner of the edge.", definition.targetNode); - return {}; - } - // We need to check carefully the target node, since we need to permit outside connections to the Graph object itself as // well as its Outputs node explicitly. std::shared_ptr targetInput{nullptr}; @@ -75,13 +92,19 @@ std::unique_ptr Edge::create(Graph *parent, const EdgeDefinition &definiti { // The target node is a Graph: create a parameter link from the sourceOutput and from it a mapped input auto graphNode = dynamic_cast(targetNode); - auto link = sourceOutput->createParameterLink(definition.targetInput); - if (!graphNode->addProxyInput(link.inputParameter, link.outputParameter)) + auto existingTargetInput = graphNode->findInput(definition.targetInput); + if (!existingTargetInput.get()) { - Messenger::error("Failed to add mapped input '{}'.\n", definition.targetInput); - return {}; + auto link = sourceOutput->createParameterLink(definition.targetInput); + if (!graphNode->addProxyInput(link.inputParameter, link.outputParameter)) + { + Messenger::error("Failed to add mapped input '{}'.\n", definition.targetInput); + return {}; + } + targetInput = link.inputParameter; } - targetInput = link.inputParameter; + else + targetInput = existingTargetInput; } else if (dynamic_cast(targetNode)) { @@ -288,3 +311,14 @@ void Edge::deserialise(const SerialisedValue &node) throw std::runtime_error("Cannot directly deserialise edges. Please contact the Dissolve development team if you are " "seeing this error - this is a bug and NOT your fault.\n"); } + +// Express as a serialisable value +void LoopEdge::serialise(std::string tag, SerialisedValue &target) const +{ + definition().serialise(tag, target); + target[tag]["targetNode"] = "LoopBacks"; + target[tag]["analogue"] = analogue_; +} + +// Read values from a serialisable value +void LoopEdge::deserialise(const SerialisedValue &node) { Edge::deserialise(node); } diff --git a/src/nodes/edge.h b/src/nodes/edge.h index dcc4d934d5..ddfdb4fc85 100644 --- a/src/nodes/edge.h +++ b/src/nodes/edge.h @@ -115,4 +115,13 @@ class LoopEdge : public Edge * */ ParameterBase *analogue_; + + /* + * Serialisation + */ + public: + // Express as a serialisable value + void serialise(std::string tag, SerialisedValue &target) const override; + // Read values from a serialisable value + void deserialise(const SerialisedValue &node) override; }; \ No newline at end of file diff --git a/src/nodes/iterableGraph.cpp b/src/nodes/iterableGraph.cpp index 6ae7583aa1..d9e9838f3f 100644 --- a/src/nodes/iterableGraph.cpp +++ b/src/nodes/iterableGraph.cpp @@ -85,6 +85,12 @@ LoopEdge *IterableGraph::findLoopEdge(const EdgeDefinition &definition) const return {}; } +// Add edge between nodes +bool IterableGraph::addLoopEdge(std::unique_ptr edge, std::string_view source) +{ + return addOutputLoopEdge(source, loopEdges_.emplace_back(LoopEdge::makeLoopEdge(edge.release(), proxyInputs())).get()); +} + // Add edge to node map Edge *IterableGraph::addOutputLoopEdge(std::string_view sourceOutput, Edge *edge) { @@ -128,23 +134,26 @@ Edge *IterableGraph::removeOutputLoopEdge(std::string_view sourceOutput, Edge *e // Add edge between nodes bool IterableGraph::addEdge(const EdgeDefinition &definition) { - if (dynamic_cast(parentGraph()->findNode(definition.sourceNode))) - setLoopBacks(); - else if (loopBacks_->findInput(definition.targetInput)) - { - auto edge = - Edge::create(this, {definition.sourceNode, definition.sourceOutput, definition.targetNode, definition.targetInput}); - if (!edge) - return false; - - loopEdges_.emplace_back(LoopEdge::makeLoopEdge(edge.release(), proxyInputs())); - - addOutputLoopEdge(definition.sourceOutput, loopEdges_.back().get()); - - return true; - } - - return Graph::addEdge(definition); + // Refresh the graph loopbacks + setLoopBacks(); + + // Check if the connection is invertible. + // Invertibility is satisfied when the source node (internal to the graph) can output to an existing loopback, + // which discounts any edge for which no loopbacks correspond to the target input, as well as the graphs own InputsNode. + auto nonInvertible = dynamic_cast(parentGraph()->findNode(definition.sourceNode)) || + !loopBacks_->findInput(definition.targetInput); + + // If not invertible, create and return a standard edge + if (nonInvertible) + return Graph::addEdge(definition); + + // Create loop edge + auto edge = + Edge::create(this, {definition.sourceNode, definition.sourceOutput, definition.targetNode, definition.targetInput}); + if (!edge) + return false; + + return addLoopEdge(std::move(edge), definition.sourceOutput); } // Remove edge between nodes @@ -183,3 +192,32 @@ NodeConstants::ProcessResult IterableGraph::process() return NodeConstants::ProcessResult::Success; } + +/* + * Serialisation + */ + +// Express as a serialisable value +void IterableGraph::serialise(std::string tag, SerialisedValue &target) const +{ + Graph::serialise(tag, target); + auto &result = target[tag]; + fromVector(loopEdges_, "loopEdges", result); +} + +// Read values from a serialisable value +void IterableGraph::deserialise(const SerialisedValue &node) +{ + Graph::deserialise(node); + toVector(node, "loopEdges", + [this](const auto &value) + { + auto definition = toml::get(value); + auto edge = Edge::create( + this, {definition.sourceNode, definition.sourceOutput, definition.targetNode, definition.targetInput}); + if (!edge) + return false; + + return addLoopEdge(std::move(edge), definition.sourceOutput); + }); +} diff --git a/src/nodes/iterableGraph.h b/src/nodes/iterableGraph.h index c8b51cc9df..a7882bddf1 100644 --- a/src/nodes/iterableGraph.h +++ b/src/nodes/iterableGraph.h @@ -46,6 +46,8 @@ class IterableGraph : public Graph void releaseLoopBack(const std::string &name); private: + // Add edge between nodes + bool addLoopEdge(std::unique_ptr edge, std::string_view source); // Add edge to node map Edge *addOutputLoopEdge(std::string_view sourceOutput, Edge *edge); // Remove edge from node map @@ -80,4 +82,13 @@ class IterableGraph : public Graph protected: // Perform processing NodeConstants::ProcessResult process() override; + + /* + * Serialisation + */ + public: + // Express as a serialisable value + void serialise(std::string tag, SerialisedValue &target) const override; + // Read values from a serialisable value + void deserialise(const SerialisedValue &node) override; }; diff --git a/tests/nodes/loop.cpp b/tests/nodes/loop.cpp index c7a9d8791c..337d60de18 100644 --- a/tests/nodes/loop.cpp +++ b/tests/nodes/loop.cpp @@ -7,6 +7,7 @@ #include "nodes/numberNode.h" #include "nodes/outputs.h" #include "nodes/registry.h" +#include "tests/testGraphFixture.h" #include namespace UnitTest @@ -37,39 +38,57 @@ class IterableGraphTest : public ::testing::Test // Create nodes i_ = dynamic_cast(root_.createNode("Number", "i")); - loop_ = dynamic_cast(root_.createNode("Iterator", "Iterator")); - x_ = dynamic_cast(loop_->createNode("Add", "x")); + loopGraph_ = dynamic_cast(root_.createNode("Iterator", "Iterator")); + x_ = dynamic_cast(loopGraph_->createNode("Add", "x")); y_ = dynamic_cast(root_.createNode("Add", "y")); ASSERT_TRUE(i_); ASSERT_TRUE(x_); ASSERT_TRUE(y_); - ASSERT_TRUE(loop_); + ASSERT_TRUE(loopGraph_); ASSERT_EQ(i_->name(), "i"); ASSERT_EQ(x_->name(), "x"); ASSERT_EQ(y_->name(), "y"); - ASSERT_EQ(loop_->name(), "Iterator"); + ASSERT_EQ(loopGraph_->name(), "Iterator"); // Create edge connections // - Number 'i' is a dynamic input to the IterableGraph - we'll call the input "I" EXPECT_TRUE(root_.addEdge({"i", "X", "Iterator", "I"})); // - Add 'x' takes the IterableGraph input "I" as its parameter "X" - EXPECT_TRUE(loop_->addEdge({"Inputs", "I", "x", "X"})); + EXPECT_TRUE(loopGraph_->addEdge({"Inputs", "I", "x", "X"})); // - Result from Add 'x' goes to graph output (which we will call "C") as well as loopback to "I" - EXPECT_TRUE(loop_->addEdge({"x", "Result", "Outputs", "C"})); - EXPECT_TRUE(loop_->addEdge({"x", "Result", "LoopBacks", "I"})); + EXPECT_TRUE(loopGraph_->addEdge({"x", "Result", "Outputs", "C"})); + EXPECT_TRUE(loopGraph_->addEdge({"x", "Result", "LoopBacks", "I"})); // - The output "C" of the loop graph then goes to input "X" of Add 'y' EXPECT_TRUE(root_.addEdge({"Iterator", "C", "y", "X"})); } protected: - DissolveGraph root_; + TestGraph root_; NumberNode *i_{nullptr}; AddNode *x_{nullptr}, *y_{nullptr}; - IterableGraph *loop_{nullptr}; + IterableGraph *loopGraph_{nullptr}; }; +TEST_F(IterableGraphTest, RoundTrip) +{ + createGraph(); + + // Serialised graph TOML + SerialisedValue graphTOML; + ASSERT_NO_THROW(root_.serialise("graph", graphTOML)); + + // Deserialise from the stored TOML + auto deserialisedGraph = std::make_unique(); + ASSERT_NO_THROW(deserialisedGraph->deserialise(graphTOML["graph"])); + + // Complete round trip - re-serialise the result and compare it to the original TOML + SerialisedValue compareTOML; + ASSERT_NO_THROW(deserialisedGraph->serialise("graph", compareTOML)); + ASSERT_NO_THROW(UnitTest::compareToml("", graphTOML, compareTOML)); +} + TEST_F(IterableGraphTest, BasicNonLoopingSeries) { auto root = std::make_unique(); @@ -183,7 +202,7 @@ TEST_F(IterableGraphTest, NoRun) EXPECT_TRUE(iA); iA->set(1); - auto nLoops = loop_->findOption("N"); + auto nLoops = loopGraph_->findOption("N"); EXPECT_TRUE(nLoops); nLoops->set(0); @@ -193,13 +212,13 @@ TEST_F(IterableGraphTest, NoRun) // Check node versioning EXPECT_EQ(i_->versionIndex(), 0); - EXPECT_EQ(loop_->proxyInputs().versionIndex(), NodeConstants::InvalidVersion); + EXPECT_EQ(loopGraph_->proxyInputs().versionIndex(), NodeConstants::InvalidVersion); EXPECT_EQ(x_->versionIndex(), NodeConstants::InvalidVersion); - EXPECT_EQ(loop_->proxyOutputs().versionIndex(), NodeConstants::InvalidVersion); + EXPECT_EQ(loopGraph_->proxyOutputs().versionIndex(), NodeConstants::InvalidVersion); EXPECT_EQ(y_->versionIndex(), 0); // Loopbacks node only runs on iteration i > 0 - EXPECT_EQ(loop_->loopBacks()->versionIndex(), NodeConstants::InvalidVersion); + EXPECT_EQ(loopGraph_->loopBacks()->versionIndex(), NodeConstants::InvalidVersion); } TEST_F(IterableGraphTest, NoFeedback) @@ -218,7 +237,7 @@ TEST_F(IterableGraphTest, NoFeedback) EXPECT_TRUE(iA); iA->set(1); - auto nLoops = loop_->findOption("N"); + auto nLoops = loopGraph_->findOption("N"); EXPECT_TRUE(nLoops); // Zero iterations: We expect 1 + (xB = 1) = 1 + 1 = 2 @@ -229,13 +248,13 @@ TEST_F(IterableGraphTest, NoFeedback) // Check node versioning EXPECT_EQ(i_->versionIndex(), 0); - EXPECT_EQ(loop_->proxyInputs().versionIndex(), 0); + EXPECT_EQ(loopGraph_->proxyInputs().versionIndex(), 0); EXPECT_EQ(x_->versionIndex(), 0); - EXPECT_EQ(loop_->proxyOutputs().versionIndex(), 0); + EXPECT_EQ(loopGraph_->proxyOutputs().versionIndex(), 0); EXPECT_EQ(y_->versionIndex(), 0); // Loopbacks node only runs on iteration i > 0 - EXPECT_EQ(loop_->loopBacks()->versionIndex(), NodeConstants::InvalidVersion); + EXPECT_EQ(loopGraph_->loopBacks()->versionIndex(), NodeConstants::InvalidVersion); } TEST_F(IterableGraphTest, SingleFeedback) @@ -254,7 +273,7 @@ TEST_F(IterableGraphTest, SingleFeedback) EXPECT_TRUE(iA); iA->set(1); - auto nLoops = loop_->findOption("N"); + auto nLoops = loopGraph_->findOption("N"); EXPECT_TRUE(nLoops); // One iteration: We expect (LB = 2) + (xB = 1) = 2 + 1 = 3 @@ -265,13 +284,13 @@ TEST_F(IterableGraphTest, SingleFeedback) // Check node versioning EXPECT_EQ(i_->versionIndex(), 0); - EXPECT_EQ(loop_->proxyInputs().versionIndex(), 1); + EXPECT_EQ(loopGraph_->proxyInputs().versionIndex(), 1); EXPECT_EQ(x_->versionIndex(), 1); - EXPECT_EQ(loop_->proxyOutputs().versionIndex(), 1); + EXPECT_EQ(loopGraph_->proxyOutputs().versionIndex(), 1); EXPECT_EQ(y_->versionIndex(), 0); // Loopbacks node only runs on iteration i > 0 - EXPECT_EQ(loop_->loopBacks()->versionIndex(), 0); + EXPECT_EQ(loopGraph_->loopBacks()->versionIndex(), 0); } TEST_F(IterableGraphTest, ExtendedFeedback) @@ -290,7 +309,7 @@ TEST_F(IterableGraphTest, ExtendedFeedback) EXPECT_TRUE(iA); iA->set(1); - auto nLoops = loop_->findOption("N"); + auto nLoops = loopGraph_->findOption("N"); EXPECT_TRUE(nLoops); /* @@ -317,27 +336,27 @@ TEST_F(IterableGraphTest, ExtendedFeedback) // Check node versioning EXPECT_EQ(i_->versionIndex(), 0); - EXPECT_EQ(loop_->proxyInputs().versionIndex(), 9); + EXPECT_EQ(loopGraph_->proxyInputs().versionIndex(), 9); EXPECT_EQ(x_->versionIndex(), 9); - EXPECT_EQ(loop_->proxyOutputs().versionIndex(), 9); + EXPECT_EQ(loopGraph_->proxyOutputs().versionIndex(), 9); EXPECT_EQ(y_->versionIndex(), 0); // Loopbacks node only runs on iteration i > 1 - EXPECT_EQ(loop_->loopBacks()->versionIndex(), 8); + EXPECT_EQ(loopGraph_->loopBacks()->versionIndex(), 8); } TEST_F(IterableGraphTest, ReleaseLoopBack) { createGraph(); - const auto nEdges = loop_->edges().size(); + const auto nEdges = loopGraph_->edges().size(); - auto flagged = loop_->proxyInputs().findOutput("I"); + auto flagged = loopGraph_->proxyInputs().findOutput("I"); - loop_->removeEdge({"x", "Result", "LoopBacks", "I"}); + loopGraph_->removeEdge({"x", "Result", "LoopBacks", "I"}); - ASSERT_EQ(loop_->loopEdges().size(), 0); - ASSERT_EQ(loop_->edges().size(), nEdges); + ASSERT_EQ(loopGraph_->loopEdges().size(), 0); + ASSERT_EQ(loopGraph_->edges().size(), nEdges); } TEST_F(IterableGraphTest, UpstreamChange) @@ -356,7 +375,7 @@ TEST_F(IterableGraphTest, UpstreamChange) EXPECT_TRUE(iA); iA->set(1); - auto nLoops = loop_->findOption("N"); + auto nLoops = loopGraph_->findOption("N"); EXPECT_TRUE(nLoops); /* @@ -372,14 +391,14 @@ TEST_F(IterableGraphTest, UpstreamChange) // Check node versioning EXPECT_EQ(i_->versionIndex(), 0); - EXPECT_EQ(loop_->proxyInputs().versionIndex(), 99); + EXPECT_EQ(loopGraph_->proxyInputs().versionIndex(), 99); EXPECT_EQ(x_->versionIndex(), 99); - EXPECT_EQ(loop_->proxyOutputs().versionIndex(), 99); + EXPECT_EQ(loopGraph_->proxyOutputs().versionIndex(), 99); EXPECT_EQ(y_->versionIndex(), 0); // Loopbacks node only runs on iteration 0 < i <= nLoops // (in 100 runs, loop backs up version 99 times, starting from -1) - EXPECT_EQ(loop_->loopBacks()->versionIndex(), 98); + EXPECT_EQ(loopGraph_->loopBacks()->versionIndex(), 98); /* * Alter upstream number node and run for another 100 iterations @@ -394,14 +413,14 @@ TEST_F(IterableGraphTest, UpstreamChange) // Check node versioning EXPECT_EQ(i_->versionIndex(), 1); - EXPECT_EQ(loop_->proxyInputs().versionIndex(), 199); + EXPECT_EQ(loopGraph_->proxyInputs().versionIndex(), 199); EXPECT_EQ(x_->versionIndex(), 199); - EXPECT_EQ(loop_->proxyOutputs().versionIndex(), 199); + EXPECT_EQ(loopGraph_->proxyOutputs().versionIndex(), 199); EXPECT_EQ(y_->versionIndex(), 1); // Loopbacks node only runs on iteration 0 < i <= nLoops // (after another 100 runs, loop backs up version a further 99 times, starting from 98) - EXPECT_EQ(loop_->loopBacks()->versionIndex(), 197); + EXPECT_EQ(loopGraph_->loopBacks()->versionIndex(), 197); } } // namespace UnitTest diff --git a/tests/nodes/subGraph.cpp b/tests/nodes/subGraph.cpp index 07d367338c..3b0c1d82a0 100644 --- a/tests/nodes/subGraph.cpp +++ b/tests/nodes/subGraph.cpp @@ -4,6 +4,7 @@ #include "nodes/add.h" #include "nodes/dissolve.h" #include "nodes/number.h" +#include "tests/testing.h" #include namespace UnitTest @@ -71,6 +72,21 @@ class SubGraphTest : public ::testing::Test wB_ = w_->findInput("Y"); ASSERT_TRUE(wB_); wB_->set(Number{5}); + + // Create a mapped input on GraphA by creating an edge to it + EXPECT_TRUE(root_.addEdge({"x", "Result", "GraphA", "C"})); + + // Connect the mapped input on GraphA internally to it's "z" node + EXPECT_TRUE(graphA_->addEdge({"Inputs", "C", "z", "X"})); + + // Connect y result to z + EXPECT_TRUE(graphA_->addEdge({"y", "Result", "z", "Y"})); + + // Connect z result to graphA output, creating a mapped output + EXPECT_TRUE(graphA_->addEdge({"z", "Result", "Outputs", "D"})); + + // Connect GraphA mapped output "D" to node "w" + EXPECT_TRUE(root_.addEdge({"GraphA", "D", "w", "X"})); } protected: @@ -80,46 +96,53 @@ class SubGraphTest : public ::testing::Test std::shared_ptr xA_{nullptr}, xB_{nullptr}; std::shared_ptr yA_{nullptr}, yB_{nullptr}; std::shared_ptr wB_{nullptr}; -}; -TEST_F(SubGraphTest, Connections) -{ - createGraph(); - - // Create a mapped input on GraphA by creating an edge to it - EXPECT_TRUE(root_.addEdge({"x", "Result", "GraphA", "C"})); + // Basic sub-graph connection test + void connect() + { + // Create a mapped input on GraphA by creating an edge to it + EXPECT_TRUE(root_.addEdge({"x", "Result", "GraphA", "C"})); - // Connect the mapped input on GraphA internally to it's "z" node - EXPECT_TRUE(graphA_->addEdge({"Inputs", "C", "z", "X"})); + // Connect the mapped input on GraphA internally to it's "z" node + EXPECT_TRUE(graphA_->addEdge({"Inputs", "C", "z", "X"})); - // Connect y result to z - EXPECT_TRUE(graphA_->addEdge({"y", "Result", "z", "Y"})); + // Connect y result to z + EXPECT_TRUE(graphA_->addEdge({"y", "Result", "z", "Y"})); - // Connect z result to graphA output, creating a mapped output - EXPECT_TRUE(graphA_->addEdge({"z", "Result", "Outputs", "D"})); + // Connect z result to graphA output, creating a mapped output + EXPECT_TRUE(graphA_->addEdge({"z", "Result", "Outputs", "D"})); - // Connect GraphA mapped output "D" to node "w" - EXPECT_TRUE(root_.addEdge({"GraphA", "D", "w", "X"})); -} + // Connect GraphA mapped output "D" to node "w" + EXPECT_TRUE(root_.addEdge({"GraphA", "D", "w", "X"})); + } +}; -TEST_F(SubGraphTest, Flow) +TEST_F(SubGraphTest, RoundTrip) { createGraph(); - // Create a mapped input on GraphA by creating an edge to it - EXPECT_TRUE(root_.addEdge({"x", "Result", "GraphA", "C"})); + // Serialised graph TOML + SerialisedValue graphTOML; + ASSERT_NO_THROW(root_.serialise("graph", graphTOML)); - // Connect the mapped input on GraphA internally to it's "z" node - EXPECT_TRUE(graphA_->addEdge({"Inputs", "C", "z", "X"})); + // Deserialise from the stored TOML + auto deserialisedGraph = std::make_unique(); + ASSERT_NO_THROW(deserialisedGraph->deserialise(graphTOML["graph"])); - // Connect y result to z - EXPECT_TRUE(graphA_->addEdge({"y", "Result", "z", "Y"})); + // Complete round trip - re-serialise the result and compare it to the original TOML + SerialisedValue compareTOML; + ASSERT_NO_THROW(deserialisedGraph->serialise("graph", compareTOML)); + ASSERT_NO_THROW(UnitTest::compareToml("", graphTOML, compareTOML)); +} - // Connect z result to graphA output, creating a mapped output - EXPECT_TRUE(graphA_->addEdge({"z", "Result", "Outputs", "D"})); +TEST_F(SubGraphTest, Connections) +{ + createGraph(); +} - // Connect GraphA mapped output "D" to node "w" - EXPECT_TRUE(root_.addEdge({"GraphA", "D", "w", "X"})); +TEST_F(SubGraphTest, Flow) +{ + createGraph(); // Run w - all nodes should update EXPECT_EQ(w_->run(), NodeConstants::ProcessResult::Success); diff --git a/tests/testing.cpp b/tests/testing.cpp index 810804bda4..aae5b6900b 100644 --- a/tests/testing.cpp +++ b/tests/testing.cpp @@ -427,9 +427,12 @@ void compareToml(std::string location, SerialisedValue toml, SerialisedValue tom if (toml.is_table()) { ASSERT_TRUE(toml2.is_table()) << location; + auto tab1 = toml.as_table(); + auto tab2 = toml2.as_table(); for (auto &[k, v] : toml.as_table()) { - ASSERT_TRUE(toml2.contains(k)) << location << "." << k << std::endl << "Expected:" << std::endl << toml[k]; + auto result = toml2.contains(k); + ASSERT_TRUE(result) << location << "." << k << std::endl << "Expected:" << std::endl << toml[k]; compareToml(std::format("{}.{}", location, k), v, toml2.at(k)); } } From 0a7bbf975eedc4457d2d2587696890386abc296c Mon Sep 17 00:00:00 2001 From: Tristan Youngs Date: Wed, 26 Aug 2026 10:43:11 +0100 Subject: [PATCH 2/6] No overrides. --- src/nodes/edge.h | 4 ++-- src/nodes/iterableGraph.h | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/nodes/edge.h b/src/nodes/edge.h index ddfdb4fc85..c50e875d53 100644 --- a/src/nodes/edge.h +++ b/src/nodes/edge.h @@ -121,7 +121,7 @@ class LoopEdge : public Edge */ public: // Express as a serialisable value - void serialise(std::string tag, SerialisedValue &target) const override; + void serialise(std::string tag, SerialisedValue &target) const; // Read values from a serialisable value - void deserialise(const SerialisedValue &node) override; + void deserialise(const SerialisedValue &node); }; \ No newline at end of file diff --git a/src/nodes/iterableGraph.h b/src/nodes/iterableGraph.h index a7882bddf1..6f7847559d 100644 --- a/src/nodes/iterableGraph.h +++ b/src/nodes/iterableGraph.h @@ -88,7 +88,7 @@ class IterableGraph : public Graph */ public: // Express as a serialisable value - void serialise(std::string tag, SerialisedValue &target) const override; + void serialise(std::string tag, SerialisedValue &target) const; // Read values from a serialisable value - void deserialise(const SerialisedValue &node) override; + void deserialise(const SerialisedValue &node); }; From 70ed5741184be1e5de09228933e04c2d0c342968 Mon Sep 17 00:00:00 2001 From: Tristan Youngs Date: Wed, 26 Aug 2026 12:38:05 +0100 Subject: [PATCH 3/6] Fixup. --- src/nodes/iterableGraph.cpp | 24 ++++++++++++------------ tests/nodes/loop.cpp | 4 ++-- tests/nodes/subGraph.cpp | 5 +---- 3 files changed, 15 insertions(+), 18 deletions(-) diff --git a/src/nodes/iterableGraph.cpp b/src/nodes/iterableGraph.cpp index d9e9838f3f..aaf101a790 100644 --- a/src/nodes/iterableGraph.cpp +++ b/src/nodes/iterableGraph.cpp @@ -202,22 +202,22 @@ void IterableGraph::serialise(std::string tag, SerialisedValue &target) const { Graph::serialise(tag, target); auto &result = target[tag]; - fromVector(loopEdges_, "loopEdges", result); + Serialisable::vector(loopEdges_, "loopEdges", result); } // Read values from a serialisable value void IterableGraph::deserialise(const SerialisedValue &node) { Graph::deserialise(node); - toVector(node, "loopEdges", - [this](const auto &value) - { - auto definition = toml::get(value); - auto edge = Edge::create( - this, {definition.sourceNode, definition.sourceOutput, definition.targetNode, definition.targetInput}); - if (!edge) - return false; - - return addLoopEdge(std::move(edge), definition.sourceOutput); - }); + Deserialisable::vector(node, "loopEdges", + [this](const auto &value) + { + auto edge = Edge::create( + this, {value.at("sourceNode").as_string(), value.at("sourceOutput").as_string(), + value.at("targetNode").as_string(), value.at("targetInput").as_string()}); + if (!edge) + return false; + + return addLoopEdge(std::move(edge), value.at("sourceOutput").as_string()); + }); } diff --git a/tests/nodes/loop.cpp b/tests/nodes/loop.cpp index 337d60de18..b431bf0930 100644 --- a/tests/nodes/loop.cpp +++ b/tests/nodes/loop.cpp @@ -7,7 +7,7 @@ #include "nodes/numberNode.h" #include "nodes/outputs.h" #include "nodes/registry.h" -#include "tests/testGraphFixture.h" +#include "tests/testing.h" #include namespace UnitTest @@ -65,7 +65,7 @@ class IterableGraphTest : public ::testing::Test } protected: - TestGraph root_; + DissolveGraph root_; NumberNode *i_{nullptr}; AddNode *x_{nullptr}, *y_{nullptr}; IterableGraph *loopGraph_{nullptr}; diff --git a/tests/nodes/subGraph.cpp b/tests/nodes/subGraph.cpp index 3b0c1d82a0..05966eb2b5 100644 --- a/tests/nodes/subGraph.cpp +++ b/tests/nodes/subGraph.cpp @@ -135,10 +135,7 @@ TEST_F(SubGraphTest, RoundTrip) ASSERT_NO_THROW(UnitTest::compareToml("", graphTOML, compareTOML)); } -TEST_F(SubGraphTest, Connections) -{ - createGraph(); -} +TEST_F(SubGraphTest, Connections) { createGraph(); } TEST_F(SubGraphTest, Flow) { From 8899ce69b7585ef587d57a626bab5b2915f9c5cb Mon Sep 17 00:00:00 2001 From: Tristan Youngs Date: Wed, 26 Aug 2026 13:08:05 +0100 Subject: [PATCH 4/6] Do it properly. --- src/nodes/iterableGraph.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/nodes/iterableGraph.cpp b/src/nodes/iterableGraph.cpp index aaf101a790..56627df3cd 100644 --- a/src/nodes/iterableGraph.cpp +++ b/src/nodes/iterableGraph.cpp @@ -212,9 +212,9 @@ void IterableGraph::deserialise(const SerialisedValue &node) Deserialisable::vector(node, "loopEdges", [this](const auto &value) { - auto edge = Edge::create( - this, {value.at("sourceNode").as_string(), value.at("sourceOutput").as_string(), - value.at("targetNode").as_string(), value.at("targetInput").as_string()}); + auto edgeDefinition = Deserialisable::deser(value); + auto edge = Edge::create(this, {edgeDefinition.sourceNode, edgeDefinition.sourceOutput, + edgeDefinition.targetNode, edgeDefinition.targetInput}); if (!edge) return false; From 751ff5b2568edf2784a1b3bc177a01faad234f4a Mon Sep 17 00:00:00 2001 From: Tristan Youngs Date: Fri, 28 Aug 2026 12:31:46 +0100 Subject: [PATCH 5/6] Virtual functions. --- src/nodes/graph.h | 4 ++-- src/nodes/iterableGraph.h | 4 ++-- src/nodes/node.h | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/nodes/graph.h b/src/nodes/graph.h index acbc6846f9..707bbde768 100644 --- a/src/nodes/graph.h +++ b/src/nodes/graph.h @@ -115,7 +115,7 @@ class Graph : public Node */ public: // Express as a serialisable value - void serialise(std::string tag, SerialisedValue &target) const; + void serialise(std::string tag, SerialisedValue &target) const override; // Read values from a serialisable value - void deserialise(const SerialisedValue &node); + void deserialise(const SerialisedValue &node) override; }; diff --git a/src/nodes/iterableGraph.h b/src/nodes/iterableGraph.h index 6f7847559d..a7882bddf1 100644 --- a/src/nodes/iterableGraph.h +++ b/src/nodes/iterableGraph.h @@ -88,7 +88,7 @@ class IterableGraph : public Graph */ public: // Express as a serialisable value - void serialise(std::string tag, SerialisedValue &target) const; + void serialise(std::string tag, SerialisedValue &target) const override; // Read values from a serialisable value - void deserialise(const SerialisedValue &node); + void deserialise(const SerialisedValue &node) override; }; diff --git a/src/nodes/node.h b/src/nodes/node.h index 77fc6a93ea..59521760b4 100644 --- a/src/nodes/node.h +++ b/src/nodes/node.h @@ -367,9 +367,9 @@ class Node serialisables_[std::string(key)] = std::make_shared>(key, data); } // Express as a serialisable value - void serialise(std::string tag, SerialisedValue &target) const; + virtual void serialise(std::string tag, SerialisedValue &target) const; // Read values from a serialisable value - void deserialise(const SerialisedValue &node); + virtual void deserialise(const SerialisedValue &node); // Express persistent data as a serialisable value SerialisedValue serialiseData() const; // Read persistent data from a serialisable value From bf0d678f0db68eed8b6e59307bd4733df0d01499 Mon Sep 17 00:00:00 2001 From: Adam Washington Date: Fri, 4 Sep 2026 08:33:12 +0000 Subject: [PATCH 6/6] fix: Properly deserialise loopback edges (#2599) --- src/nodes/iterableGraph.cpp | 2 +- tests/nodes/loop.cpp | 13 +++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/src/nodes/iterableGraph.cpp b/src/nodes/iterableGraph.cpp index 56627df3cd..5284c2d36e 100644 --- a/src/nodes/iterableGraph.cpp +++ b/src/nodes/iterableGraph.cpp @@ -214,7 +214,7 @@ void IterableGraph::deserialise(const SerialisedValue &node) { auto edgeDefinition = Deserialisable::deser(value); auto edge = Edge::create(this, {edgeDefinition.sourceNode, edgeDefinition.sourceOutput, - edgeDefinition.targetNode, edgeDefinition.targetInput}); + "LoopBacks", edgeDefinition.targetInput}); if (!edge) return false; diff --git a/tests/nodes/loop.cpp b/tests/nodes/loop.cpp index b431bf0930..cdfd58f99a 100644 --- a/tests/nodes/loop.cpp +++ b/tests/nodes/loop.cpp @@ -79,6 +79,14 @@ TEST_F(IterableGraphTest, RoundTrip) SerialisedValue graphTOML; ASSERT_NO_THROW(root_.serialise("graph", graphTOML)); + // count how many loop edges were made + auto loop = dynamic_cast(root_.findNode("Iterator")); + ASSERT_TRUE(loop); + auto correctEdges = loop->loopEdges().size(); + + // Confirm that loop edges were rebuilt + ASSERT_EQ(loop->loopEdges().size(), 1); + // Deserialise from the stored TOML auto deserialisedGraph = std::make_unique(); ASSERT_NO_THROW(deserialisedGraph->deserialise(graphTOML["graph"])); @@ -87,6 +95,11 @@ TEST_F(IterableGraphTest, RoundTrip) SerialisedValue compareTOML; ASSERT_NO_THROW(deserialisedGraph->serialise("graph", compareTOML)); ASSERT_NO_THROW(UnitTest::compareToml("", graphTOML, compareTOML)); + + // Confirm that loop edges were rebuilt + loop = dynamic_cast(deserialisedGraph->findNode("Iterator")); + ASSERT_TRUE(loop); + ASSERT_EQ(loop->loopEdges().size(), correctEdges); } TEST_F(IterableGraphTest, BasicNonLoopingSeries)