GH-646 Function graph validation for return node links

src/script/function.h

@@ -18,6 +18,7 @@
 #define ORCHESTRATOR_SCRIPT_FUNCTION_H
 
 #include "common/guid.h"
+#include "orchestration/build_log.h"
 
 #include <godot_cpp/classes/resource.hpp>
 #include <godot_cpp/variant/dictionary.hpp>

src/script/nodes/functions/function_result.cpp

@@ -17,6 +17,9 @@
 #include "function_result.h"
 
 #include "common/property_utils.h"
+#include "script/nodes/flow_control/for.h"
+#include "script/nodes/flow_control/for_each.h"
+#include "script/nodes/flow_control/while.h"
 
 class OScriptNodeFunctionResultInstance : public OScriptNodeInstance
 {
@@ -46,10 +49,14 @@ class OScriptNodeFunctionResultInstance : public OScriptNodeInstance
 
 void OScriptNodeFunctionResult::pre_remove()
 {
-    // When this node is removed, clear the function's return value
+    // When this node is removed, clear the function's return value if this is the last return node
     Ref<OScriptFunction> function = get_function();
     if (function.is_valid())
-        _function->set_has_return_value(false);
+    {
+        Vector<Ref<OScriptNode>> return_nodes = function->get_return_nodes();
+        if (return_nodes.size() == 1)
+            _function->set_has_return_value(false);
+    }
 }
 
 void OScriptNodeFunctionResult::allocate_default_pins()
@@ -90,6 +97,87 @@ void OScriptNodeFunctionResult::validate_node_during_build(BuildLog& p_log) cons
                     p_log.error(this, pin, "Requires a connection.");
             }
         }
+
+        // Nothing stops a user from adding multiple return nodes to a function graph
+        // The control flow connection validation should only be called once per function
+        if (function->get_return_node().ptr() == this)
+        {
+            // Collect all nodes that participate in the function graph
+            HashMap<int, Ref<OScriptNode>> graph_nodes;
+            for (const Ref<OScriptNode>& node : function->get_function_graph()->get_nodes())
+                graph_nodes[node->get_id()] = node;
+
+            // Collect all control flow connections in the function graph
+            RBSet<OScriptConnection> control_flows;
+            for (const OScriptConnection& E : function->get_orchestration()->get_connections())
+            {
+                if (graph_nodes.has(E.from_node))
+                {
+                    const Ref<OScriptNode> node = graph_nodes[E.from_node];
+                    for (const Ref<OScriptNodePin>& output : node->find_pins(PD_Output))
+                    {
+                        if (output.is_valid() && output->is_execution() && E.from_port == output->get_pin_index())
+                            control_flows.insert(E);
+                    }
+                }
+            }
+
+            RBSet<uint64_t> skipped;
+            List<uint64_t> seen;
+
+            seen.push_back(function->get_owning_node_id()); // starting node
+            seen.push_back(_id); // return node
+
+            // Traverse from the starting function node
+            List<int> queue;
+            queue.push_back(function->get_owning_node_id());
+            while(!queue.is_empty())
+            {
+                const int current_id = queue.front()->get();
+
+                for (const OScriptConnection& E : control_flows)
+                {
+                    const Ref<OScriptNode> source = graph_nodes[E.from_node];
+                    if (source.is_valid())
+                    {
+                        Ref<OScriptNodeForEach> for_each_node = source;
+                        Ref<OScriptNodeForLoop> for_loop_node = source;
+                        Ref<OScriptNodeWhile> while_node = source;
+
+                        if ((for_each_node.is_valid() && E.from_port <= 2)
+                            || (for_loop_node.is_valid() && E.from_port <= 1)
+                            || (while_node.is_valid() && E.from_port <= 0))
+                            skipped.insert(E.to_node);
+                    }
+
+                    if (skipped.has(E.from_node))
+                        skipped.insert(E.to_node);
+
+                    if (E.from_node == current_id && !seen.find(E.to_node))
+                    {
+                        queue.push_back(E.to_node);
+                        seen.push_back(E.to_node);
+                    }
+                }
+                queue.pop_front();
+            }
+
+            for (uint64_t node_id : seen)
+            {
+                if (skipped.has(node_id))
+                    continue;
+
+                const Ref<OScriptNode> node = graph_nodes[node_id];
+                if (node.is_valid())
+                {
+                    for (const Ref<OScriptNodePin>& output : node->find_pins(PD_Output))
+                    {
+                        if (output.is_valid() && output->is_execution() && !output->has_any_connections())
+                            p_log.error(node.ptr(), output, "This pin should be connected to the return node.");
+                    }
+                }
+            }
+        }
     }
 }
 

src/script/nodes/functions/function_result.h

@@ -48,7 +48,6 @@ class OScriptNodeFunctionResult : public OScriptNodeFunctionTerminator
     bool is_compatible_with_graph(const Ref<OScriptGraph>& p_graph) const override;
     void post_placed_new_node() override;
     bool can_user_delete_node() const override;
-    bool can_duplicate() const override { return false; }
     OScriptNodeInstance* instantiate() override;
     //~ End OScriptNode Interface
 

src/script/nodes/functions/function_terminator.cpp

@@ -17,6 +17,7 @@
 #include "function_terminator.h"
 
 #include "common/dictionary_utils.h"
+#include "common/macros.h"
 #include "common/property_utils.h"
 #include "common/variant_utils.h"
 
@@ -157,11 +158,8 @@ void OScriptNodeFunctionTerminator::post_initialize()
     super::post_initialize();
 
     _function = get_orchestration()->find_function(_guid);
-    if (_function.is_valid())
-    {
-        if (_is_in_editor())
-            _function->connect("changed", callable_mp(this, &OScriptNodeFunctionTerminator::_on_function_changed));
-    }
+    if (_function.is_valid() && _is_in_editor())
+        OCONNECT(_function, "changed", callable_mp(this, &OScriptNodeFunctionTerminator::_on_function_changed));
 
     // Always reconstruct entry/exit nodes
     reconstruct_node();
@@ -171,9 +169,6 @@ void OScriptNodeFunctionTerminator::post_placed_new_node()
 {
     super::post_placed_new_node();
 
-    if (_function.is_valid())
-    {
-        if (_is_in_editor())
-            _function->connect("changed", callable_mp(this, &OScriptNodeFunctionTerminator::_on_function_changed));
-    }
+    if (_function.is_valid() && _is_in_editor())
+        OCONNECT(_function, "changed", callable_mp(this, &OScriptNodeFunctionTerminator::_on_function_changed));
 }