Core - Fix data model and event condition bugs found by the tests

src/Artemis.Core.Tests/Conditions/EventConditionTests.cs

@@ -79,16 +79,24 @@ public void Play_TriggeringTwiceBetweenFrames_PlaysOnce()
     }
 
     [Fact]
-    public void Play_EventTriggeredBeforeTheConditionExisted_StillPlaysTheLayer()
+    public void Play_EventTriggeredBeforeTheConditionExisted_DoesNotPlayTheLayer()
     {
         DataModel.Event.Trigger();
         (Layer layer, _) = AddLayer();
 
         using RenderFrame frame = Render(layer.Profile);
 
-        // Pinned as-is: the condition starts out having seen no triggers at all, so a trigger from before it was created
-        // (for example long before the profile was activated) still counts as new and plays the layer once. Looks like
-        // the condition should only react to triggers that happen after it started watching.
+        ShouldBeOff(frame);
+    }
+
+    [Fact]
+    public void Play_EventTriggeredAfterTheConditionExisted_PlaysTheLayer_EvenBeforeTheFirstFrame()
+    {
+        (Layer layer, _) = AddLayer();
+        DataModel.Event.Trigger();
+
+        using RenderFrame frame = Render(layer.Profile);
+
         ShouldBeOn(frame);
     }
 
@@ -416,15 +424,14 @@ public void PathToAValue_PlaysWhenTheValueChanges()
     }
 
     [Fact]
-    public void PathToAValue_PlaysOnTheFirstFrame()
+    public void PathToAValue_DoesNotPlayOnTheFirstFrame()
     {
         (Layer layer, _) = AddLayer(eventPath: "Integer");
 
         using RenderFrame frame = Render(layer.Profile);
 
-        // Pinned as-is: the last seen value starts out empty, so the first frame always sees the value as changed (0 is
-        // not empty) and plays the layer even though nothing changed.
-        ShouldBeOn(frame);
+        // The first frame only learns the starting value, nothing changed yet
+        ShouldBeOff(frame);
     }
 
     [Fact]

src/Artemis.Core.Tests/DataModels/DataModelPathTests.cs

@@ -340,7 +340,7 @@ public void AddingAnotherDynamicChild_DoesNotAffectThePath()
     }
 
     [Fact]
-    public void RemovingTheDynamicChild_LeavesThePathValid()
+    public void RemovingTheDynamicChild_MakesThePathInvalid()
     {
         DataModel.AddDynamicChild("Extra", 5);
         using DataModelPath path = new(DataModel, "Extra");
@@ -349,25 +349,34 @@ public void RemovingTheDynamicChild_LeavesThePathValid()
 
         DataModel.RemoveDynamicChildByKey("Extra");
 
-        // Pinned as-is: the segment only invalidates the path when the value of the removed child is the data model the
-        // segment looks in, rather than when the child it points to is removed. So the path stays valid and reads the
-        // default value instead of becoming invalid.
+        path.IsValid.ShouldBeFalse();
+        path.GetValue().ShouldBeNull();
+        invalidated.ShouldBe(1);
+    }
+
+    [Fact]
+    public void RemovingTheDynamicChild_AndAddingItAgain_MakesThePathValidAgain()
+    {
+        DataModel.AddDynamicChild("Extra", 5);
+        using DataModelPath path = new(DataModel, "Extra");
+        DataModel.RemoveDynamicChildByKey("Extra");
+
+        DataModel.AddDynamicChild("Extra", 6);
+
         path.IsValid.ShouldBeTrue();
-        path.GetValue().ShouldBe(0);
-        invalidated.ShouldBe(0);
+        path.GetValue().ShouldBe(6);
     }
 
     [Fact]
-    public void RemovingADynamicDataModel_LeavesThePathThroughItValid()
+    public void RemovingADynamicDataModel_MakesThePathThroughItInvalid()
     {
         DataModel.AddDynamicChild("Extra", new TestChildDataModel {Integer = 4}, "Extra data");
         using DataModelPath path = new(DataModel, "Extra.Integer");
 
         DataModel.RemoveDynamicChildByKey("Extra");
 
-        // Pinned as-is, see RemovingTheDynamicChild_LeavesThePathValid
-        path.IsValid.ShouldBeTrue();
-        path.GetValue().ShouldBe(0);
+        path.IsValid.ShouldBeFalse();
+        path.GetValue().ShouldBeNull();
     }
 
     [Fact]
@@ -450,20 +459,17 @@ public void SavedPath_MovesToANewInstanceOfItsDataModel()
     }
 
     [Fact]
