diff --git a/Content.Tests/DMProject/Tests/Image/Image.dm b/Content.Tests/DMProject/Broken Tests/Image/Image.dm similarity index 100% rename from Content.Tests/DMProject/Tests/Image/Image.dm rename to Content.Tests/DMProject/Broken Tests/Image/Image.dm diff --git a/Content.Tests/DMProject/Tests/Image/subclass.dm b/Content.Tests/DMProject/Broken Tests/Image/subclass.dm similarity index 100% rename from Content.Tests/DMProject/Tests/Image/subclass.dm rename to Content.Tests/DMProject/Broken Tests/Image/subclass.dm diff --git a/OpenDreamClient/Rendering/ClientAppearanceSystem.cs b/OpenDreamClient/Rendering/ClientAppearanceSystem.cs index a7a472e0f8..c6fb2b43a2 100644 --- a/OpenDreamClient/Rendering/ClientAppearanceSystem.cs +++ b/OpenDreamClient/Rendering/ClientAppearanceSystem.cs @@ -24,6 +24,7 @@ internal sealed class ClientAppearanceSystem : SharedAppearanceSystem { public override void Initialize() { SubscribeNetworkEvent(OnNewAppearance); + SubscribeNetworkEvent(e => _appearances.Remove(e.AppearanceId)); SubscribeNetworkEvent(OnAnimation); SubscribeLocalEvent(OnWorldAABB); } diff --git a/OpenDreamRuntime/AtomManager.cs b/OpenDreamRuntime/AtomManager.cs index b9b7ae5b67..a4c70cf331 100644 --- a/OpenDreamRuntime/AtomManager.cs +++ b/OpenDreamRuntime/AtomManager.cs @@ -32,10 +32,18 @@ public sealed class AtomManager { private readonly Dictionary _entityToAtom = new(); private readonly Dictionary _definitionAppearanceCache = new(); - private ServerAppearanceSystem AppearanceSystem => _appearanceSystem ??= _entitySystemManager.GetEntitySystem(); + private ServerAppearanceSystem? AppearanceSystem{ + get { + if(_appearanceSystem is null) + _entitySystemManager.TryGetEntitySystem(out _appearanceSystem); + return _appearanceSystem; + } + } private ServerVerbSystem VerbSystem => _verbSystem ??= _entitySystemManager.GetEntitySystem(); private ServerAppearanceSystem? _appearanceSystem; private ServerVerbSystem? _verbSystem; + private DMISpriteSystem DMISpriteSystem => _dmiSpriteSystem ??= _entitySystemManager.GetEntitySystem(); + private DMISpriteSystem? _dmiSpriteSystem; // ReSharper disable ForCanBeConvertedToForeach (the collections could be added to) public IEnumerable EnumerateAtoms(TreeEntry? filterType = null) { @@ -191,7 +199,7 @@ public sealed class AtomManager { var entity = _entityManager.SpawnEntity(null, new MapCoordinates(0, 0, MapId.Nullspace)); DMISpriteComponent sprite = _entityManager.AddComponent(entity); - sprite.SetAppearance(GetAppearanceFromDefinition(movable.ObjectDefinition)); + DMISpriteSystem.SetSpriteAppearance(new(entity, sprite), GetAppearanceFromDefinition(movable.ObjectDefinition)); _entityToAtom.Add(entity, movable); return entity; @@ -446,7 +454,7 @@ public sealed class AtomManager { /// The atom to find the appearance of. public IconAppearance? MustGetAppearance(DreamObject atom) { return atom switch { - DreamObjectTurf turf => AppearanceSystem.MustGetAppearance(turf.AppearanceId), + DreamObjectTurf turf => AppearanceSystem?.MustGetAppearance(turf.AppearanceId), DreamObjectMovable movable => movable.SpriteComponent.Appearance, DreamObjectArea area => AppearanceSystem.MustGetAppearance(area.AppearanceId), DreamObjectImage image => image.Appearance, @@ -459,7 +467,7 @@ public sealed class AtomManager { /// public bool TryGetAppearance(DreamObject atom, [NotNullWhen(true)] out IconAppearance? appearance) { if (atom is DreamObjectTurf turf) - appearance = AppearanceSystem.MustGetAppearance(turf.AppearanceId); + appearance = AppearanceSystem?.MustGetAppearance(turf.AppearanceId); else if (atom is DreamObjectMovable movable) appearance = movable.SpriteComponent.Appearance; else if (atom is DreamObjectImage image) @@ -475,7 +483,6 @@ public sealed class AtomManager { public void UpdateAppearance(DreamObject atom, Action update) { var appearance = MustGetAppearance(atom); appearance = (appearance != null) ? new(appearance) : new(); // Clone the appearance - update(appearance); SetAtomAppearance(atom, appearance); } @@ -484,7 +491,7 @@ public sealed class AtomManager { if (atom is DreamObjectTurf turf) { _dreamMapManager.SetTurfAppearance(turf, appearance); } else if (atom is DreamObjectMovable movable) { - movable.SpriteComponent.SetAppearance(appearance); + DMISpriteSystem.SetSpriteAppearance(new(movable.Entity, movable.SpriteComponent), appearance); } else if (atom is DreamObjectImage image) { image.Appearance = appearance; } else if (atom is DreamObjectArea area) { @@ -492,6 +499,14 @@ public sealed class AtomManager { } } + public void SetMovableScreenLoc(DreamObjectMovable movable, ScreenLocation screenLocation) { + DMISpriteSystem.SetSpriteScreenLocation(new(movable.Entity, movable.SpriteComponent), screenLocation); + } + + public void SetSpriteAppearance(Entity ent, IconAppearance appearance) { + DMISpriteSystem.SetSpriteAppearance(ent, appearance); + } + public void AnimateAppearance(DreamObject atom, TimeSpan duration, AnimationEasing easing, int loop, AnimationFlags flags, int delay, bool chainAnim, Action animate) { if (atom is not DreamObjectMovable movable) return; //Animating non-movables is unimplemented TODO: should handle images and maybe filters @@ -501,11 +516,11 @@ public sealed class AtomManager { animate(appearance); // Don't send the updated appearance to clients, they will animate it - movable.SpriteComponent.SetAppearance(appearance, dirty: false); + DMISpriteSystem.SetSpriteAppearance(new(movable.Entity, movable.SpriteComponent), appearance, dirty: false); NetEntity ent = _entityManager.GetNetEntity(movable.Entity); - AppearanceSystem.Animate(ent, appearance, duration, easing, loop, flags, delay, chainAnim); + AppearanceSystem?.Animate(ent, appearance, duration, easing, loop, flags, delay, chainAnim); } public bool TryCreateAppearanceFrom(DreamValue value, [NotNullWhen(true)] out IconAppearance? appearance) { @@ -595,6 +610,7 @@ public sealed class AtomManager { } _definitionAppearanceCache.Add(def, appearance); + AppearanceSystem?.IncreaseAppearanceRefCount(appearance); return appearance; } diff --git a/OpenDreamRuntime/DreamManager.cs b/OpenDreamRuntime/DreamManager.cs index 46ee16f661..466c8a3c3c 100644 --- a/OpenDreamRuntime/DreamManager.cs +++ b/OpenDreamRuntime/DreamManager.cs @@ -224,6 +224,7 @@ public sealed partial class DreamManager { refType = RefType.DreamAppearance; _appearanceSystem ??= _entitySystemManager.GetEntitySystem(); idx = (int)_appearanceSystem.AddAppearance(appearance); + _appearanceSystem.IncreaseAppearanceRefCount(appearance); } else if (value.TryGetValueAsDreamResource(out var refRsc)) { refType = RefType.DreamResource; idx = refRsc.Id; diff --git a/OpenDreamRuntime/DreamMapManager.cs b/OpenDreamRuntime/DreamMapManager.cs index 5f8ee2fbd3..dc20b6e526 100644 --- a/OpenDreamRuntime/DreamMapManager.cs +++ b/OpenDreamRuntime/DreamMapManager.cs @@ -216,8 +216,10 @@ public sealed class DreamMapManager : IDreamMapManager { _turfAreaLookup.Clear(); int oldAppearance = area.AppearanceId; area.AppearanceId = _appearanceSystem.AddAppearance(appearance); + _appearanceSystem.DecreaseAppearanceRefCount(oldAppearance); + _appearanceSystem.IncreaseAppearanceRefCount(area.AppearanceId); foreach (var turf in area.Contents.GetTurfs()) { - var turfAppearance = _atomManager.MustGetAppearance(turf); + IconAppearance? turfAppearance = new(_atomManager.MustGetAppearance(turf)); if(turfAppearance is null) continue; diff --git a/OpenDreamRuntime/Objects/Types/DreamObjectImage.cs b/OpenDreamRuntime/Objects/Types/DreamObjectImage.cs index 06c5c87e30..58259f5b4f 100644 --- a/OpenDreamRuntime/Objects/Types/DreamObjectImage.cs +++ b/OpenDreamRuntime/Objects/Types/DreamObjectImage.cs @@ -6,7 +6,8 @@ namespace OpenDreamRuntime.Objects.Types; public sealed class DreamObjectImage : DreamObject { - public IconAppearance? Appearance; + private IconAppearance? _appearance; + public IconAppearance? Appearance { get => _appearance; set => SetAppearance(value); } private DreamObject? _loc; private DreamList _overlays; @@ -42,7 +43,8 @@ public sealed class DreamObjectImage : DreamObject { base.Initialize(args); DreamValue icon = args.GetArgument(0); - if (icon.IsNull || !AtomManager.TryCreateAppearanceFrom(icon, out Appearance)) { + IconAppearance? iconAppearance = null; + if (icon.IsNull || !AtomManager.TryCreateAppearanceFrom(icon, out iconAppearance)) { // Use a default appearance, but log a warning about it if icon wasn't null Appearance = new(AtomManager.GetAppearanceFromDefinition(ObjectDefinition)); if (!icon.IsNull) @@ -50,25 +52,36 @@ public sealed class DreamObjectImage : DreamObject { .Warning($"Attempted to create an /image from {icon}. This is invalid and a default image was created instead."); } + if(iconAppearance is not null) + Appearance = iconAppearance; + + int argIndex = 1; DreamValue loc = args.GetArgument(1); if (loc.TryGetValueAsDreamObject(out _loc)) { // If it's not a DreamObject, it's actually icon_state and not loc argIndex = 2; } + iconAppearance = null; //mutate the appearance and then set it at the end to handle ref counts and client update foreach (string argName in IconCreationArgs) { var arg = args.GetArgument(argIndex++); if (arg.IsNull) continue; + iconAppearance ??= new(Appearance!); - AtomManager.SetAppearanceVar(Appearance, argName, arg); + AtomManager.SetAppearanceVar(iconAppearance, argName, arg); if (argName == "dir") { // If a dir is explicitly given in the constructor then overlays using this won't use their owner's dir // Setting dir after construction does not affect this // This is undocumented and I hate it - Appearance.InheritsDirection = false; + iconAppearance.InheritsDirection = false; } } + + if (iconAppearance is not null) { + AppearanceSystem!.AddAppearance(iconAppearance); // this is a no-op if the appearance is already in the system + Appearance = iconAppearance; + } } protected override bool TryGetVar(string varName, out DreamValue value) { @@ -110,7 +123,7 @@ public sealed class DreamObjectImage : DreamObject { Appearance = newAppearance; if(_entity != EntityUid.Invalid) { DMISpriteComponent sprite = EntityManager.GetComponent(_entity); - sprite.SetAppearance(Appearance!); + AtomManager.SetSpriteAppearance(new(_entity, sprite), Appearance); } break; @@ -207,10 +220,12 @@ public sealed class DreamObjectImage : DreamObject { } default: if (AtomManager.IsValidAppearanceVar(varName)) { - AtomManager.SetAppearanceVar(Appearance!, varName, value); + IconAppearance iconAppearance = new(Appearance!); + AtomManager.SetAppearanceVar(iconAppearance, varName, value); + Appearance = iconAppearance; if(_entity != EntityUid.Invalid) { DMISpriteComponent sprite = EntityManager.GetComponent(_entity); - sprite.SetAppearance(Appearance!); + AtomManager.SetSpriteAppearance(new(_entity, sprite), Appearance!); } break; @@ -233,13 +248,24 @@ public sealed class DreamObjectImage : DreamObject { if(_entity == EntityUid.Invalid) { _entity = EntityManager.SpawnEntity(null, new MapCoordinates(0, 0, MapId.Nullspace)); DMISpriteComponent sprite = EntityManager.AddComponent(_entity); - sprite.SetAppearance(Appearance!); + if(Appearance is not null) + AtomManager.SetSpriteAppearance(new(_entity, sprite), Appearance); } return _entity; } + public void SetAppearance(IconAppearance? appearance) { + if(appearance is not null) + AppearanceSystem!.IncreaseAppearanceRefCount(appearance); + if(_appearance is not null) + AppearanceSystem!.DecreaseAppearanceRefCount(_appearance); + _appearance = appearance; + } + protected override void HandleDeletion() { + if(_appearance is not null) + AppearanceSystem!.DecreaseAppearanceRefCount(_appearance); if(_entity != EntityUid.Invalid) { EntityManager.DeleteEntity(_entity); } diff --git a/OpenDreamRuntime/Objects/Types/DreamObjectMovable.cs b/OpenDreamRuntime/Objects/Types/DreamObjectMovable.cs index 36c3ee56ab..4dacc228cc 100644 --- a/OpenDreamRuntime/Objects/Types/DreamObjectMovable.cs +++ b/OpenDreamRuntime/Objects/Types/DreamObjectMovable.cs @@ -22,15 +22,7 @@ public class DreamObjectMovable : DreamObjectAtom { private string? ScreenLoc { get => _screenLoc; - set { - _screenLoc = value; - if (!EntityManager.TryGetComponent(Entity, out var sprite)) - return; - - sprite.ScreenLocation = !string.IsNullOrEmpty(value) ? - new ScreenLocation(value) : - new ScreenLocation(0, 0, 0, 0); - } + set => SetScreenLoc(value); } private string? _screenLoc; @@ -198,4 +190,9 @@ public class DreamObjectMovable : DreamObjectAtom { throw new ArgumentException($"Invalid loc {loc}"); } } + + private void SetScreenLoc(string? screenLoc) { + _screenLoc = screenLoc; + AtomManager.SetMovableScreenLoc(this, !string.IsNullOrEmpty(screenLoc) ? new ScreenLocation(screenLoc) : new ScreenLocation(0, 0, 0, 0)); + } } diff --git a/OpenDreamRuntime/Objects/Types/DreamObjectTurf.cs b/OpenDreamRuntime/Objects/Types/DreamObjectTurf.cs index 3ceb6e995a..33e1a1079f 100644 --- a/OpenDreamRuntime/Objects/Types/DreamObjectTurf.cs +++ b/OpenDreamRuntime/Objects/Types/DreamObjectTurf.cs @@ -4,7 +4,9 @@ public sealed class DreamObjectTurf : DreamObjectAtom { public readonly int X, Y, Z; public readonly IDreamMapManager.Cell Cell; public readonly TurfContentsList Contents; - public int AppearanceId; + public int AppearanceId { get => _appearanceId!.Value; set => SetAppearanceId(value); } + + private int? _appearanceId; public DreamObjectTurf(DreamObjectDefinition objectDefinition, int x, int y, int z, IDreamMapManager.Cell cell) : base(objectDefinition) { X = x; @@ -63,4 +65,13 @@ public sealed class DreamObjectTurf : DreamObjectAtom { break; } } + + public void SetAppearanceId(int appearanceId) { + AppearanceSystem!.IncreaseAppearanceRefCount(appearanceId); + if (_appearanceId is not null) { + AppearanceSystem!.DecreaseAppearanceRefCount(_appearanceId.Value); + } + + _appearanceId = appearanceId; + } } diff --git a/OpenDreamRuntime/Rendering/DMISpriteComponent.cs b/OpenDreamRuntime/Rendering/DMISpriteComponent.cs index f89e0b9047..2a774470bb 100644 --- a/OpenDreamRuntime/Rendering/DMISpriteComponent.cs +++ b/OpenDreamRuntime/Rendering/DMISpriteComponent.cs @@ -1,27 +1,15 @@ using OpenDreamShared.Dream; using OpenDreamShared.Rendering; -namespace OpenDreamRuntime.Rendering { - [RegisterComponent] - public sealed partial class DMISpriteComponent : SharedDMISpriteComponent { - [ViewVariables] - public ScreenLocation? ScreenLocation { - get => _screenLocation; - set { - _screenLocation = value; - Dirty(); - } - } - private ScreenLocation? _screenLocation; +namespace OpenDreamRuntime.Rendering; - [ViewVariables] public IconAppearance? Appearance { get; private set; } +[RegisterComponent] +public sealed partial class DMISpriteComponent : SharedDMISpriteComponent { + [ViewVariables] + [Access(typeof(DMISpriteSystem))] + public ScreenLocation ScreenLocation; - public void SetAppearance(IconAppearance? appearance, bool dirty = true) { - Appearance = appearance; - - if (dirty) { - Dirty(); - } - } - } + [Access(typeof(DMISpriteSystem))] + [ViewVariables] public IconAppearance? Appearance; } + diff --git a/OpenDreamRuntime/Rendering/DMISpriteSystem.cs b/OpenDreamRuntime/Rendering/DMISpriteSystem.cs index 7d7d7c865c..0c561b4c77 100644 --- a/OpenDreamRuntime/Rendering/DMISpriteSystem.cs +++ b/OpenDreamRuntime/Rendering/DMISpriteSystem.cs @@ -1,21 +1,41 @@ -using OpenDreamShared.Rendering; +using OpenDreamShared.Dream; +using OpenDreamShared.Rendering; using Robust.Shared.GameStates; namespace OpenDreamRuntime.Rendering; public sealed class DMISpriteSystem : EntitySystem { - [Dependency] private readonly ServerAppearanceSystem _appearance = default!; + private ServerAppearanceSystem? _appearance; + [Dependency] private readonly IEntitySystemManager _entitySystemManager = default!; public override void Initialize() { SubscribeLocalEvent(GetComponentState); + _entitySystemManager.TryGetEntitySystem(out _appearance); } private void GetComponentState(EntityUid uid, DMISpriteComponent component, ref ComponentGetState args) { int? appearanceId = null; if (component.Appearance != null) { - appearanceId = _appearance.AddAppearance(component.Appearance); + appearanceId = _appearance?.AddAppearance(component.Appearance); } args.State = new SharedDMISpriteComponent.DMISpriteComponentState(appearanceId, component.ScreenLocation); } + + public void SetSpriteAppearance(Entity ent, IconAppearance appearance, bool dirty = true) { + DMISpriteComponent component = ent.Comp; + _appearance?.IncreaseAppearanceRefCount(appearance); + if(component.Appearance is not null) + _appearance?.DecreaseAppearanceRefCount(component.Appearance); + + component.Appearance = appearance; + if(dirty) + Dirty(ent, component); + } + + public void SetSpriteScreenLocation(Entity ent, ScreenLocation screenLocation) { + DMISpriteComponent component = ent.Comp; + component.ScreenLocation = screenLocation; + Dirty(ent, component); + } } diff --git a/OpenDreamRuntime/Rendering/ServerAppearanceSystem.cs b/OpenDreamRuntime/Rendering/ServerAppearanceSystem.cs index 8f05b3b686..41510c6f65 100644 --- a/OpenDreamRuntime/Rendering/ServerAppearanceSystem.cs +++ b/OpenDreamRuntime/Rendering/ServerAppearanceSystem.cs @@ -11,8 +11,10 @@ namespace OpenDreamRuntime.Rendering; public sealed class ServerAppearanceSystem : SharedAppearanceSystem { private readonly Dictionary _appearanceToId = new(); private readonly Dictionary _idToAppearance = new(); + private readonly Dictionary _appearanceRefCounts = new(); private int _appearanceIdCounter; + /// /// This system is used by the PVS thread, we need to be thread-safe /// @@ -24,6 +26,7 @@ public sealed class ServerAppearanceSystem : SharedAppearanceSystem { //register empty appearance as ID 0 _appearanceToId.Add(IconAppearance.Default, 0); _idToAppearance.Add(0, IconAppearance.Default); + _appearanceRefCounts.Add(IconAppearance.Default, 1); _appearanceIdCounter = 1; _playerManager.PlayerStatusChanged += OnPlayerStatusChanged; } @@ -42,6 +45,67 @@ public sealed class ServerAppearanceSystem : SharedAppearanceSystem { } } + public void IncreaseAppearanceRefCount(IconAppearance appearance) { + lock (_lock) { + int count = _appearanceRefCounts.GetValueOrDefault(appearance, 0); + foreach(var overlayid in appearance.Overlays) { + IncreaseAppearanceRefCount(overlayid); + } + + foreach(var underlayid in appearance.Underlays) { + IncreaseAppearanceRefCount(underlayid); + } + + _appearanceRefCounts[appearance] = count + 1; + } + } + + public void IncreaseAppearanceRefCount(int appearanceId) { + if (!_idToAppearance.TryGetValue(appearanceId, out IconAppearance? appearance)) { + throw new InvalidOperationException("Trying to increase ref count of an appearance that doesn't exist."); + } + + IncreaseAppearanceRefCount(appearance); + } + + public void DecreaseAppearanceRefCount(IconAppearance appearance) { + lock (_lock) { + if (!_appearanceRefCounts.TryGetValue(appearance, out int count)) { + throw new InvalidOperationException($"Appearance {appearance.GetHashCode()} ref count is already 0. You might be trying to remove an appearance that was never added."); + } + + if (count == 1) { + foreach(var overlayid in appearance.Overlays) { + DecreaseAppearanceRefCount(overlayid); + } + + foreach(var underlayid in appearance.Underlays) { + DecreaseAppearanceRefCount(underlayid); + } + + if(_appearanceToId.TryGetValue(appearance, out int id)) { + if(id==0) //don't ever remove the default appearance + return; + _idToAppearance.Remove(id); + RaiseNetworkEvent(new RemoveAppearanceEvent(id)); + } + + _appearanceRefCounts.Remove(appearance); + _appearanceToId.Remove(appearance); + //let the GC sort out the rest + } else { + _appearanceRefCounts[appearance] = count - 1; + } + } + } + + public void DecreaseAppearanceRefCount(int appearanceId) { + if (!_idToAppearance.TryGetValue(appearanceId, out IconAppearance? appearance)) { + throw new InvalidOperationException("Trying to decrease ref count of an appearance that doesn't exist."); + } + + DecreaseAppearanceRefCount(appearance); + } public int AddAppearance(IconAppearance appearance) { lock (_lock) { if (!_appearanceToId.TryGetValue(appearance, out int appearanceId)) { @@ -50,7 +114,6 @@ public sealed class ServerAppearanceSystem : SharedAppearanceSystem { _idToAppearance.Add(appearanceId, appearance); RaiseNetworkEvent(new NewAppearanceEvent(appearanceId, appearance)); } - return appearanceId; } } diff --git a/OpenDreamRuntime/ServerContentIoC.cs b/OpenDreamRuntime/ServerContentIoC.cs index 77ec1706bf..2673ade59f 100644 --- a/OpenDreamRuntime/ServerContentIoC.cs +++ b/OpenDreamRuntime/ServerContentIoC.cs @@ -1,6 +1,7 @@ using OpenDreamRuntime.Objects; using OpenDreamRuntime.Procs; using OpenDreamRuntime.Procs.DebugAdapter; +using OpenDreamRuntime.Rendering; using OpenDreamRuntime.Resources; namespace OpenDreamRuntime { @@ -14,6 +15,7 @@ public static class ServerContentIoC { IoCManager.Register(); IoCManager.Register(); IoCManager.Register(); + IoCManager.Register(); #if DEBUG IoCManager.Register(); diff --git a/OpenDreamShared/Rendering/SharedAppearanceSystem.cs b/OpenDreamShared/Rendering/SharedAppearanceSystem.cs index d2d1ed5d5a..d7e95b465e 100644 --- a/OpenDreamShared/Rendering/SharedAppearanceSystem.cs +++ b/OpenDreamShared/Rendering/SharedAppearanceSystem.cs @@ -12,6 +12,11 @@ public sealed class NewAppearanceEvent(int appearanceId, IconAppearance appearan public IconAppearance Appearance { get; } = appearance; } + [Serializable, NetSerializable] + public sealed class RemoveAppearanceEvent(int appearanceId) : EntityEventArgs { + public int AppearanceId { get; } = appearanceId; + } + [Serializable, NetSerializable] public sealed class AnimationEvent(NetEntity entity, int targetAppearanceId, TimeSpan duration, AnimationEasing easing, int loop, AnimationFlags flags, int delay, bool chainAnim) : EntityEventArgs {