Core - Fix node script and pin bugs found by the tests

src/Artemis.Core.Tests/Services/ProfileImportExportTests.cs

@@ -248,7 +248,7 @@ public async Task ImportProfile_WithUnavailableBrushesAndEffects_KeepsThemForWhe
     }
 
     [Fact]
-    public async Task ImportProfile_WithUnavailableNodes_DropsThemFromTheActivationCondition()
+    public async Task ImportProfile_WithUnavailableNodes_KeepsThemInTheActivationCondition()
     {
         TestNodeProvider nodes = EnableFeature(new TestNodeProvider());
         ProfileConfiguration original = CreateSavedProfile("Test profile");
@@ -259,17 +259,15 @@ public async Task ImportProfile_WithUnavailableNodes_DropsThemFromTheActivationC
 
         ProfileConfiguration imported = await ProfileService.ImportProfile(export, General, true, false, null);
 
-        // Pinned as-is: the node can't be loaded without its plugin, and because importing saves the category right
-        // away, the node and its connection are removed from the stored activation condition for good. Unlike missing
-        // brushes and effects, enabling the plugin afterwards doesn't bring it back, and the imported profile now
-        // activates unconditionally
+        // The node can't be loaded without its plugin, but importing saves the category right away and that keeps it, so
+        // enabling the plugin afterwards brings it back like missing brushes and effects
         imported.ActivationCondition.Nodes.OfType<TestBooleanNode>().ShouldBeEmpty();
-        imported.Entity.ProfileConfiguration.ActivationCondition!.Nodes.ShouldNotContain(n => n.Type == nameof(TestBooleanNode));
+        imported.Entity.ProfileConfiguration.ActivationCondition!.Nodes.ShouldContain(n => n.Type == nameof(TestBooleanNode));
         EnableFeature(new TestNodeProvider());
         ProfileConfiguration reloaded = CreateNewProfileService().ProfileCategories.Single(c => c.Name == "General").ProfileConfigurations
             .Single(c => c.ProfileId == imported.ProfileId);
-        reloaded.ActivationCondition.Nodes.OfType<TestBooleanNode>().ShouldBeEmpty();
-        reloaded.ActivationCondition.ExitNodeConnected.ShouldBeFalse();
+        reloaded.ActivationCondition.Nodes.OfType<TestBooleanNode>().ShouldHaveSingleItem();
+        reloaded.ActivationCondition.ExitNodeConnected.ShouldBeTrue();
     }
 
     [Fact]

src/Artemis.Core.Tests/VisualScripting/NodeScriptStorageTests.cs

@@ -214,10 +214,9 @@ public void NodesOfAMissingPlugin_AreLeftOut_AndTheirConnectionsDropped()
     }
 
     [Fact]
-    public void NodesOfAMissingPlugin_AreLostWhenTheScriptIsSavedAgain()
+    public void NodesOfAMissingPlugin_AreKeptWhenTheScriptIsSavedAgain()
     {
-        // Pinned as-is: a script loaded while a plugin is missing forgets that plugin's nodes the next time it is saved.
-        // Saving a profile while a plugin is disabled permanently removes its nodes from the profile's scripts.
+        // So they come back once the plugin is available again
         TestPlugin missingPlugin = new();
         ScriptTestNodeProvider missingNodes = missingPlugin.AddFeature(new ScriptTestNodeProvider());
         NodeScript<string> script = CreateScript<string>();
@@ -227,11 +226,13 @@ public void NodesOfAMissingPlugin_AreLostWhenTheScriptIsSavedAgain()
         missingPlugin.Dispose();
 
         NodeScript<string> loaded = Load<string>(script.Entity);
-        loaded.Entity.Nodes.Count.ShouldBe(2);
+        loaded.Nodes.ShouldHaveSingleItem().IsExitNode.ShouldBeTrue();
         loaded.Save();
 
-        loaded.Entity.Nodes.ShouldHaveSingleItem().IsExitNode.ShouldBeTrue();
-        loaded.Entity.Connections.ShouldBeEmpty();
+        loaded.Entity.Nodes.Count.ShouldBe(2);
+        loaded.Entity.Nodes.ShouldContain(n => n.Id == node.Id);
+        NodeConnectionEntity connection = loaded.Entity.Connections.ShouldHaveSingleItem();
+        new[] {connection.SourceNode, connection.TargetNode}.ShouldContain(node.Id);
     }
 
     [Fact]
