Appearance ref counts

OpenDreamClient/Rendering/ClientAppearanceSystem.cs

@@ -24,6 +24,7 @@ internal sealed class ClientAppearanceSystem : SharedAppearanceSystem {
 
     public override void Initialize() {
         SubscribeNetworkEvent<NewAppearanceEvent>(OnNewAppearance);
+        SubscribeNetworkEvent<RemoveAppearanceEvent>(e => _appearances.Remove(e.AppearanceId));
         SubscribeNetworkEvent<AnimationEvent>(OnAnimation);
         SubscribeLocalEvent<DMISpriteComponent, WorldAABBEvent>(OnWorldAABB);
     }

OpenDreamRuntime/AtomManager.cs

@@ -32,10 +32,18 @@ public sealed class AtomManager {
     private readonly Dictionary<EntityUid, DreamObjectMovable> _entityToAtom = new();
     private readonly Dictionary<DreamObjectDefinition, IconAppearance> _definitionAppearanceCache = new();
 
-    private ServerAppearanceSystem AppearanceSystem => _appearanceSystem ??= _entitySystemManager.GetEntitySystem<ServerAppearanceSystem>();
+    private ServerAppearanceSystem? AppearanceSystem{
+                                                    get {
+                                                        if(_appearanceSystem is null)
+                                                            _entitySystemManager.TryGetEntitySystem(out _appearanceSystem);
+                                                        return _appearanceSystem;
+                                                    }
+                                                }
     private ServerVerbSystem VerbSystem => _verbSystem ??= _entitySystemManager.GetEntitySystem<ServerVerbSystem>();
     private ServerAppearanceSystem? _appearanceSystem;
     private ServerVerbSystem? _verbSystem;
+    private DMISpriteSystem DMISpriteSystem => _dmiSpriteSystem ??= _entitySystemManager.GetEntitySystem<DMISpriteSystem>();
+    private DMISpriteSystem? _dmiSpriteSystem;
 
     // ReSharper disable ForCanBeConvertedToForeach (the collections could be added to)
     public IEnumerable<DreamObjectAtom> 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<DMISpriteComponent>(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 {
     /// <param name="atom">The atom to find the appearance of.</param>
     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 => new IconAppearance(),
             DreamObjectImage image => image.Appearance,
@@ -459,7 +467,7 @@ public sealed class AtomManager {
     /// </summary>
     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)
@@ -473,7 +481,6 @@ public sealed class AtomManager {
     public void UpdateAppearance(DreamObject atom, Action<IconAppearance> update) {
         var appearance = MustGetAppearance(atom);
         appearance = (appearance != null) ? new(appearance) : new(); // Clone the appearance
-
         update(appearance);
         SetAtomAppearance(atom, appearance);
     }
@@ -482,12 +489,20 @@ 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;
         }
     }
 
+    public void SetMovableScreenLoc(DreamObjectMovable movable, ScreenLocation screenLocation) {
+        DMISpriteSystem.SetSpriteScreenLocation(new(movable.Entity, movable.SpriteComponent), screenLocation);
+    }
+
+    public void SetSpriteAppearance(Entity<DMISpriteComponent> 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<IconAppearance> animate) {
         if (atom is not DreamObjectMovable movable)
             return; //Animating non-movables is unimplemented TODO: should handle images and maybe filters
@@ -497,11 +512,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) {
@@ -591,6 +606,7 @@ public sealed class AtomManager {
         }
 
         _definitionAppearanceCache.Add(def, appearance);
+        AppearanceSystem?.IncreaseAppearanceRefCount(appearance);
         return appearance;
     }
 

OpenDreamRuntime/DreamManager.cs

@@ -224,6 +224,7 @@ public sealed partial class DreamManager {
                 refType = RefType.DreamAppearance;
                 _appearanceSystem ??= _entitySystemManager.GetEntitySystem<ServerAppearanceSystem>();
                 idx = (int)_appearanceSystem.AddAppearance(appearance);
+                _appearanceSystem.IncreaseAppearanceRefCount(appearance);
             } else if (value.TryGetValueAsDreamResource(out var refRsc)) {
                 refType = RefType.DreamResource;
                 idx = refRsc.Id;

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,33 +43,45 @@ 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)
                 Logger.GetSawmill("opendream.image")
                     .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) {
@@ -111,7 +124,7 @@ public sealed class DreamObjectImage : DreamObject {
                 Appearance = newAppearance;
                 if(_entity != EntityUid.Invalid) {
                     DMISpriteComponent sprite = EntityManager.GetComponent<DMISpriteComponent>(_entity);
-                    sprite.SetAppearance(Appearance!);
+                    AtomManager.SetSpriteAppearance(new(_entity, sprite), Appearance);
                 }
                 break;
             case "loc":
@@ -206,10 +219,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<DMISpriteComponent>(_entity);
-                        sprite.SetAppearance(Appearance!);
+                        AtomManager.SetSpriteAppearance(new(_entity, sprite), Appearance!);
                     }
                     break;
                 }
@@ -231,12 +246,23 @@ public sealed class DreamObjectImage : DreamObject {
         if(_entity == EntityUid.Invalid) {
             _entity = EntityManager.SpawnEntity(null, new MapCoordinates(0, 0, MapId.Nullspace));
             DMISpriteComponent sprite = EntityManager.AddComponent<DMISpriteComponent>(_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);
         }

OpenDreamRuntime/Objects/Types/DreamObjectMovable.cs

@@ -22,15 +22,7 @@ public class DreamObjectMovable : DreamObjectAtom {
 
     private string? ScreenLoc {
         get => _screenLoc;
-        set {
-            _screenLoc = value;
-            if (!EntityManager.TryGetComponent<DMISpriteComponent>(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));
+    }
 }

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;
+    }
 }

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;
 }
+

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<DMISpriteComponent, ComponentGetState>(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<DMISpriteComponent> 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<DMISpriteComponent> ent, ScreenLocation screenLocation) {
+        DMISpriteComponent component = ent.Comp;
+        component.ScreenLocation = screenLocation;
+        Dirty(ent, component);
+    }
 }