-    public void NewPath_DoesNotFollowTheStoreUntilItIsSaved()
+    public void NewPath_FollowsTheStoreWithoutBeingSaved()
     {
         using DataModelPath path = new(DataModel, "Integer");
 
-        // Pinned as-is: the constructors call Save before the path is set up, while it is still invalid, so nothing is
-        // saved and the entity has no data model ID. The path only reacts to its data model leaving and returning to the
-        // store once Save is called. Looks like Save should be called after Initialize.
-        path.Entity.DataModelId.ShouldBeNull();
-        path.Entity.Path.ShouldBe("");
+        path.Entity.DataModelId.ShouldBe(_module.Id);
+        path.Entity.Path.ShouldBe("Integer");
 
         RemoveFromStore();
 
-        path.IsValid.ShouldBeTrue();
-        path.Target.ShouldBeSameAs(DataModel);
+        path.IsValid.ShouldBeFalse();
+        path.Target.ShouldBeNull();
     }
 
     [Fact]
@@ -540,17 +546,23 @@ public void InvalidPath_FallsBackToTheSavedType()
     }
 
     [Fact]
-    public void InvalidPath_CannotFallBackToATypeOutsideTheCoreLibraries()
+    public void InvalidPath_FallsBackToATypeOutsideTheCoreLibraries()
     {
         using DataModelPath valid = new(DataModel, "Enum");
         valid.Save();
         valid.Entity.Type.ShouldBe(typeof(TestEnum).FullName);
 
         using DataModelPath path = new(new DataModelPathEntity {Path = "Enum", DataModelId = "Missing-12345678", Type = valid.Entity.Type});
 
-        // Pinned as-is: the type is saved by its full name without the assembly, which Type.GetType can only find in
-        // Artemis.Core and the .NET libraries, so types from plugins (and SkiaSharp, like SKColor) can't be found again.
-        path.GetPropertyType().ShouldBeNull();
+        path.GetPropertyType().ShouldBe(typeof(TestEnum));
+    }
+
+    [Fact]
+    public void InvalidPath_FallsBackToASkiaSharpType()
+    {
+        using DataModelPath path = new(new DataModelPathEntity {Path = "Color", DataModelId = "Missing-12345678", Type = typeof(SKColor).FullName});
+
+        path.GetPropertyType().ShouldBe(typeof(SKColor));
     }
 
     [Fact]

src/Artemis.Core.Tests/DataModels/DataModelTests.cs

@@ -184,19 +184,16 @@ public void RemovingAMissingDynamicChildByKey_DoesNothing()
     }
 
     [Fact]
-    public void RemovingADynamicChild_DoesNotRemoveIt()
+    public void RemovingADynamicChild_RemovesIt()
     {
         DynamicChild<int> child = DataModel.AddDynamicChild("Extra", 5);
         int removed = 0;
         DataModel.DynamicChildRemoved += (_, _) => removed++;
 
         DataModel.RemoveDynamicChild(child);
 
-        // Pinned as-is: RemoveDynamicChild compares the value of each child (BaseValue) with the child that was passed in,
-        // rather than the child itself, so it only removes a child whose value is the child object. Looks like it should
-        // compare the children themselves; RemoveDynamicChildByKey works.
-        DataModel.DynamicChildren.ShouldContainKey("Extra");
-        removed.ShouldBe(0);
+        DataModel.DynamicChildren.ShouldNotContainKey("Extra");
+        removed.ShouldBe(1);
     }
 
     [Fact]
@@ -304,14 +301,14 @@ public void ParentProperty_IsOnlyInUseWhenIncludingChildren()
     }
 
     [Fact]
-    public void IncludingChildren_AlsoMatchesPropertiesThatStartWithTheSameName()
+    public void IncludingChildren_DoesNotMatchPropertiesThatStartWithTheSameName()
     {
         using DataModelPath path = new(DataModel, "Integers");
 
-        // Pinned as-is: children are matched by checking whether a path starts with the given text, so "Integer" counts as
-        // in use because of a path to "Integers", a different property. Looks like it should check for "Integer."
-        _module.IsPropertyInUse("Integer", true).ShouldBeTrue();
+        // Integers is a different property than Integer, not a child of it
+        _module.IsPropertyInUse("Integer", true).ShouldBeFalse();
         _module.IsPropertyInUse("Integer", false).ShouldBeFalse();
+        _module.IsPropertyInUse("Integers", true).ShouldBeTrue();
     }
 
     [Fact]

src/Artemis.Core/Models/Profile/Conditions/EventCondition.cs

@@ -17,8 +17,10 @@ public class EventCondition : CorePropertyChanged, INodeScriptCondition
     private NodeScript<bool> _script;
     private DefaultNode _startNode;
     private DataModelPath? _eventPath;
-    private DateTime _lastProcessedTrigger;
+    // Only what happens after the condition starts watching its path counts, see StartWatching
+    private DateTime _lastProcessedTrigger = DateTime.Now;
     private object? _lastProcessedValue;
+    private bool _hasProcessedValue;
     private EventOverlapMode _overlapMode;
     private EventToggleOffMode _toggleOffMode;
     private EventTriggerMode _triggerMode;