@@ -301,10 +302,9 @@ public void IncompatibleConnections_AreDroppedWhenLoading()
     }
 
     [Fact]
-    public void InputConnectedToTwoOutputs_KeepsOnlyTheLastConnectionWhenLoading()
+    public void InputConnectedToTwoOutputs_KeepsTheConnectionItReadsFromWhenLoading()
     {
-        // Pinned as-is: in code an input can be connected to several outputs, it then reads from the first one. Loading
-        // keeps only the last connection, so after loading the script reads from a different node than before.
+        // In code an input can be connected to several outputs, it then reads from the first one. Loading keeps that one
         NodeScript<Numeric> script = CreateScript<Numeric>();
         TestNumericNode first = Nodes.AddNode<TestNumericNode>(script);
         TestNumericNode second = Nodes.AddNode<TestNumericNode>(script);
@@ -318,8 +318,8 @@ public void InputConnectedToTwoOutputs_KeepsOnlyTheLastConnectionWhenLoading()
         NodeScript<Numeric> loaded = SaveAndLoad(script);
         loaded.Run();
 
-        loaded.GetExitNode().Input.ConnectedTo.ShouldHaveSingleItem().Node.Id.ShouldBe(second.Id);
-        loaded.Result.ShouldBe(new Numeric(2));
+        loaded.GetExitNode().Input.ConnectedTo.ShouldHaveSingleItem().Node.Id.ShouldBe(first.Id);
+        loaded.Result.ShouldBe(new Numeric(1));
     }
 
     [Fact]

src/Artemis.Core.Tests/VisualScripting/NodeScriptTests.cs

@@ -267,17 +267,15 @@ public void RemoveNode_LeavesTheConnectionsOfTheNodeAlone()
     }
 
     [Fact]
-    public void RemoveNode_RaisesNodeRemoved_EvenForANodeThatIsNotInTheScript()
+    public void RemoveNode_OfANodeThatIsNotInTheScript_DoesNotRaiseNodeRemoved()
     {
-        // Pinned as-is: removing a node that was never added still tells listeners it was removed, an editor listening
-        // to the event could remove a view model that belongs to another script
         NodeScript<bool> script = CreateScript<bool>();
         int raised = 0;
         script.NodeRemoved += (_, _) => raised++;
 
         script.RemoveNode(new TestNode());
 
-        raised.ShouldBe(1);
+        raised.ShouldBe(0);
     }
 
     [Fact]
@@ -311,10 +309,8 @@ public void DisablingTheNodeProvider_RemovesItsNodesFromTheScript()
     }
 
     [Fact]
-    public void DisablingTheNodeProvider_AlsoRemovesNodesOfTheSameTypeFromAnotherProvider()
+    public void DisablingTheNodeProvider_LeavesNodesOfTheSameTypeFromAnotherProvider()
     {
-        // Pinned as-is: the script removes nodes by their type, not by the provider that created them. When two enabled
-        // providers register the same node type, disabling one also removes the nodes created by the other.
         TestPlugin otherPlugin = new();
         TestNodeProvider otherNodes = otherPlugin.AddFeature(new TestNodeProvider());
         NodeScript<bool> script = CreateScript<bool>();
@@ -324,7 +320,7 @@ public void DisablingTheNodeProvider_AlsoRemovesNodesOfTheSameTypeFromAnotherPro
         otherPlugin.Dispose();
 
         script.Nodes.ShouldNotContain(fromOtherProvider);
-        script.Nodes.ShouldNotContain(fromStillEnabledProvider);
+        script.Nodes.ShouldContain(fromStillEnabledProvider);
     }
 
     [Fact]

src/Artemis.Core.Tests/VisualScripting/NodeTests.cs

@@ -218,10 +218,9 @@ public void TryInitialize_WhenInitializingFails_BreaksTheNode()
     }
 
     [Fact]
-    public void FailingNode_InAScript_LeavesTheResultAtTheLastValueItProduced()
+    public void FailingNode_InAScript_SetsItsOutputsToTheirDefaultValues()
     {
-        // Pinned as-is: when a node fails, its output keeps the value from the last run that worked. The script carries on
-        // with that old value and only the broken state of the node shows something is wrong.
+        // Rather than letting the value from the last run that worked flow on
         NodeScript<Numeric> script = CreateScript<Numeric>();
         TestThrowingNode node = ScriptNodes.AddNode<TestThrowingNode>(script);
         script.GetExitNode().Input.ConnectTo(node.Output);
@@ -232,15 +231,15 @@ public void FailingNode_InAScript_LeavesTheResultAtTheLastValueItProduced()
         node.FailsOn = TestThrowingNode.FailureMode.Evaluate;
         script.Run();
 
-        script.Result.ShouldBe(new Numeric(5));
+        script.Result.ShouldBe(new Numeric(0));
         node.BrokenState.ShouldBe("Failed to evaluate");
         script.GetExitNode().BrokenState.ShouldBeNull();
     }
 
     [Fact]
