From abd5149245a3a89308d8a90685efce667bf2168f Mon Sep 17 00:00:00 2001 From: Leon Friedrich <60421075+ElectroJr@users.noreply.github.com> Date: Thu, 18 Sep 2025 07:44:56 +1200 Subject: [PATCH] Improve map serialization error logging & exception tolerance (#6188) * Improve map serialization error logging * Prevent remove children of erroring entities * better logging * Improve error tolerance * Even more exception tolerance * missing ! * Improve handling of category errors Helps prevents weird bugs that arise due to deleting un-initialized entities * release notes * Typo fix --------- Co-authored-by: Pieter-Jan Briers --- RELEASE-NOTES.md | 1 + .../EntitySerialization/EntityDeserializer.cs | 30 ++-- .../EntitySerialization/EntitySerializer.cs | 152 +++++++++++++----- Robust.Shared/EntitySerialization/Options.cs | 9 +- .../EntitySerialization/SerializationEnums.cs | 30 ++++ .../Systems/MapLoaderSystem.Load.cs | 15 +- 6 files changed, 179 insertions(+), 58 deletions(-) diff --git a/RELEASE-NOTES.md b/RELEASE-NOTES.md index 864442fa1c..2471f67b99 100644 --- a/RELEASE-NOTES.md +++ b/RELEASE-NOTES.md @@ -42,6 +42,7 @@ END TEMPLATE--> * `Control.OrderedChildCollection` (gotten from `.Children`) now implements `IReadOnlyList`, allowing it to be indexed directly. * `System.WeakReference` is now available in the sandbox. * `IClydeViewport` now has an `Id` and `ClearCachedResources` event. Together, these allow you to properly cache rendering resources per viewport. +* Added a new entity yaml deserialization option (`SerializationOptions.EntityExceptionBehaviour`) that can optionally make deserialization more exception tolerant. ### Bugfixes diff --git a/Robust.Shared/EntitySerialization/EntityDeserializer.cs b/Robust.Shared/EntitySerialization/EntityDeserializer.cs index ad7eb21adc..11da552364 100644 --- a/Robust.Shared/EntitySerialization/EntityDeserializer.cs +++ b/Robust.Shared/EntitySerialization/EntityDeserializer.cs @@ -685,38 +685,38 @@ public sealed class EntityDeserializer : foreach (var yamlId in MapYamlIds) { - var uid = UidMap[yamlId]; - if (_mapQuery.TryComp(uid, out var map)) + if (UidMap.TryGetValue(yamlId, out var uid) && _mapQuery.TryComp(uid, out var map)) { Result.Maps.Add((uid, map)); EntMan.EnsureComponent(uid); } else - _log.Error($"Missing map entity: {EntMan.ToPrettyString(uid)}"); + _log.Error($"Missing map entity: {EntMan.ToPrettyString(uid)}. YamlId: {yamlId}"); } foreach (var yamlId in GridYamlIds) { - var uid = UidMap[yamlId]; - if (_gridQuery.TryComp(uid, out var grid)) + if (UidMap.TryGetValue(yamlId, out var uid) && _gridQuery.TryComp(uid, out var grid)) Result.Grids.Add((uid, grid)); else - _log.Error($"Missing grid entity: {EntMan.ToPrettyString(uid)}"); + _log.Error($"Missing grid entity: {EntMan.ToPrettyString(uid)}. YamlId: {yamlId}"); } foreach (var yamlId in OrphanYamlIds) { - var uid = UidMap[yamlId]; - if (_mapQuery.HasComponent(uid) || _xformQuery.Comp(uid).ParentUid.IsValid()) - _log.Error($"Entity {EntMan.ToPrettyString(uid)} was incorrectly labelled as an orphan?"); + if (!UidMap.TryGetValue(yamlId, out var uid)) + _log.Error($"Missing orphan entity with YamlId: {yamlId}"); + else if (_mapQuery.HasComponent(uid) || _xformQuery.Comp(uid).ParentUid.IsValid()) + _log.Error($"Entity {EntMan.ToPrettyString(uid)} was incorrectly labelled as an orphan? YamlId: {yamlId}"); else Result.Orphans.Add(uid); } foreach (var yamlId in NullspaceYamlIds) { - var uid = UidMap[yamlId]; - if (_mapQuery.HasComponent(uid) || _xformQuery.Comp(uid).ParentUid.IsValid()) + if (!UidMap.TryGetValue(yamlId, out var uid)) + _log.Error($"Missing nullspace entity with YamlId: {yamlId}"); + else if (_mapQuery.HasComponent(uid) || _xformQuery.Comp(uid).ParentUid.IsValid()) _log.Error($"Entity {EntMan.ToPrettyString(uid)} was incorrectly labelled as a null-space entity?"); else Result.NullspaceEntities.Add(uid); @@ -1152,6 +1152,7 @@ public sealed class EntityDeserializer : ISerializationContext? context, ISerializationManager.InstantiationDelegate? _) { + string msg; if (node.Value == "invalid") { if (CurrentComponent == "Transform") @@ -1160,7 +1161,7 @@ public sealed class EntityDeserializer : if (!Options.LogInvalidEntities) return EntityUid.Invalid; - var msg = CurrentReadingEntity is not { } curr + msg = CurrentReadingEntity is not { } curr ? $"Encountered invalid EntityUid reference" : $"Encountered invalid EntityUid reference wile reading entity {curr.YamlId}, component: {CurrentComponent}"; _log.Error(msg); @@ -1170,7 +1171,10 @@ public sealed class EntityDeserializer : if (int.TryParse(node.Value, out var val) && UidMap.TryGetValue(val, out var entity)) return entity; - _log.Error($"Invalid yaml entity id: '{val}'"); + msg = CurrentReadingEntity is not { } ent + ? "Encountered unknown entity yaml uid" + : $"Encountered unknown entity yaml uid wile reading entity {ent.YamlId}, component: {CurrentComponent}"; + _log.Error(msg); return EntityUid.Invalid; } diff --git a/Robust.Shared/EntitySerialization/EntitySerializer.cs b/Robust.Shared/EntitySerialization/EntitySerializer.cs index 2b9b6fa124..422fd0c672 100644 --- a/Robust.Shared/EntitySerialization/EntitySerializer.cs +++ b/Robust.Shared/EntitySerialization/EntitySerializer.cs @@ -110,6 +110,11 @@ public sealed class EntitySerializer : ISerializationContext, /// public readonly Dictionary> Prototypes = new(); + /// + /// Set of entities that have encountered issues during serialization and are now being ignored. + /// + public HashSet ErroringEntities = new(); + /// /// Yaml ids of all serialized map entities. /// @@ -412,7 +417,7 @@ public sealed class EntitySerializer : ISerializationContext, // It might be possible that something could cause an entity to be included twice. // E.g., if someone serializes a grid w/o its map, and then tries to separately include the map and all its children. - // In that case, the grid would already have been serialized as a orphan. + // In that case, the grid would already have been serialized as an orphan. // uhhh.... I guess its fine? if (EntityData.ContainsKey(saveId)) return; @@ -489,6 +494,95 @@ public sealed class EntitySerializer : ISerializationContext, xform._localRotation = 0; } + try + { + SerializeComponents(uid, cache, components); + } + catch(Exception e) + { + if (Options.EntityExceptionBehaviour == EntityExceptionBehaviour.Rethrow) + { + _log.Error($"Caught exception while serializing component {CurrentComponent} of entity {EntMan.ToPrettyString(uid)}"); + throw; + } + + _log.Error($"Caught exception while serializing component {CurrentComponent} of entity {EntMan.ToPrettyString(uid)}:\n{e}"); + CurrentEntityYamlUid = 0; + CurrentEntity = null; + CurrentComponent = null; + RemoveErroringEntity(uid); + return; + } + + CurrentComponent = null; + if (components.Count != 0) + entData.Add("components", components); + + // TODO ENTITY SERIALIZATION + // Consider adding a Action? OnEntitySerialized + // I.e., allow content to modify the per-entity data? I don't know if that would actually be useful, as content + // could just as easily append a separate entity dictionary to the output that has the extra per-entity data they + // want to serialize. + + if (meta.EntityPrototype == null) + { + CurrentEntityYamlUid = 0; + CurrentEntity = null; + return; + } + + // an entity may have fewer components than the original prototype, so we need to check if any are missing. + SequenceDataNode? missingComponents = null; + foreach (var (name, comp) in meta.EntityPrototype.Components) + { + // try comp instead of has-comp as it checks whether the component is supposed to have been + // deleted. + if (EntMan.TryGetComponent(uid, comp.Component.GetType(), out _)) + continue; + + missingComponents ??= new(); + missingComponents.Add(new ValueDataNode(name)); + } + + if (missingComponents != null) + entData.Add("missingComponents", missingComponents); + + CurrentEntityYamlUid = 0; + CurrentEntity = null; + } + + /// + /// Remove an exception throwing entity (and possibly its children) from the serialized data. + /// + private void RemoveErroringEntity(EntityUid uid) + { + if (Options.EntityExceptionBehaviour == EntityExceptionBehaviour.IgnoreEntityAndChildren) + { + foreach (var child in _xformQuery.GetComponent(uid)._children) + { + RemoveErroringEntity(child); + } + } + + ErroringEntities.Add(uid); + if (!YamlUidMap.TryGetValue(uid, out var yamlId)) + return; + + Nullspace.Remove(yamlId); + Orphans.Remove(yamlId); + Maps.Remove(yamlId); + Grids.Remove(yamlId); + EntityData.Remove(yamlId); + if (_metaQuery.TryGetComponent(uid, out var meta) + && meta.EntityPrototype != null + && Prototypes.TryGetValue(meta.EntityPrototype.ID, out var proto)) + { + proto.Remove(yamlId); + } + } + + private void SerializeComponents(EntityUid uid, Dictionary? cache, SequenceDataNode components) + { foreach (var component in EntMan.GetComponentsInternal(uid)) { var compType = component.GetType(); @@ -523,48 +617,12 @@ public sealed class EntitySerializer : ISerializationContext, // Don't need to write it if nothing was written! Note that if this entity has no associated // prototype, we ALWAYS want to write the component, because merely the fact that it exists is // information that needs to be written. - if (compMapping.Children.Count != 0 || protoMapping == null) - { - compMapping.InsertAt(0, "type", new ValueDataNode(reg.Name)); - components.Add(compMapping); - } - } - - CurrentComponent = null; - if (components.Count != 0) - entData.Add("components", components); - - // TODO ENTITY SERIALIZATION - // Consider adding a Action? OnEntitySerialized - // I.e., allow content to modify the per-entity data? I don't know if that would actually be useful, as content - // could just as easily append a separate entity dictionary to the output that has the extra per-entity data they - // want to serialize. - - if (meta.EntityPrototype == null) - { - CurrentEntityYamlUid = 0; - CurrentEntity = null; - return; - } - - // an entity may have less components than the original prototype, so we need to check if any are missing. - SequenceDataNode? missingComponents = null; - foreach (var (name, comp) in meta.EntityPrototype.Components) - { - // try comp instead of has-comp as it checks whether the component is supposed to have been - // deleted. - if (EntMan.TryGetComponent(uid, comp.Component.GetType(), out _)) + if (compMapping.Children.Count == 0 && protoMapping != null) continue; - missingComponents ??= new(); - missingComponents.Add(new ValueDataNode(name)); + compMapping.InsertAt(0, "type", new ValueDataNode(reg.Name)); + components.Add(compMapping); } - - if (missingComponents != null) - entData.Add("missingComponents", missingComponents); - - CurrentEntityYamlUid = 0; - CurrentEntity = null; } private Dictionary? GetProtoCache(EntityPrototype? proto) @@ -656,7 +714,10 @@ public sealed class EntitySerializer : ISerializationContext, public SequenceDataNode WriteEntitySection() { - if (YamlIds.Count != YamlUidMap.Count || YamlIds.Count != EntityData.Count) + // Check that EntityData contains the expected number of entities. + if (Options.EntityExceptionBehaviour != EntityExceptionBehaviour.IgnoreEntity + && Options.EntityExceptionBehaviour != EntityExceptionBehaviour.IgnoreEntityAndChildren + && (YamlIds.Count != YamlUidMap.Count || YamlIds.Count != EntityData.Count)) { // Maybe someone reserved a yaml id with ReserveYamlId() or implicitly with GetId() without actually // ever serializing the entity, This can lead to references to non-existent entities. @@ -878,6 +939,7 @@ public sealed class EntitySerializer : ISerializationContext, if (YamlUidMap.TryGetValue(value, out var yamlId)) return new ValueDataNode(yamlId.ToString(CultureInfo.InvariantCulture)); + if (CurrentComponent == _xformName) { if (value == EntityUid.Invalid) @@ -886,12 +948,18 @@ public sealed class EntitySerializer : ISerializationContext, DebugTools.Assert(!Orphans.Contains(CurrentEntityYamlUid)); Orphans.Add(CurrentEntityYamlUid); - if (Options.ErrorOnOrphan && CurrentEntity != null && value != Truncate) + if (Options.ErrorOnOrphan && CurrentEntity != null && value != Truncate && !ErroringEntities.Contains(value)) _log.Error($"Serializing entity {EntMan.ToPrettyString(CurrentEntity)} without including its parent {EntMan.ToPrettyString(value)}"); return new ValueDataNode("invalid"); } + if (ErroringEntities.Contains(value)) + { + // Referenced entity already logged an error, so we just silently fail. + return new ValueDataNode("invalid"); + } + if (value == EntityUid.Invalid) { if (Options.MissingEntityBehaviour != MissingEntityBehaviour.Ignore) diff --git a/Robust.Shared/EntitySerialization/Options.cs b/Robust.Shared/EntitySerialization/Options.cs index 0237f3f9c7..1c343d204a 100644 --- a/Robust.Shared/EntitySerialization/Options.cs +++ b/Robust.Shared/EntitySerialization/Options.cs @@ -1,5 +1,4 @@ using System.Numerics; -using JetBrains.Annotations; using Robust.Shared.EntitySerialization.Components; using Robust.Shared.GameObjects; using Robust.Shared.Log; @@ -21,7 +20,13 @@ public record struct SerializationOptions public MissingEntityBehaviour MissingEntityBehaviour = MissingEntityBehaviour.IncludeNullspace; /// - /// Whether or not to log an error when serializing an entity without its parent. + /// What to do when an exception is thrown while trying to serialize an entity. The default behaviour is to abort + /// the serialization. + /// + public EntityExceptionBehaviour EntityExceptionBehaviour = EntityExceptionBehaviour.Rethrow; + + /// + /// Whether to log an error when serializing an entity without its parent. /// public bool ErrorOnOrphan = true; diff --git a/Robust.Shared/EntitySerialization/SerializationEnums.cs b/Robust.Shared/EntitySerialization/SerializationEnums.cs index f64fe4c398..1e296feb13 100644 --- a/Robust.Shared/EntitySerialization/SerializationEnums.cs +++ b/Robust.Shared/EntitySerialization/SerializationEnums.cs @@ -86,3 +86,33 @@ public enum MissingEntityBehaviour /// AutoInclude, } + + +public enum EntityExceptionBehaviour +{ + /// + /// Re-throw the exception, interrupting the serialization. + /// + Rethrow, + + /// + /// Continue serializing and simply skip/ignore this entity. May result in broken maps that log errors or simply + /// fail to load. + /// + IgnoreEntity, + + /// + /// Continue serializing and simply skip/ignore this entity and all of its children. + /// May result in broken maps that log errors or simply fail to load. + /// + IgnoreEntityAndChildren, + + // TODO SERIALIZATION + /* + /// + /// Continue the serialization while skipping over the component that caused the exception to be thrown. May result + /// in broken maps that log errors or simply fail to load. + /// + IgnoreComponent, + */ +} diff --git a/Robust.Shared/EntitySerialization/Systems/MapLoaderSystem.Load.cs b/Robust.Shared/EntitySerialization/Systems/MapLoaderSystem.Load.cs index f62000e4c8..6f349145a8 100644 --- a/Robust.Shared/EntitySerialization/Systems/MapLoaderSystem.Load.cs +++ b/Robust.Shared/EntitySerialization/Systems/MapLoaderSystem.Load.cs @@ -110,7 +110,7 @@ public sealed partial class MapLoaderSystem // Using a local deserializer instead of a cached value, both to ensure that we don't accidentally carry over // data from a previous serializations, and because some entities cause other maps/grids to be loaded during - // during mapinit. + // mapinit. var deserializer = new EntityDeserializer( _dependency, data, @@ -124,6 +124,17 @@ public sealed partial class MapLoaderSystem return false; } + // If the file isn't of the expected category, stop before we ever create any entities. + if (opts.ExpectedCategory is { } expected + && expected != deserializer.Result.Category + && deserializer.Result.Category != FileCategory.Unknown) + { + // Did someone try to load a map file as a grid or vice versa? + Log.Error($"Map {fileName} does not contain the expected data. Expected {expected} but got {deserializer.Result.Category}"); + Delete(deserializer.Result); + return false; + } + try { deserializer.CreateEntities(); @@ -135,6 +146,8 @@ public sealed partial class MapLoaderSystem throw; } + // If the map file was an older version, the category has to be inferred from the file's contents in CreateEntities() + // Hence the category is checked again here. if (opts.ExpectedCategory is { } exp && exp != deserializer.Result.Category) { // Did someone try to load a map file as a grid or vice versa?