From 6df81b19b148f9bd2cfa0add0c232cc405b79518 Mon Sep 17 00:00:00 2001 From: RobertBeekman Date: Tue, 20 Oct 2026 13:46:06 +0200 Subject: [PATCH] Core - Fix data model and event condition bugs found by the tests (#910) * Core - Fix removing dynamic children and checking which properties are in use - DataModel.RemoveDynamicChild compared the value of each child with the child that was passed in, so it never removed anything. - IsPropertyInUse with includeChildren matched paths by their start, so a path to "Integers" made "Integer" count as in use. Children are now matched on what follows a dot. * Core - Fix data model paths around saving, removed children and stored types - The constructors saved the path before setting it up, while it was still invalid, so a new path stored nothing and didn't follow its data model leaving and returning to the store until it was saved. - Removing the dynamic child a path points to left the path valid and reading a default value, it now becomes invalid like when the child was never there. - The stored type of a path is now also looked up in all loaded assemblies, Type.GetType only found it in Artemis.Core and the .NET libraries, not in plugins or SkiaSharp. * Core - Only let event conditions react to what happens after they start watching - A trigger from before the condition watched its event, for example long before the profile was activated, counted as new and played the element once. Triggers now count from the moment the condition starts watching its path. - A condition on a plain value treated the first value it saw as a change and played the element on the first frame. The first value is now taken as the starting value. --- .../Conditions/EventConditionTests.cs | 23 +++++--- .../DataModels/DataModelPathTests.cs | 58 +++++++++++-------- .../DataModels/DataModelTests.cs | 17 +++--- .../Profile/Conditions/EventCondition.cs | 27 ++++++++- .../Models/Profile/DataModel/DataModelPath.cs | 17 ++++-- .../Profile/DataModel/DataModelPathSegment.cs | 10 +++- src/Artemis.Core/Plugins/Modules/DataModel.cs | 6 +- 7 files changed, 108 insertions(+), 50 deletions(-) diff --git a/src/Artemis.Core.Tests/Conditions/EventConditionTests.cs b/src/Artemis.Core.Tests/Conditions/EventConditionTests.cs index c5bb7edf6..d3358d904 100644 --- a/src/Artemis.Core.Tests/Conditions/EventConditionTests.cs +++ b/src/Artemis.Core.Tests/Conditions/EventConditionTests.cs @@ -79,16 +79,24 @@ public class EventConditionTests : RenderTestBase } [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); } @@ -418,15 +426,14 @@ public class EventConditionTests : RenderTestBase } [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] diff --git a/src/Artemis.Core.Tests/DataModels/DataModelPathTests.cs b/src/Artemis.Core.Tests/DataModels/DataModelPathTests.cs index 44c7eb910..6eb3e1709 100644 --- a/src/Artemis.Core.Tests/DataModels/DataModelPathTests.cs +++ b/src/Artemis.Core.Tests/DataModels/DataModelPathTests.cs @@ -340,7 +340,7 @@ public class DataModelPathTests : IDisposable } [Fact] - public void RemovingTheDynamicChild_LeavesThePathValid() + public void RemovingTheDynamicChild_MakesThePathInvalid() { DataModel.AddDynamicChild("Extra", 5); using DataModelPath path = new(DataModel, "Extra"); @@ -349,25 +349,34 @@ public class DataModelPathTests : IDisposable 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.ShouldBeTrue(); - path.GetValue().ShouldBe(0); - invalidated.ShouldBe(0); + path.IsValid.ShouldBeFalse(); + path.GetValue().ShouldBeNull(); + invalidated.ShouldBe(1); } [Fact] - public void RemovingADynamicDataModel_LeavesThePathThroughItValid() + 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(6); + } + + [Fact] + 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 class DataModelPathTests : IDisposable } [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,7 +546,7 @@ public class DataModelPathTests : IDisposable } [Fact] - public void InvalidPath_CannotFallBackToATypeOutsideTheCoreLibraries() + public void InvalidPath_FallsBackToATypeOutsideTheCoreLibraries() { using DataModelPath valid = new(DataModel, "Enum"); valid.Save(); @@ -548,9 +554,15 @@ public class DataModelPathTests : IDisposable 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] diff --git a/src/Artemis.Core.Tests/DataModels/DataModelTests.cs b/src/Artemis.Core.Tests/DataModels/DataModelTests.cs index 68d002dd9..df4f49416 100644 --- a/src/Artemis.Core.Tests/DataModels/DataModelTests.cs +++ b/src/Artemis.Core.Tests/DataModels/DataModelTests.cs @@ -184,7 +184,7 @@ public class DataModelTests : IDisposable } [Fact] - public void RemovingADynamicChild_DoesNotRemoveIt() + public void RemovingADynamicChild_RemovesIt() { DynamicChild child = DataModel.AddDynamicChild("Extra", 5); int removed = 0; @@ -192,11 +192,8 @@ public class DataModelTests : IDisposable 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 class DataModelTests : IDisposable } [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] diff --git a/src/Artemis.Core/Models/Profile/Conditions/EventCondition.cs b/src/Artemis.Core/Models/Profile/Conditions/EventCondition.cs index bbe50bdc1..acf68a02d 100644 --- a/src/Artemis.Core/Models/Profile/Conditions/EventCondition.cs +++ b/src/Artemis.Core/Models/Profile/Conditions/EventCondition.cs @@ -17,8 +17,10 @@ public class EventCondition : CorePropertyChanged, INodeScriptCondition private NodeScript _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 class EventCondition : CorePropertyChanged, INodeScriptCondition public DataModelPath? EventPath { get => _eventPath; - set => SetAndNotify(ref _eventPath, value); + set + { + if (SetAndNotify(ref _eventPath, value)) + StartWatching(); + } } /// @@ -243,6 +249,15 @@ public class EventCondition : CorePropertyChanged, INodeScriptCondition 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 @@ public class EventCondition : CorePropertyChanged, INodeScriptCondition } 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; diff --git a/src/Artemis.Core/Models/Profile/DataModel/DataModelPath.cs b/src/Artemis.Core/Models/Profile/DataModel/DataModelPath.cs index 62ebd93bb..9039e6792 100644 --- a/src/Artemis.Core/Models/Profile/DataModel/DataModelPath.cs +++ b/src/Artemis.Core/Models/Profile/DataModel/DataModelPath.cs @@ -29,8 +29,8 @@ public class DataModelPath : IStorageModel, IDisposable, IPluginFeatureDependent _segments = []; - Save(); Initialize(); + Save(); SubscribeToDataModelStore(); } @@ -47,8 +47,8 @@ public class DataModelPath : IStorageModel, IDisposable, IPluginFeatureDependent _segments = []; - Save(); Initialize(); + Save(); SubscribeToDataModelStore(); } @@ -67,8 +67,8 @@ public class DataModelPath : IStorageModel, IDisposable, IPluginFeatureDependent _segments = []; - Save(); Initialize(); + Save(); SubscribeToDataModelStore(); } @@ -165,11 +165,20 @@ public class DataModelPath : IStorageModel, IDisposable, IPluginFeatureDependent // 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); + } + /// /// Gets the property description of the property this path points to /// diff --git a/src/Artemis.Core/Models/Profile/DataModel/DataModelPathSegment.cs b/src/Artemis.Core/Models/Profile/DataModel/DataModelPathSegment.cs index f1c71bd34..c1db971bc 100644 --- a/src/Artemis.Core/Models/Profile/DataModel/DataModelPathSegment.cs +++ b/src/Artemis.Core/Models/Profile/DataModel/DataModelPathSegment.cs @@ -286,8 +286,16 @@ public class DataModelPathSegment : IDisposable 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(); + } } /// diff --git a/src/Artemis.Core/Plugins/Modules/DataModel.cs b/src/Artemis.Core/Plugins/Modules/DataModel.cs index 2d82d8037..12a3e5b97 100644 --- a/src/Artemis.Core/Plugins/Modules/DataModel.cs +++ b/src/Artemis.Core/Plugins/Modules/DataModel.cs @@ -289,7 +289,7 @@ public abstract class DataModel public void RemoveDynamicChild(DynamicChild dynamicChild) { if (dynamicChild == null) throw new ArgumentNullException(nameof(dynamicChild)); - List keys = _dynamicChildren.Where(kvp => kvp.Value.BaseValue == dynamicChild).Select(kvp => kvp.Key).ToList(); + List keys = _dynamicChildren.Where(kvp => kvp.Value == dynamicChild).Select(kvp => kvp.Key).ToList(); foreach (string key in keys) { _dynamicChildren.Remove(key); @@ -335,8 +335,10 @@ public abstract class DataModel { 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); } }