-    public void FailingNode_InAScript_IsEvaluatedAgainForEveryInputReadingIt()
+    public void FailingNode_InAScript_IsEvaluatedOncePerRun()
     {
-        // A failing node never marks its outputs as evaluated, so every pin reading from it tries again within one run
+        // A failing node sets its outputs to their default values, so the other pins reading from it don't try again
         NodeScript<Numeric> script = CreateScript<Numeric>();
         TestNode failing = new() {OnEvaluate = _ => throw new InvalidOperationException()};
         OutputPin<Numeric> output = failing.CreateOutputPin<Numeric>();
@@ -252,7 +251,7 @@ public void FailingNode_InAScript_IsEvaluatedAgainForEveryInputReadingIt()
 
         script.Run();
 
-        failing.Evaluations.ShouldBe(2);
+        failing.Evaluations.ShouldBe(1);
         sum.BrokenState.ShouldBeNull();
     }
 

src/Artemis.Core.Tests/VisualScripting/NumericTests.cs

@@ -99,14 +99,12 @@ public void ToInt_RoundsHalfAwayFromZero(float value, int expected)
     }
 
     [Fact]
-    public void ToLong_CutsOffTheDecimals_UnlikeTheOtherWholeNumberConversions()
+    public void ToLong_Rounds_LikeTheOtherWholeNumberConversions()
     {
-        // Pinned as-is: turning a Numeric into a long cuts off the decimals, while turning it into an int rounds, and so
-        // does the IConvertible conversion to a long
         long cast = new Numeric(2.7f);
         int castToInt = new Numeric(2.7f);
 
-        cast.ShouldBe(2);
+        cast.ShouldBe(3);
         castToInt.ShouldBe(3);
         AsConvertible(2.7f).ToInt64(null).ShouldBe(3);
     }
@@ -123,12 +121,11 @@ public void ToByte_KeepsTheValueBetween0And255(float value, byte expected)
     }
 
     [Fact]
-    public void ToSByte_KeepsTheValueBetween0And255_WhichDoesNotFitAnSByte()
+    public void ToSByte_KeepsTheValueWithinTheRangeOfAnSByte()
     {
-        // Pinned as-is: an sbyte holds -128 to 127, but the conversion clamps to 0-255 like a byte does. Anything above
-        // 127 wraps around (200 becomes -56) and negative numbers become 0.
-        AsConvertible(200f).ToSByte(null).ShouldBe((sbyte) -56);
-        AsConvertible(-5f).ToSByte(null).ShouldBe((sbyte) 0);
+        AsConvertible(200f).ToSByte(null).ShouldBe(sbyte.MaxValue);
+        AsConvertible(-200f).ToSByte(null).ShouldBe(sbyte.MinValue);
+        AsConvertible(-5f).ToSByte(null).ShouldBe((sbyte) -5);
         AsConvertible(100f).ToSByte(null).ShouldBe((sbyte) 100);
     }
 
@@ -175,8 +172,9 @@ public void Numeric_CannotBecomeACharacterOrDate()
     [Fact]
     public void PassingANumericToConvert_PicksTheByteVersion()
     {
-        // Pinned as-is: Numeric converts implicitly to byte, int, long, float, double and decimal, so the compiler sends
-        // Convert.ToInt32(numeric) and friends to their byte versions. The value is clamped to 0-255 and loses its
+        // Known limitation: Numeric converts implicitly to byte, int, long, float, double and decimal, so the compiler
+        // sends Convert.ToInt32(numeric) and friends to their byte versions. Fixing this means removing implicit
+        // conversions, which would break plugins that rely on them. The value is clamped to 0-255 and loses its
         // decimals before it is converted. Passing it as an object uses IConvertible instead.
         Numeric numeric = new(2.5f);
 

src/Artemis.Core.Tests/VisualScripting/PinTests.cs

@@ -105,17 +105,15 @@ public void ConnectTo_DoesNotLimitInputsToOneConnection()
     }
 
     [Fact]
