From f917748809bb0eb9bcdb686a54fe4e486cd179ff Mon Sep 17 00:00:00 2001 From: Leon Friedrich <60421075+ElectroJr@users.noreply.github.com> Date: Tue, 25 Apr 2023 15:58:10 +1200 Subject: [PATCH] Make map loading log errors on invalid UIDs (#3970) --- .../EntitySystems/MapLoaderSystem.cs | 30 +++++++------- Robust.Shared/Map/MapSerializationContext.cs | 40 ++++++++++--------- Robust.Shared/Prototypes/EntityPrototype.cs | 9 +++++ .../Server/Maps/MapLoaderTest.cs | 11 +++-- 4 files changed, 51 insertions(+), 39 deletions(-) diff --git a/Robust.Server/GameObjects/EntitySystems/MapLoaderSystem.cs b/Robust.Server/GameObjects/EntitySystems/MapLoaderSystem.cs index 7aff813626..8c1c8bcffd 100644 --- a/Robust.Server/GameObjects/EntitySystems/MapLoaderSystem.cs +++ b/Robust.Server/GameObjects/EntitySystems/MapLoaderSystem.cs @@ -368,7 +368,7 @@ public sealed class MapLoaderSystem : EntitySystem var entities = data.RootMappingNode.Get("entities"); var mapUid = _mapManager.GetMapEntityId(data.TargetMap); var pauseTime = mapUid.IsValid() ? _meta.GetPauseTime(mapUid) : TimeSpan.Zero; - _context.Set(data.UidEntityMap, new Dictionary(), pauseTime); + _context.Set(data.UidEntityMap, new Dictionary(), pauseTime, null); data.Entities.EnsureCapacity(entities.Count); data.UidEntityMap.EnsureCapacity(entities.Count); data.EntitiesToDeserialize.EnsureCapacity(entities.Count); @@ -381,14 +381,7 @@ public sealed class MapLoaderSystem : EntitySystem type = typeNode.Value; } - // TODO Fix this. If the entities are ever defined out of order, and if one of them does not have a - // "uid" node, then defaulting to Entities.Count will error. - var uid = data.Entities.Count; - - if (entityDef.TryGet("uid", out var uidNode)) - { - uid = uidNode.AsInt(); - } + var uid = entityDef.Get("uid").AsInt(); var entity = _serverEntityManager.AllocEntity(type); data.Entities.Add(entity); @@ -440,7 +433,10 @@ public sealed class MapLoaderSystem : EntitySystem compType, new[] { protData.Mapping }, datanode, _context); } + + _context.CurrentComponent = value; _context.CurrentReadingEntityComponents[value] = (IComponent) _serManager.Read(compType, datanode, _context)!; + _context.CurrentComponent = null; } } @@ -814,7 +810,9 @@ public sealed class MapLoaderSystem : EntitySystem _logLoader.Debug($"Populated entity list in {_stopwatch.Elapsed}"); var pauseTime = _meta.GetPauseTime(uid); - _context.Set(uidEntityMap, entityUidMap, pauseTime); + + var rootXform = _serverEntityManager.GetComponent(uid); + _context.Set(uidEntityMap, entityUidMap, pauseTime, rootXform.ParentUid); _stopwatch.Restart(); WriteEntitySection(data, uidEntityMap, entityUidMap); @@ -879,7 +877,7 @@ public sealed class MapLoaderSystem : EntitySystem RecursivePopulate(uid, entities, uidEntityMap, withoutUid, metaCompQuery, transformCompQuery, saveCompQuery); - var uidCounter = 0; + var uidCounter = 1; foreach (var entity in withoutUid) { while (uidEntityMap.ContainsKey(uidCounter)) @@ -958,9 +956,9 @@ public sealed class MapLoaderSystem : EntitySystem var emptyMetaNode = _serManager.WriteValueAs(typeof(MetaDataComponent), new MetaDataComponent(), alwaysWrite: true, context: _context); - _context.CurrentWritingComponent = _factory.GetComponentName(typeof(TransformComponent)); + _context.CurrentComponent = _factory.GetComponentName(typeof(TransformComponent)); var emptyXformNode = _serManager.WriteValueAs(typeof(TransformComponent), new TransformComponent(), alwaysWrite: true, context: _context); - _context.CurrentWritingComponent = null; + _context.CurrentComponent = null; foreach (var (saveId, entityUid) in uidEntityMap.OrderBy( e=> e.Key)) { @@ -984,11 +982,11 @@ public sealed class MapLoaderSystem : EntitySystem foreach (var (compType, comp) in prototype.Components) { - _context.CurrentWritingComponent = compType; + _context.CurrentComponent = compType; cache.Add(compType, _serManager.WriteValueAs(comp.Component.GetType(), comp.Component, alwaysWrite: true, context: _context)); } - _context.CurrentWritingComponent = null; + _context.CurrentComponent = null; cache.TryAdd("MetaData", emptyMetaNode); cache.TryAdd("Transform", emptyXformNode); } @@ -1010,7 +1008,7 @@ public sealed class MapLoaderSystem : EntitySystem var compType = component.GetType(); var compName = _factory.GetComponentName(compType); - _context.CurrentWritingComponent = compName; + _context.CurrentComponent = compName; MappingDataNode? compMapping; MappingDataNode? protMapping = null; if (cache != null && cache.TryGetValue(compName, out protMapping)) diff --git a/Robust.Shared/Map/MapSerializationContext.cs b/Robust.Shared/Map/MapSerializationContext.cs index f0ef4f2bc4..f0abb847fb 100644 --- a/Robust.Shared/Map/MapSerializationContext.cs +++ b/Robust.Shared/Map/MapSerializationContext.cs @@ -24,7 +24,7 @@ internal sealed class MapSerializationContext : ISerializationContext, IEntityLo public Dictionary? TileMap; public readonly Dictionary CurrentReadingEntityComponents = new(); public HashSet CurrentlyIgnoredComponents = new(); - public string? CurrentWritingComponent; + public string? CurrentComponent; public EntityUid? CurrentWritingEntity; private Dictionary _uidEntityMap = new(); @@ -35,23 +35,31 @@ internal sealed class MapSerializationContext : ISerializationContext, IEntityLo /// public TimeSpan PauseTime; + /// + /// The parent of the entity being saved, This entity is not itself getting saved. + /// + private EntityUid? _parentUid; + public MapSerializationContext() { SerializerProvider.RegisterSerializer(this); } - public void Set(Dictionary uidEntityMap, Dictionary entityUidMap, TimeSpan pauseTime) + public void Set(Dictionary uidEntityMap, Dictionary entityUidMap, + TimeSpan pauseTime, EntityUid? parentUid) { _uidEntityMap = uidEntityMap; _entityUidMap = entityUidMap; PauseTime = pauseTime; + if (parentUid != null && parentUid.Value.IsValid()) + _parentUid = parentUid; } public void Clear() { CurrentReadingEntityComponents.Clear(); CurrentlyIgnoredComponents.Clear(); - CurrentWritingComponent = null; + CurrentComponent = null; CurrentWritingEntity = null; PauseTime = TimeSpan.Zero; } @@ -94,12 +102,13 @@ internal sealed class MapSerializationContext : ISerializationContext, IEntityLo { if (!_entityUidMap.TryGetValue(value, out var entityUidMapped)) { - // Terrible hack to mute this error on the grids themselves when serializing blueprints. - if (value.IsValid() || CurrentWritingComponent != "Transform") + if (CurrentComponent == "Transform") { - Logger.ErrorS("map", "Encountered an invalid entityUid '{0}' while serializing a map.", value); + if (!value.IsValid() || value == _parentUid) + return new ValueDataNode("invalid"); } + Logger.ErrorS("map", "Encountered an invalid entityUid '{0}' while serializing a map.", value); return new ValueDataNode("invalid"); } @@ -112,22 +121,15 @@ internal sealed class MapSerializationContext : ISerializationContext, IEntityLo SerializationHookContext hookCtx, ISerializationContext? context, ISerializationManager.InstantiationDelegate? _) { - if (node.Value == "invalid") - { + if (node.Value == "invalid" && CurrentComponent == "Transform") return EntityUid.Invalid; - } - var val = int.Parse(node.Value); - - if (!_uidEntityMap.TryGetValue(val, out var entity)) - { - Logger.ErrorS("map", "Error in map file: found local entity UID '{0}' which does not exist.", val); - return EntityUid.Invalid; - } - else - { + if (int.TryParse(node.Value, out var val) && _uidEntityMap.TryGetValue(val, out var entity)) return entity; - } + + Logger.ErrorS("map", "Error in map file: found local entity UID '{0}' which does not exist.", val); + return EntityUid.Invalid; + } [MustUseReturnValue] diff --git a/Robust.Shared/Prototypes/EntityPrototype.cs b/Robust.Shared/Prototypes/EntityPrototype.cs index a908b9437c..30994ce77c 100644 --- a/Robust.Shared/Prototypes/EntityPrototype.cs +++ b/Robust.Shared/Prototypes/EntityPrototype.cs @@ -4,6 +4,7 @@ using System.Diagnostics.CodeAnalysis; using Robust.Shared.GameObjects; using Robust.Shared.IoC; using Robust.Shared.Localization; +using Robust.Shared.Map; using Robust.Shared.Maths; using Robust.Shared.Serialization; using Robust.Shared.Serialization.Manager; @@ -244,7 +245,15 @@ namespace Robust.Shared.Prototypes component = newComponent; } + if (context is not MapSerializationContext map) + { + serManager.CopyTo(data, ref component, context, notNullableOverride: true); + return; + } + + map.CurrentComponent = compName; serManager.CopyTo(data, ref component, context, notNullableOverride: true); + map.CurrentComponent = null; } public override string ToString() diff --git a/Robust.UnitTesting/Server/Maps/MapLoaderTest.cs b/Robust.UnitTesting/Server/Maps/MapLoaderTest.cs index b8b6f41137..3ddf4fc971 100644 --- a/Robust.UnitTesting/Server/Maps/MapLoaderTest.cs +++ b/Robust.UnitTesting/Server/Maps/MapLoaderTest.cs @@ -99,11 +99,14 @@ entities: entMan.EnsureComponent(mapUid); var mapLoad = IoCManager.Resolve().GetEntitySystem(); - var geid = mapLoad.LoadGrid(mapId, "/TestMap.yml"); + if (!mapLoad.TryLoad(mapId, "/TestMap.yml", out var root) + || root.FirstOrDefault() is not { Valid:true } geid) + { + Assert.Fail(); + return; + } - Assert.That(geid, NUnit.Framework.Is.Not.Null); - - var entity = entMan.GetComponent(geid!.Value).Children.Single().Owner; + var entity = entMan.GetComponent(geid).Children.Single().Owner; var c = entMan.GetComponent(entity); Assert.That(c.Bar, Is.EqualTo(2));