TraverseAll skips nodes in an unexpected way

[SlimeQ]

I'm working in my own codebase at this point but I noticed a weird bug when loading graphs today.

When a project loads, it calls TraverseAll(), which looks like this:

        /// <summary>
        /// Recalculate from every node regarded as an input node
        /// </summary>
        public override void TraverseAll()
        {
            workList = new List<Node>();
            for (int i = 0; i < nodeCanvas.nodes.Count; i++)
            {
                Node node = nodeCanvas.nodes[i];
                if (node.isInput())
                { // Add all Inputs
                    node.ClearCalculation();
                    workList.Add(node);
                }
            }
            StartCalculation();
        }

And of course node.isInput() looks like this:

        /// <summary>
        /// Returns whether the node acts as an input (no inputs or no inputs assigned)
        /// </summary>
        public bool isInput()
        {
            for (int i = 0; i < inputPorts.Count; i++)
            {
                if (!inputPorts[i].connected())
                {
                    return false;
                }
            }
            return true;
        }

Very important bit here: (no inputs or no inputs assigned)

So the top node here will be calculated on graph load, but the bottom node will not. In most cases this is not an issue, as most nodes require input to work. However, in this particular case my Card View node could potentially have data generated within it even with no inputs. This means on project load it will output nothing instead of whatever was intended.

Additionally, in some nodes I do things in Calculate() when the input is empty. This works fine in 99% of cases, because new nodes are calculated and nodes are recalculated when breaking a connection. It does not work, however, on graph load because TraverseAll() skips them.

While I understand this is probably an edge case, I'd argue that including the isInput check in TraverseAll does more harm than good. The amount of time it saves is negligible since the nodes it skips have no input data and do (mostly) nothing in Calculate(). And it potentially can create very weird and confusing situations if the dev isn't aware of this weird rule.

So I propose this TraverseAll function instead:

        /// <summary>
        /// Recalculate from every node regarded as an input node
        /// </summary>
        public override void TraverseAll()
        {
            workList = new List<Node>();
            for (int i = 0; i < nodeCanvas.nodes.Count; i++)
            {
                Node node = nodeCanvas.nodes[i];
                node.ClearCalculation();
                workList.Add(node);
            }
            StartCalculation();
        }

[Seneral]

Ah, thanks for pointing that out. The inversion in the if-check is straight up wrong. Which means this is a bug and unintentionally stopped those nodes from being calculated (when it was supposed to regard them) and ALSO added unnecessary load on the traversal system. Plan was, that not all nodes should be added to the work list, ONLY the input nodes, and following nodes would get added after all parents are calculated. Of course, now all nodes except those input nodes not having any inputs are added to the worklist, creating the unintended behaviour you found but also asking for calculation of nodes when their predecessors were not calculated. It seems it slipped away because I'm always implementing checks (as in example nodes) so it would just deny calculation until all predecessors would work, and thus stay in the work list. I'll post a fix now.

[Seneral]

Ok just looked into the source code, seems this has already been fixed in 3ce5323 Since you seem to be using a modified version (looking good!), you might also want to check out this commit, especially those two lines changing the static keyword. They are integral and that bug has caused some serious performance issues. Thanks for notifying anyway!