From eee3756a719425cbbf59307eddca35b6fd43ecef Mon Sep 17 00:00:00 2001 From: ElectroJr Date: Mon, 23 Dec 2024 16:41:24 +1300 Subject: [PATCH 1/2] Try fix entity deserialization component composition --- .../EntitySystems/MapLoaderSystem.cs | 29 ++++++++++++--- .../Manager/ISerializationManager.cs | 7 ++++ .../SerializationManager.Composition.cs | 22 ++++------- .../Markdown/Mapping/MappingDataNode.cs | 37 +++++++++++++++++++ 4 files changed, 76 insertions(+), 19 deletions(-) diff --git a/Robust.Server/GameObjects/EntitySystems/MapLoaderSystem.cs b/Robust.Server/GameObjects/EntitySystems/MapLoaderSystem.cs index 98644cb6128..a1fa08bc229 100644 --- a/Robust.Server/GameObjects/EntitySystems/MapLoaderSystem.cs +++ b/Robust.Server/GameObjects/EntitySystems/MapLoaderSystem.cs @@ -545,8 +545,6 @@ private void LoadEntity(EntityUid uid, MappingDataNode data, MetaDataComponent m _context.CurrentReadingEntityComponents.EnsureCapacity(componentList.Count); foreach (var compData in componentList.Cast()) { - var datanode = compData.Copy(); - datanode.Remove("type"); var value = ((ValueDataNode)compData["type"]).Value; if (!_factory.TryGetRegistration(value, out var reg)) { @@ -555,15 +553,36 @@ private void LoadEntity(EntityUid uid, MappingDataNode data, MetaDataComponent m continue; } + + MappingDataNode datanode; var compType = reg.Type; if (prototype?.Components != null && prototype.Components.TryGetValue(value, out var protData)) { - datanode = - _serManager.PushCompositionWithGenericNode( + // Previously this method used generic composition pushing. I.e.: + /* + datanode = _serManager.PushCompositionWithGenericNode( compType, - new[] { protData.Mapping }, datanode, _context); + new[] {protData.Mapping}, + datanode, + _context); + */ + // However, I don't think this is what we want to do here. I.e., we want to ignore things like the + // AlwaysPushInheritanceAttribute. Complex inheritance pushing should have already been done when + // creating the proto data. Now we just want to override the prototype information with the + // serialized data. + // + // If we do ever want to support this, we need to change entity serialization so that it doesn't do + // a simple diff with respect to the prototype data and instead does some kind of inheritance + // subtraction / removal. + + datanode = _serManager.CombineMappings(compData, protData.Mapping); + } + else + { + datanode = compData.ShallowClone(); } + datanode.Remove("type"); _context.CurrentComponent = value; _context.CurrentReadingEntityComponents[value] = (IComponent) _serManager.Read(compType, datanode, _context)!; _context.CurrentComponent = null; diff --git a/Robust.Shared/Serialization/Manager/ISerializationManager.cs b/Robust.Shared/Serialization/Manager/ISerializationManager.cs index 22fd2c56e17..247ab142d2b 100644 --- a/Robust.Shared/Serialization/Manager/ISerializationManager.cs +++ b/Robust.Shared/Serialization/Manager/ISerializationManager.cs @@ -3,6 +3,7 @@ using JetBrains.Annotations; using Robust.Shared.Reflection; using Robust.Shared.Serialization.Markdown; +using Robust.Shared.Serialization.Markdown.Mapping; using Robust.Shared.Serialization.Markdown.Validation; using Robust.Shared.Serialization.TypeSerializers.Interfaces; @@ -440,6 +441,12 @@ public TNode PushCompositionWithGenericNode(Type type, TNode[] parents, T return (TNode) PushComposition(type, parents, child, context); } + /// + /// Simple inheritance pusher clones data and overrides a parent's values with + /// the child's. + /// + MappingDataNode CombineMappings(MappingDataNode child, MappingDataNode parent); + #endregion public bool TryGetVariableType(Type type, string variableName, [NotNullWhen(true)] out Type? variableType); diff --git a/Robust.Shared/Serialization/Manager/SerializationManager.Composition.cs b/Robust.Shared/Serialization/Manager/SerializationManager.Composition.cs index bdb40e6df3a..7011d4fae3c 100644 --- a/Robust.Shared/Serialization/Manager/SerializationManager.Composition.cs +++ b/Robust.Shared/Serialization/Manager/SerializationManager.Composition.cs @@ -95,7 +95,7 @@ private PushCompositionDelegate GetOrCreatePushCompositionDelegate(Type type, Da Expression.Convert(parentParam, nodeType)), MappingDataNode => Expression.Call( instanceConst, - nameof(PushInheritanceMapping), + nameof(CombineMappings), Type.EmptyTypes, Expression.Convert(childParam, nodeType), Expression.Convert(parentParam, nodeType)), @@ -117,32 +117,26 @@ private SequenceDataNode PushInheritanceSequence(SequenceDataNode child, Sequenc //todo implement different inheritancebehaviours for yamlfield // I have NFI what this comment means. - var result = new SequenceDataNode(child.Count + parent.Count); + var result = child.Copy(); foreach (var entry in parent) { - result.Add(entry); - } - foreach (var entry in child) - { - result.Add(entry); + result.Add(entry.Copy()); } return result; } - private MappingDataNode PushInheritanceMapping(MappingDataNode child, MappingDataNode parent) + public MappingDataNode CombineMappings(MappingDataNode child, MappingDataNode parent) { //todo implement different inheritancebehaviours for yamlfield // I have NFI what this comment means. + // I still don't know what it means, but if it's talking about the always/never push inheritance attributes, + // make sure it doesn't break entity serialization. - var result = new MappingDataNode(child.Count + parent.Count); + var result = child.Copy(); foreach (var (k, v) in parent) { - result[k] = v; - } - foreach (var (k, v) in child) - { - result[k] = v; + result.TryAddCopy(k, v); } return result; diff --git a/Robust.Shared/Serialization/Markdown/Mapping/MappingDataNode.cs b/Robust.Shared/Serialization/Markdown/Mapping/MappingDataNode.cs index 598c3d1b046..777eeb2a96f 100644 --- a/Robust.Shared/Serialization/Markdown/Mapping/MappingDataNode.cs +++ b/Robust.Shared/Serialization/Markdown/Mapping/MappingDataNode.cs @@ -3,6 +3,7 @@ using System.Collections.Generic; using System.Diagnostics.CodeAnalysis; using System.Linq; +using System.Runtime.InteropServices; using System.Threading; using Robust.Shared.Serialization.Markdown.Value; using Robust.Shared.Utility; @@ -272,6 +273,26 @@ public override MappingDataNode Copy() return newMapping; } + /// + /// Variant of that doesn't clone the keys or values. + /// + public MappingDataNode ShallowClone() + { + var newMapping = new MappingDataNode(_children.Count) + { + Tag = Tag, + Start = Start, + End = End + }; + + foreach (var (key, val) in _list) + { + newMapping.Add(key, val); + } + + return newMapping; + } + /// /// Variant of that will recursively call except rather than only checking equality. /// @@ -396,5 +417,21 @@ public bool Remove(KeyValuePair item) public int Count => _children.Count; public bool IsReadOnly => false; + + + public bool TryAdd(DataNode key, DataNode value) + { + return _children.TryAdd(key, value); + } + + public bool TryAddCopy(DataNode key, DataNode value) + { + ref var entry = ref CollectionsMarshal.GetValueRefOrAddDefault(_children, key, out var exists); + if (exists) + return false; + + entry = value.Copy(); + return true; + } } } From 043ac1a1ac110e9bc4291c1bb0e36637f87c28e0 Mon Sep 17 00:00:00 2001 From: ElectroJr Date: Mon, 23 Dec 2024 16:51:21 +1300 Subject: [PATCH 2/2] CL --- RELEASE-NOTES.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/RELEASE-NOTES.md b/RELEASE-NOTES.md index a0daaf128e8..bde24625491 100644 --- a/RELEASE-NOTES.md +++ b/RELEASE-NOTES.md @@ -43,7 +43,7 @@ END TEMPLATE--> ### Bugfixes -*None yet* +* Fixed entity deserialization for components with a data fields that have a AlwaysPushInheritance Attribute ### Other