-    public void ConnectTo_TheSamePinTwice_AddsTheConnectionTwiceOnOneSide()
+    public void ConnectTo_TheSamePinTwice_ConnectsThemOnce()
     {
-        // Pinned as-is: connecting pins that are already connected adds a second connection to the pin it is called on,
-        // while the other pin keeps one. Disconnecting once removes both again.
         OutputPin<Numeric> output = _source.CreateOutputPin<Numeric>();
         InputPin<Numeric> input = _target.CreateInputPin<Numeric>();
 
         input.ConnectTo(output);
         input.ConnectTo(output);
 
-        input.ConnectedTo.ShouldBe([output, output]);
+        input.ConnectedTo.ShouldBe([output]);
         output.ConnectedTo.ShouldBe([input]);
 
         input.DisconnectFrom(output);
@@ -215,14 +213,15 @@ public void OutputPin_FitsInputsThatTakeItsType(Type pinType, Type inputType, bo
     }
 
     [Fact]
-    public void IsTypeCompatible_MatchesEnums_EvenWhenNotForgiving()
+    public void IsTypeCompatible_OnlyMatchesASpecificEnumToAnyEnumWhenForgiving()
     {
-        // Pinned as-is: the forgiving enum check has no effect, a specific enum type already counts as an Enum without it
         InputPin input = _target.CreateInputPin(typeof(Enum));
         OutputPin output = _source.CreateOutputPin(typeof(TestEnum));
 
-        input.IsTypeCompatible(typeof(TestEnum), false).ShouldBeTrue();
-        output.IsTypeCompatible(typeof(Enum), false).ShouldBeTrue();
+        input.IsTypeCompatible(typeof(TestEnum), false).ShouldBeFalse();
+        output.IsTypeCompatible(typeof(Enum), false).ShouldBeFalse();
+        input.IsTypeCompatible(typeof(TestEnum), true).ShouldBeTrue();
+        output.IsTypeCompatible(typeof(Enum), true).ShouldBeTrue();
     }
 
     [Fact]
@@ -390,38 +389,29 @@ public void UntypedOutput_RejectsValuesOfTheWrongType()
     }
 
     [Fact]
-    public void UntypedOutput_ForAValueType_AcceptsNull()
+    public void UntypedOutput_ForAValueType_TurnsNullIntoTheDefaultValue()
     {
-        // Pinned as-is: the setter means to turn null into the default value for value types (see its comment), but
-        // then stores null anyway. A typed input reading the pin gets its default value, an untyped one turns the null
-        // into the default value itself.
         OutputPin output = _source.CreateOutputPin(typeof(Numeric));
         output.Value = new Numeric(3);
 
         output.Value = null;
 
-        output.Value.ShouldBeNull();
+        output.Value.ShouldBe(new Numeric(0));
     }
 
     [Fact]
-    public void NewTypedInput_IgnoresItsConnectionUntilItIsReset()
+    public void NewTypedInput_ReadsItsConnection_LikeAnUntypedInput()
     {
-        // Pinned as-is: a new InputPin<T> counts as evaluated with its default value, so it only reads the pin it is
-        // connected to after a reset. Running a script resets every pin first, so this only shows outside a script.
-        // Untyped input pins don't do this.
         OutputPin<Numeric> output = _source.CreateOutputPin<Numeric>();
         InputPin<Numeric> typed = _target.CreateInputPin<Numeric>();
         InputPin untyped = _target.CreateInputPin(typeof(Numeric));
         typed.ConnectTo(output);
         untyped.ConnectTo(output);
         output.Value = new Numeric(5);
 
-        typed.IsEvaluated.ShouldBeTrue();
-        typed.Value.ShouldBe(new Numeric(0));
-        untyped.Value.ShouldBe(new Numeric(5));
-
-        typed.Reset();
+        typed.IsEvaluated.ShouldBeFalse();
         typed.Value.ShouldBe(new Numeric(5));
+        untyped.Value.ShouldBe(new Numeric(5));
     }
 
     [Fact]

src/Artemis.Core/Utilities/Numeric.cs

@@ -182,7 +182,7 @@ public static implicit operator Numeric(byte b)
 
     public static implicit operator long(Numeric p)
     {
-        return (long) p._value;
+        return (long) MathF.Round(p._value, MidpointRounding.AwayFromZero);
     }
 
     public static bool operator >(Numeric a, Numeric b)