@@ -64,7 +66,11 @@ public NodeScript<bool> Script
     public DataModelPath? EventPath
     {
         get => _eventPath;
-        set => SetAndNotify(ref _eventPath, value);
+        set
+        {
+            if (SetAndNotify(ref _eventPath, value))
+                StartWatching();
+        }
     }
 
     /// <summary>
@@ -243,6 +249,15 @@ private void ReplaceStartNode(DefaultNode newStartNode)
             Script.AddNode(newStartNode);
     }
 
+    private void StartWatching()
+    {
+        // Triggers and values from before the condition watched the path don't count, otherwise for example an event that
+        // triggered long before the profile was activated would still play the element once
+        _lastProcessedTrigger = DateTime.Now;
+        _lastProcessedValue = null;
+        _hasProcessedValue = false;
+    }
+
     private bool Evaluate()
     {
         if (EventPath == null)
@@ -258,6 +273,14 @@ private bool Evaluate()
         }
         else if (_startNode is EventConditionValueChangedStartNode valueChangedNode)
         {
+            // The first value seen is where the value starts, not a change
+            if (!_hasProcessedValue)
+            {
+                _lastProcessedValue = value;
+                _hasProcessedValue = true;
+                return false;
+            }
+
             if (Equals(value, _lastProcessedValue))
                 return false;
 

src/Artemis.Core/Models/Profile/DataModel/DataModelPath.cs

@@ -29,8 +29,8 @@ public DataModelPath(DataModel target)
 
         _segments = [];
 
-        Save();
         Initialize();
+        Save();
         SubscribeToDataModelStore();
     }
 
@@ -47,8 +47,8 @@ public DataModelPath(DataModel target, string path)
 
         _segments = [];
 
-        Save();
         Initialize();
+        Save();
         SubscribeToDataModelStore();
     }
 
@@ -67,8 +67,8 @@ public DataModelPath(DataModelPath dataModelPath)
 
         _segments = [];
 
-        Save();
         Initialize();
+        Save();
         SubscribeToDataModelStore();
     }
 
@@ -165,11 +165,20 @@ public DataModelPath(DataModelPathEntity entity)
 
         // Fall back to stored type
         if (!string.IsNullOrWhiteSpace(Entity.Type))
-            return Type.GetType(Entity.Type);
+            return FindType(Entity.Type);
 
         return null;
     }
 
+    private static Type? FindType(string fullName)
+    {
+        // The type is stored without its assembly, Type.GetType only finds it in Artemis.Core and the .NET libraries. Types
+        // of plugins and other libraries (like SKColor) are found by looking through the loaded assemblies
+        return Type.GetType(fullName) ?? AppDomain.CurrentDomain.GetAssemblies()
+            .Select(assembly => assembly.GetType(fullName))
+            .FirstOrDefault(type => type != null);
+    }
+
     /// <summary>
     ///     Gets the property description of the property this path points to
     /// </summary>

src/Artemis.Core/Models/Profile/DataModel/DataModelPathSegment.cs

@@ -286,8 +286,16 @@ private void DynamicChildOnDynamicChildAdded(object? sender, DynamicDataModelChi
 
     private void DynamicChildOnDynamicChildRemoved(object? sender, DynamicDataModelChildEventArgs e)
     {
-        if (e.DynamicChild.BaseValue == _dynamicDataModel)
+        // Like when a child is added, initialize the path again so it no longer points to the removed child
+        if (e.Key == Identifier)
+        {
             DataModelPath.Invalidate();
+            DataModelPath.Initialize();
+        }
+        else if (e.DynamicChild.BaseValue == _dynamicDataModel)
+        {
+            DataModelPath.Invalidate();
+        }
     }
 
     /// <inheritdoc />

src/Artemis.Core/Plugins/Modules/DataModel.cs

@@ -289,7 +289,7 @@ public void RemoveDynamicChildByKey(string key)
     public void RemoveDynamicChild(DynamicChild dynamicChild)
     {
         if (dynamicChild == null) throw new ArgumentNullException(nameof(dynamicChild));
-        List<string> keys = _dynamicChildren.Where(kvp => kvp.Value.BaseValue == dynamicChild).Select(kvp => kvp.Key).ToList();
+        List<string> keys = _dynamicChildren.Where(kvp => kvp.Value == dynamicChild).Select(kvp => kvp.Key).ToList();
         foreach (string key in keys)
         {
             _dynamicChildren.Remove(key);
@@ -335,8 +335,10 @@ internal bool IsPropertyInUse(string path, bool includeChildren)
     {
         lock (_activePaths)
         {
+            // A child's path continues after a dot, matching on the text alone would also match a property that merely starts
+            // with the same name (Integer and Integers)
             return includeChildren
-                ? _activePathsHashSet.Any(p => p.StartsWith(path, PathsStringComparison))
+                ? _activePathsHashSet.Any(p => string.Equals(p, path, PathsStringComparison) || p.StartsWith(path + ".", PathsStringComparison))
                 : _activePathsHashSet.Contains(path);
         }
     }