Appearance Weakrefs

OpenDreamClient/Rendering/ClientAppearanceSystem.cs

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

OpenDreamRuntime/AtomManager.cs

@@ -36,6 +36,8 @@ public sealed class AtomManager {
     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 +193,7 @@ public EntityUid CreateMovableEntity(DreamObjectMovable movable) {
         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;
@@ -473,7 +475,6 @@ public bool TryGetAppearance(DreamObject atom, [NotNullWhen(true)] out IconAppea
     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 +483,20 @@ public void SetAtomAppearance(DreamObject atom, IconAppearance appearance) {
         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,7 +506,7 @@ public void AnimateAppearance(DreamObject atom, TimeSpan duration, AnimationEasi
         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);
 
@@ -591,6 +600,7 @@ public IconAppearance GetAppearanceFromDefinition(DreamObjectDefinition def) {
         }
 
         _definitionAppearanceCache.Add(def, appearance);
+        AppearanceSystem.IncreaseAppearanceRefCount(appearance);
         return appearance;
     }
 

OpenDreamRuntime/DreamManager.cs

@@ -224,6 +224,7 @@ public string CreateRef(DreamValue value) {
                 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,43 @@
         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 +122,7 @@
                 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 +217,12 @@
             }
             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 +244,23 @@
         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 @@ private void SetLoc(DreamObjectAtom? loc) {
                 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 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,16 @@
                 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,14 @@
 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;
-
-        [ViewVariables] public IconAppearance? Appearance { get; private set; }
-
-        public void SetAppearance(IconAppearance? appearance, bool dirty = true) {
-            Appearance = appearance;
-
-            if (dirty) {
-                Dirty();
-            }
-        }
-    }
+namespace OpenDreamRuntime.Rendering;
+[RegisterComponent]
+public sealed partial class DMISpriteComponent : SharedDMISpriteComponent {
+    [ViewVariables]
+    [Access(typeof(DMISpriteSystem))]
+    public ScreenLocation ScreenLocation;
+
+    [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);
+        _appearance = _entitySystemManager.GetEntitySystem<ServerAppearanceSystem>();
     }
 
     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);
+    }
 }