@@ -367,7 +367,7 @@ public long ToInt64(IFormatProvider? provider)
     /// <inheritdoc />
     public sbyte ToSByte(IFormatProvider? provider)
     {
-        return (sbyte) Math.Clamp(_value, 0, 255);
+        return (sbyte) Math.Clamp(_value, sbyte.MinValue, sbyte.MaxValue);
     }
 
     /// <inheritdoc />

src/Artemis.Core/VisualScripting/NodeScript.cs

@@ -22,7 +22,10 @@ private void NodeTypeStoreOnNodeTypeAdded(object? sender, NodeTypeStoreEvent e)
 
     private void NodeTypeStoreOnNodeTypeRemoved(object? sender, NodeTypeStoreEvent e)
     {
-        List<INode> nodes = Nodes.Where(n => n.GetType() == e.TypeRegistration.NodeData.Type).ToList();
+        // Match on the node data rather than the type, another provider may have registered the same type
+        List<INode> nodes = Nodes.Where(n => n.NodeData != null
+            ? n.NodeData == e.TypeRegistration.NodeData
+            : n.GetType() == e.TypeRegistration.NodeData.Type).ToList();
         foreach (INode node in nodes)
             RemoveNode(node);
     }
@@ -145,7 +148,8 @@ public void RemoveNode(INode node)
     {
         lock (_nodes)
         {
-            _nodes.Remove(node);
+            if (!_nodes.Remove(node))
+                return;
         }
 
         NodeRemoved?.Invoke(this, new SingleValueEventArgs<INode>(node));
@@ -253,6 +257,8 @@ private void LoadExistingNode(INode node, NodeEntity nodeEntity)
     public void LoadConnections()
     {
         List<INode> nodes = Nodes.ToList();
+        // An input reads from its first connection, so when there are several the first one is kept
+        HashSet<IPin> connectedInputs = [];
         foreach (NodeConnectionEntity nodeConnectionEntity in Entity.Connections.OrderBy(p => p.SourcePinCollectionId))
         {
             INode? source = nodes.FirstOrDefault(n => n.Id == nodeConnectionEntity.SourceNode);
@@ -275,6 +281,9 @@ public void LoadConnections()
             // Ensure the connection is valid
             if (sourcePin.Direction == targetPin.Direction)
                 continue;
+            IPin inputPin = targetPin.Direction == PinDirection.Input ? targetPin : sourcePin;
+            if (!connectedInputs.Add(inputPin))
+                continue;
 
             // Clear existing connections on input pins, we don't want none of that now
             if (targetPin.Direction == PinDirection.Input)
@@ -310,7 +319,16 @@ public void Save()
     {
         Entity.Name = Name;
         Entity.Description = Description;
+
+        // Nodes whose type isn't registered couldn't be loaded, most likely because their plugin is missing or disabled.
+        // Keep them and their connections, so they come back once the plugin is available again
+        List<NodeEntity> unavailableNodes = Entity.Nodes.Where(e => !e.IsExitNode && Nodes.All(n => n.Id != e.Id) && NodeTypeStore.Get(e) == null).ToList();
+        List<NodeConnectionEntity> unavailableConnections = Entity.Connections
+            .Where(c => unavailableNodes.Any(n => n.Id == c.SourceNode || n.Id == c.TargetNode))
+            .ToList();
+
         Entity.Nodes.Clear();
+        Entity.Nodes.AddRange(unavailableNodes);
 
         foreach (INode node in Nodes)
         {
@@ -346,6 +364,7 @@ public void Save()
 
         // Store connections
         Entity.Connections.Clear();
+        Entity.Connections.AddRange(unavailableConnections);
         foreach (INode node in Nodes)
         {
             SavePins(node, -1, node.Pins);

src/Artemis.Core/VisualScripting/Nodes/Node.cs

@@ -385,7 +385,13 @@ public void TryInitialize(INodeScript script)
     /// <inheritdoc />
     public void TryEvaluate()
     {
-        TryOrBreak(Evaluate, "Failed to evaluate");
+        if (TryOrBreak(Evaluate, "Failed to evaluate"))
+            return;
+
+        // Don't let the values of the last successful run flow on, and don't try again for every pin that reads from this
+        // node. Setting a value marks the pin as evaluated until the next run
+        foreach (Pin pin in Pins.Concat(PinCollections.SelectMany(c => c)).OfType<Pin>().Where(p => p.Direction == PinDirection.Output))
+            pin.SetDefaultValue();
     }
 
     /// <summary>