From 1240edb8f0caba38ae9cbe57f0b80c772907c31c Mon Sep 17 00:00:00 2001 From: amy Date: Thu, 7 Sep 2023 22:39:22 +0100 Subject: [PATCH 01/13] we buffer the edits --- OpenDreamRuntime/AtomManager.cs | 53 ++++++++++++++++++++-- OpenDreamRuntime/Procs/DMOpcodeHandlers.cs | 19 +++----- OpenDreamRuntime/Procs/DreamEnumerators.cs | 53 ++++++++++++++++++++++ 3 files changed, 107 insertions(+), 18 deletions(-) diff --git a/OpenDreamRuntime/AtomManager.cs b/OpenDreamRuntime/AtomManager.cs index adcc0bb55b..2b001888c5 100644 --- a/OpenDreamRuntime/AtomManager.cs +++ b/OpenDreamRuntime/AtomManager.cs @@ -10,12 +10,55 @@ using Dependency = Robust.Shared.IoC.DependencyAttribute; namespace OpenDreamRuntime { + + /// + /// A list that can be buffered to prevent changes from being applied until FinishBuffering is called. + /// + /// + public sealed class BufferedList : List { + private bool _isBuffering = false; + private List _bufferedAdds = new(); + private List _bufferedRemoves = new(); + public new void Add(T Value) { + if (!_isBuffering) { + base.Add(Value); + } + _bufferedRemoves.Remove(Value); + _bufferedAdds.Add(Value); + } + + public new void Remove(T Value) { + if (!_isBuffering) { + base.Remove(Value); + } + _bufferedAdds.Remove(Value); + _bufferedRemoves.Remove(Value); + } + + public void StartBuffering() { + _isBuffering = true; + } + + public void FinishBuffering() { + _isBuffering = false; + foreach (var item in _bufferedAdds) + { + base.Add(item); + } + foreach (var item in _bufferedRemoves) + { + base.Remove(item); + } + _bufferedAdds.Clear(); + _bufferedRemoves.Clear(); + } + } public sealed class AtomManager { - public List Areas { get; } = new(); - public List Turfs { get; } = new(); - public List Movables { get; } = new(); - public List Objects { get; } = new(); - public List Mobs { get; } = new(); + public BufferedList Areas { get; } = new(); + public BufferedList Turfs { get; } = new(); + public BufferedList Movables { get; } = new(); + public BufferedList Objects { get; } = new(); + public BufferedList Mobs { get; } = new(); public int AtomCount => Areas.Count + Turfs.Count + Movables.Count + Objects.Count + Mobs.Count; [Dependency] private readonly IEntityManager _entityManager = default!; diff --git a/OpenDreamRuntime/Procs/DMOpcodeHandlers.cs b/OpenDreamRuntime/Procs/DMOpcodeHandlers.cs index f9b8cab3ca..e1b378d2f7 100644 --- a/OpenDreamRuntime/Procs/DMOpcodeHandlers.cs +++ b/OpenDreamRuntime/Procs/DMOpcodeHandlers.cs @@ -85,17 +85,6 @@ private static IDreamValueEnumerator GetContentsEnumerator(DreamObjectTree objec if (dreamObject is DreamObjectAtom) { list = dreamObject.GetVariable("contents").MustGetValueAsDreamList(); } else if (dreamObject is DreamObjectWorld) { - // Use a different enumerator for /area and /turf that only enumerates those rather than all atoms - if (filterType?.ObjectDefinition.IsSubtypeOf(objectTree.Area) == true) { - return new DreamObjectEnumerator(atomManager.Areas, filterType); - } else if (filterType?.ObjectDefinition.IsSubtypeOf(objectTree.Turf) == true) { - return new DreamObjectEnumerator(atomManager.Turfs, filterType); - } else if (filterType?.ObjectDefinition.IsSubtypeOf(objectTree.Obj) == true) { - return new DreamObjectEnumerator(atomManager.Objects, filterType); - } else if (filterType?.ObjectDefinition.IsSubtypeOf(objectTree.Mob) == true) { - return new DreamObjectEnumerator(atomManager.Mobs, filterType); - } - return new WorldContentsEnumerator(atomManager, filterType); } } @@ -238,8 +227,10 @@ public static ProcStatus Enumerate(DMProcState state) { DreamReference outputRef = state.ReadReference(); int jumpToIfFailure = state.ReadInt(); - if (!enumerator.Enumerate(state, outputRef)) + if (!enumerator.Enumerate(state, outputRef)) { + enumerator.EndEnumeration(); state.Jump(jumpToIfFailure); + } return ProcStatus.Continue; } @@ -248,8 +239,10 @@ public static ProcStatus EnumerateNoAssign(DMProcState state) { IDreamValueEnumerator enumerator = state.EnumeratorStack.Peek(); int jumpToIfFailure = state.ReadInt(); - if (!enumerator.Enumerate(state, null)) + if (!enumerator.Enumerate(state, null)) { + enumerator.EndEnumeration(); state.Jump(jumpToIfFailure); + } return ProcStatus.Continue; } diff --git a/OpenDreamRuntime/Procs/DreamEnumerators.cs b/OpenDreamRuntime/Procs/DreamEnumerators.cs index 38b424e640..1fe144efb9 100644 --- a/OpenDreamRuntime/Procs/DreamEnumerators.cs +++ b/OpenDreamRuntime/Procs/DreamEnumerators.cs @@ -1,9 +1,11 @@ using OpenDreamRuntime.Objects; +using OpenDreamRuntime.Objects.Types; using OpenDreamShared.Dream.Procs; namespace OpenDreamRuntime.Procs { public interface IDreamValueEnumerator { public bool Enumerate(DMProcState state, DreamReference? reference); + public void EndEnumeration(); } /// @@ -30,6 +32,9 @@ public bool Enumerate(DMProcState state, DreamReference? reference) { return successful; } + + void IDreamValueEnumerator.EndEnumeration() { + } } /// @@ -57,6 +62,9 @@ public bool Enumerate(DMProcState state, DreamReference? reference) { state.AssignReference(reference.Value, new DreamValue(_dreamObjectEnumerator.Current)); return success; } + + void IDreamValueEnumerator.EndEnumeration() { + } } /// @@ -79,6 +87,9 @@ public bool Enumerate(DMProcState state, DreamReference? reference) { state.AssignReference(reference.Value, success ? _dreamValueArray[_current] : DreamValue.Null); // Assign regardless of success return success; } + + void IDreamValueEnumerator.EndEnumeration() { + } } /// @@ -112,6 +123,9 @@ public bool Enumerate(DMProcState state, DreamReference? reference) { } } while (true); } + + void IDreamValueEnumerator.EndEnumeration() { + } } /// @@ -126,6 +140,24 @@ sealed class WorldContentsEnumerator : IDreamValueEnumerator { public WorldContentsEnumerator(AtomManager atomManager, TreeEntry? filterType) { _atomManager = atomManager; _filterType = filterType; + if(filterType is not null) + if(filterType.ObjectDefinition.IsSubtypeOf(filterType.ObjectDefinition.ObjectTree.Area)) + atomManager.Areas.StartBuffering(); + else if(filterType.ObjectDefinition.IsSubtypeOf(filterType.ObjectDefinition.ObjectTree.Mob)) + atomManager.Mobs.StartBuffering(); + else if(filterType.ObjectDefinition.IsSubtypeOf(filterType.ObjectDefinition.ObjectTree.Obj)) + atomManager.Objects.StartBuffering(); + else if(filterType.ObjectDefinition.IsSubtypeOf(filterType.ObjectDefinition.ObjectTree.Turf)) + atomManager.Turfs.StartBuffering(); + else if(filterType.ObjectDefinition.IsSubtypeOf(filterType.ObjectDefinition.ObjectTree.Movable)) + atomManager.Movables.StartBuffering(); + else { + atomManager.Areas.StartBuffering(); + atomManager.Mobs.StartBuffering(); + atomManager.Objects.StartBuffering(); + atomManager.Turfs.StartBuffering(); + atomManager.Movables.StartBuffering(); + } } public bool Enumerate(DMProcState state, DreamReference? reference) { @@ -145,5 +177,26 @@ public bool Enumerate(DMProcState state, DreamReference? reference) { } } while (true); } + + void IDreamValueEnumerator.EndEnumeration() { + if(_filterType is not null) + if(_filterType.ObjectDefinition.IsSubtypeOf(_filterType.ObjectDefinition.ObjectTree.Area)) + _atomManager.Areas.FinishBuffering(); + else if(_filterType.ObjectDefinition.IsSubtypeOf(_filterType.ObjectDefinition.ObjectTree.Mob)) + _atomManager.Mobs.FinishBuffering(); + else if(_filterType.ObjectDefinition.IsSubtypeOf(_filterType.ObjectDefinition.ObjectTree.Obj)) + _atomManager.Objects.FinishBuffering(); + else if(_filterType.ObjectDefinition.IsSubtypeOf(_filterType.ObjectDefinition.ObjectTree.Turf)) + _atomManager.Turfs.FinishBuffering(); + else if(_filterType.ObjectDefinition.IsSubtypeOf(_filterType.ObjectDefinition.ObjectTree.Movable)) + _atomManager.Movables.FinishBuffering(); + else { + _atomManager.Areas.FinishBuffering(); + _atomManager.Mobs.FinishBuffering(); + _atomManager.Objects.FinishBuffering(); + _atomManager.Turfs.FinishBuffering(); + _atomManager.Movables.FinishBuffering(); + } + } } } From 4961fa1765faa611646af04f87608578c24ad277 Mon Sep 17 00:00:00 2001 From: amy Date: Thu, 7 Sep 2023 23:11:32 +0100 Subject: [PATCH 02/13] add broken test --- .../DMProject/Tests/List/worldcontents.dm | 29 +++++++++++++++++++ OpenDreamRuntime/AtomManager.cs | 4 +-- 2 files changed, 31 insertions(+), 2 deletions(-) create mode 100644 Content.Tests/DMProject/Tests/List/worldcontents.dm diff --git a/Content.Tests/DMProject/Tests/List/worldcontents.dm b/Content.Tests/DMProject/Tests/List/worldcontents.dm new file mode 100644 index 0000000000..f8021c124f --- /dev/null +++ b/Content.Tests/DMProject/Tests/List/worldcontents.dm @@ -0,0 +1,29 @@ +/obj/one + name = "one" +/obj/two + name = "two" + + +/proc/RunTest() + var/obj/onehandle = new /obj/one(locate(1,1,1)) + ASSERT(length(world.contents) == 1) + var/count = 0 + for(var/obj/O in world) + count++ + ASSERT(count == 1) + + for(var/obj/O in world) + count++ + new /obj/two(locate(1,1,1)) + ASSERT(length(world.contents) == 1) //while iterating over world.contents, adding a new object should be buffered still + ASSERT(count == 1) + ASSERT(length(world.contents) == 2) //it should no longer be buffered + + count = 0 + var/worldcontlen = length(world.contents) + for(var/obj/O in world) + count++ + world.contents -= onehandle + ASSERT(worldcontlen == length(world.contents)) //removing an object should be buffered + ASSERT(worldcontlen == length(world.contents)+1) //it should no longer be buffered + diff --git a/OpenDreamRuntime/AtomManager.cs b/OpenDreamRuntime/AtomManager.cs index 2b001888c5..228018c9de 100644 --- a/OpenDreamRuntime/AtomManager.cs +++ b/OpenDreamRuntime/AtomManager.cs @@ -23,8 +23,8 @@ public sealed class BufferedList : List { if (!_isBuffering) { base.Add(Value); } - _bufferedRemoves.Remove(Value); - _bufferedAdds.Add(Value); + if(!_bufferedRemoves.Remove(Value)) //if we don't already have a remove queued, add it, otherwise we're just undoing a remove + _bufferedAdds.Add(Value); } public new void Remove(T Value) { From 35457c27d6b90a419067618c35802d55bff4d2b5 Mon Sep 17 00:00:00 2001 From: amy Date: Thu, 7 Sep 2023 23:52:24 +0100 Subject: [PATCH 03/13] more verbose test --- .../DMProject/Tests/List/worldcontents.dm | 22 ++++++++++++------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/Content.Tests/DMProject/Tests/List/worldcontents.dm b/Content.Tests/DMProject/Tests/List/worldcontents.dm index f8021c124f..4ff4fb8bf3 100644 --- a/Content.Tests/DMProject/Tests/List/worldcontents.dm +++ b/Content.Tests/DMProject/Tests/List/worldcontents.dm @@ -3,27 +3,33 @@ /obj/two name = "two" - /proc/RunTest() var/obj/onehandle = new /obj/one(locate(1,1,1)) - ASSERT(length(world.contents) == 1) + onehandle.name = "one \ref[onehandle]" + ASSERT(length(world.contents) == 1 && "sanity check") var/count = 0 for(var/obj/O in world) count++ - ASSERT(count == 1) + for(var/obj/O in world) + count++ + for(var/obj/O in world) + count++ + CRASH(json_encode(world.contents)) + ASSERT(count == 1 && "more than one object in world") + ASSERT(length(world.contents) == 1 && "iterating over world editted world.contents") for(var/obj/O in world) count++ new /obj/two(locate(1,1,1)) - ASSERT(length(world.contents) == 1) //while iterating over world.contents, adding a new object should be buffered still - ASSERT(count == 1) - ASSERT(length(world.contents) == 2) //it should no longer be buffered + ASSERT(length(world.contents) == 1 && "length changed during buffered add") //while iterating over world.contents, adding a new object should be buffered still + ASSERT(count == 1 && "more than one object in world") + ASSERT(length(world.contents) == 2 && "length didn't change after buffered add") //it should no longer be buffered count = 0 var/worldcontlen = length(world.contents) for(var/obj/O in world) count++ world.contents -= onehandle - ASSERT(worldcontlen == length(world.contents)) //removing an object should be buffered - ASSERT(worldcontlen == length(world.contents)+1) //it should no longer be buffered + ASSERT(worldcontlen == length(world.contents) && "length changed during buffered remove") //removing an object should be buffered + ASSERT(worldcontlen == length(world.contents)+1 && "length didn't change after buffered remove") //it should no longer be buffered From 86a7a8dba0011531949b74420dc2138e78775066 Mon Sep 17 00:00:00 2001 From: amylizzle Date: Fri, 8 Sep 2023 15:33:49 +0100 Subject: [PATCH 04/13] Fixed it! --- OpenDreamRuntime/AtomManager.cs | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/OpenDreamRuntime/AtomManager.cs b/OpenDreamRuntime/AtomManager.cs index 228018c9de..6b322a8d68 100644 --- a/OpenDreamRuntime/AtomManager.cs +++ b/OpenDreamRuntime/AtomManager.cs @@ -22,19 +22,32 @@ public sealed class BufferedList : List { public new void Add(T Value) { if (!_isBuffering) { base.Add(Value); + return; } if(!_bufferedRemoves.Remove(Value)) //if we don't already have a remove queued, add it, otherwise we're just undoing a remove _bufferedAdds.Add(Value); } + public new void AddRange(IEnumerable collection) { + if (!_isBuffering) { + base.AddRange(collection); + return; + } + foreach (var item in collection) + { + if(!_bufferedRemoves.Remove(item)) //if we don't already have a remove queued, add it, otherwise we're just undoing a remove + _bufferedAdds.Add(item); + } + } + public new void Remove(T Value) { if (!_isBuffering) { base.Remove(Value); + return; } _bufferedAdds.Remove(Value); _bufferedRemoves.Remove(Value); } - public void StartBuffering() { _isBuffering = true; } From d028069b9a114290a8aa9c334609434c400861f8 Mon Sep 17 00:00:00 2001 From: amylizzle Date: Fri, 8 Sep 2023 16:09:47 +0100 Subject: [PATCH 05/13] mark /world.contents as const --- DMCompiler/DMStandard/Types/World.dm | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/DMCompiler/DMStandard/Types/World.dm b/DMCompiler/DMStandard/Types/World.dm index b5242483ab..7d59b85272 100644 --- a/DMCompiler/DMStandard/Types/World.dm +++ b/DMCompiler/DMStandard/Types/World.dm @@ -1,6 +1,6 @@ /world - var/list/contents = null - var/list/vars + var/const/list/contents = null + var/const/list/vars var/log = null From 44337c1cbb0c5dd226704e91190c16b6498ce481 Mon Sep 17 00:00:00 2001 From: amylizzle Date: Fri, 8 Sep 2023 16:10:27 +0100 Subject: [PATCH 06/13] clean up test --- Content.Tests/DMProject/Tests/List/worldcontents.dm | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/Content.Tests/DMProject/Tests/List/worldcontents.dm b/Content.Tests/DMProject/Tests/List/worldcontents.dm index 4ff4fb8bf3..579785ecc0 100644 --- a/Content.Tests/DMProject/Tests/List/worldcontents.dm +++ b/Content.Tests/DMProject/Tests/List/worldcontents.dm @@ -10,26 +10,26 @@ var/count = 0 for(var/obj/O in world) count++ + count = 0 for(var/obj/O in world) count++ - for(var/obj/O in world) - count++ - CRASH(json_encode(world.contents)) + ASSERT(count == 1 && "more than one object in world") ASSERT(length(world.contents) == 1 && "iterating over world editted world.contents") + count = 0 for(var/obj/O in world) count++ new /obj/two(locate(1,1,1)) ASSERT(length(world.contents) == 1 && "length changed during buffered add") //while iterating over world.contents, adding a new object should be buffered still - ASSERT(count == 1 && "more than one object in world") + ASSERT(count == 1 && "more than one object in world during add") ASSERT(length(world.contents) == 2 && "length didn't change after buffered add") //it should no longer be buffered count = 0 var/worldcontlen = length(world.contents) for(var/obj/O in world) count++ - world.contents -= onehandle + del(onehandle) ASSERT(worldcontlen == length(world.contents) && "length changed during buffered remove") //removing an object should be buffered ASSERT(worldcontlen == length(world.contents)+1 && "length didn't change after buffered remove") //it should no longer be buffered From 1e64388a32695e5ea6be62bc7c8870bcd5c9d875 Mon Sep 17 00:00:00 2001 From: amylizzle Date: Fri, 8 Sep 2023 16:31:30 +0100 Subject: [PATCH 07/13] got it --- OpenDreamRuntime/AtomManager.cs | 22 ++++++++++++++++--- .../Objects/Types/DreamObjectMovable.cs | 6 ++--- 2 files changed, 22 insertions(+), 6 deletions(-) diff --git a/OpenDreamRuntime/AtomManager.cs b/OpenDreamRuntime/AtomManager.cs index 6b322a8d68..f67db6fd52 100644 --- a/OpenDreamRuntime/AtomManager.cs +++ b/OpenDreamRuntime/AtomManager.cs @@ -40,13 +40,29 @@ public sealed class BufferedList : List { } } + /// + /// Removes the first instance of the given value, replacing it with the last value in the list. This is done to avoid shifting the entire list. + /// + /// + /// + private bool RemoveSwapLast(T Value) { + int index = IndexOf(Value); + if (index == -1) + return false; + var old = this[index]; + var replacement = this[this.Count - 1]; + this[index] = replacement; + base.RemoveAt(this.Count - 1); + return true; + } + public new void Remove(T Value) { if (!_isBuffering) { - base.Remove(Value); + RemoveSwapLast(Value); return; } _bufferedAdds.Remove(Value); - _bufferedRemoves.Remove(Value); + _bufferedRemoves.Add(Value); } public void StartBuffering() { _isBuffering = true; @@ -60,7 +76,7 @@ public void FinishBuffering() { } foreach (var item in _bufferedRemoves) { - base.Remove(item); + RemoveSwapLast(item); } _bufferedAdds.Clear(); _bufferedRemoves.Clear(); diff --git a/OpenDreamRuntime/Objects/Types/DreamObjectMovable.cs b/OpenDreamRuntime/Objects/Types/DreamObjectMovable.cs index af1ff3dd33..3173400fb1 100644 --- a/OpenDreamRuntime/Objects/Types/DreamObjectMovable.cs +++ b/OpenDreamRuntime/Objects/Types/DreamObjectMovable.cs @@ -63,11 +63,11 @@ public override void Initialize(DreamProcArguments args) { protected override void HandleDeletion() { if (IsSubtypeOf(ObjectTree.Obj)) - AtomManager.Objects.RemoveSwap(AtomManager.Objects.IndexOf(this)); + AtomManager.Objects.Remove(this); else if (IsSubtypeOf(ObjectTree.Mob)) - AtomManager.Mobs.RemoveSwap(AtomManager.Mobs.IndexOf((DreamObjectMob)this)); + AtomManager.Mobs.Remove((DreamObjectMob)this); else - AtomManager.Movables.RemoveSwap(AtomManager.Movables.IndexOf(this)); + AtomManager.Movables.Remove(this); AtomManager.DeleteMovableEntity(this); base.HandleDeletion(); From b35876eda06ee59ced7807e9c0c2095314ef218c Mon Sep 17 00:00:00 2001 From: amylizzle Date: Fri, 8 Sep 2023 16:40:16 +0100 Subject: [PATCH 08/13] handle different numbers of world contents --- Content.Tests/DMProject/Tests/List/worldcontents.dm | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/Content.Tests/DMProject/Tests/List/worldcontents.dm b/Content.Tests/DMProject/Tests/List/worldcontents.dm index 579785ecc0..f01e9c9c40 100644 --- a/Content.Tests/DMProject/Tests/List/worldcontents.dm +++ b/Content.Tests/DMProject/Tests/List/worldcontents.dm @@ -4,9 +4,11 @@ name = "two" /proc/RunTest() + var/start_world_count = length(world.contents) var/obj/onehandle = new /obj/one(locate(1,1,1)) onehandle.name = "one \ref[onehandle]" - ASSERT(length(world.contents) == 1 && "sanity check") + + ASSERT(length(world.contents) == start_world_count + 1 && "sanity check") var/count = 0 for(var/obj/O in world) count++ @@ -14,16 +16,16 @@ for(var/obj/O in world) count++ - ASSERT(count == 1 && "more than one object in world") - ASSERT(length(world.contents) == 1 && "iterating over world editted world.contents") + ASSERT(count == start_world_count + 1 && "more than one object in world") + ASSERT(length(world.contents) == start_world_count + 1 && "iterating over world editted world.contents") count = 0 for(var/obj/O in world) count++ new /obj/two(locate(1,1,1)) - ASSERT(length(world.contents) == 1 && "length changed during buffered add") //while iterating over world.contents, adding a new object should be buffered still + ASSERT(length(world.contents) == start_world_count + 1 && "length changed during buffered add") //while iterating over world.contents, adding a new object should be buffered still ASSERT(count == 1 && "more than one object in world during add") - ASSERT(length(world.contents) == 2 && "length didn't change after buffered add") //it should no longer be buffered + ASSERT(length(world.contents) == start_world_count + 2 && "length didn't change after buffered add") //it should no longer be buffered count = 0 var/worldcontlen = length(world.contents) From 9b7c8734bb19f81eba7bb29986dedbeb75c00ebe Mon Sep 17 00:00:00 2001 From: amylizzle Date: Fri, 8 Sep 2023 17:11:00 +0100 Subject: [PATCH 09/13] make test more robust, don't return deleted atoms --- .../DMProject/Tests/List/worldcontents.dm | 22 +++++++++++-------- OpenDreamRuntime/Procs/DreamEnumerators.cs | 2 +- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/Content.Tests/DMProject/Tests/List/worldcontents.dm b/Content.Tests/DMProject/Tests/List/worldcontents.dm index f01e9c9c40..fef11deb97 100644 --- a/Content.Tests/DMProject/Tests/List/worldcontents.dm +++ b/Content.Tests/DMProject/Tests/List/worldcontents.dm @@ -4,11 +4,13 @@ name = "two" /proc/RunTest() + new /obj/one(locate(1,1,1)) + new /obj/one(locate(1,1,1)) var/start_world_count = length(world.contents) var/obj/onehandle = new /obj/one(locate(1,1,1)) onehandle.name = "one \ref[onehandle]" - ASSERT(length(world.contents) == start_world_count + 1 && "sanity check") + ASSERT(length(world.contents) == (start_world_count + 1) && "sanity check") var/count = 0 for(var/obj/O in world) count++ @@ -16,22 +18,24 @@ for(var/obj/O in world) count++ - ASSERT(count == start_world_count + 1 && "more than one object in world") - ASSERT(length(world.contents) == start_world_count + 1 && "iterating over world editted world.contents") + ASSERT(count == (start_world_count + 1) && "more than one object in world") + ASSERT(length(world.contents) == (start_world_count + 1) && "iterating over world editted world.contents") count = 0 for(var/obj/O in world) + if(count == 0) + new /obj/two(locate(1,1,1)) count++ - new /obj/two(locate(1,1,1)) - ASSERT(length(world.contents) == start_world_count + 1 && "length changed during buffered add") //while iterating over world.contents, adding a new object should be buffered still - ASSERT(count == 1 && "more than one object in world during add") - ASSERT(length(world.contents) == start_world_count + 2 && "length didn't change after buffered add") //it should no longer be buffered + ASSERT(length(world.contents) == (start_world_count + 1) && "length changed during buffered add") //while iterating over world.contents, adding a new object should be buffered still + ASSERT(count == (start_world_count + 1) && "more than one object in world during add") + ASSERT(length(world.contents) == (start_world_count + 2) && "length didn't change after buffered add") //it should no longer be buffered count = 0 var/worldcontlen = length(world.contents) for(var/obj/O in world) - count++ - del(onehandle) + if(!isnull(onehandle)) + del(onehandle) + count++ ASSERT(worldcontlen == length(world.contents) && "length changed during buffered remove") //removing an object should be buffered ASSERT(worldcontlen == length(world.contents)+1 && "length didn't change after buffered remove") //it should no longer be buffered diff --git a/OpenDreamRuntime/Procs/DreamEnumerators.cs b/OpenDreamRuntime/Procs/DreamEnumerators.cs index 1fe144efb9..c936d0d3af 100644 --- a/OpenDreamRuntime/Procs/DreamEnumerators.cs +++ b/OpenDreamRuntime/Procs/DreamEnumerators.cs @@ -170,7 +170,7 @@ public bool Enumerate(DMProcState state, DreamReference? reference) { } DreamObject atom = _atomManager.GetAtom(_current); - if (_filterType == null || atom.IsSubtypeOf(_filterType)) { + if (!atom.Deleted && (_filterType == null || atom.IsSubtypeOf(_filterType))) { if (reference != null) state.AssignReference(reference.Value, new DreamValue(atom)); return true; From 4f42534bae0b46b013b44c3b9320669c64bb97a4 Mon Sep 17 00:00:00 2001 From: amylizzle Date: Fri, 8 Sep 2023 17:18:35 +0100 Subject: [PATCH 10/13] dumb --- Content.Tests/DMProject/Tests/List/worldcontents.dm | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Content.Tests/DMProject/Tests/List/worldcontents.dm b/Content.Tests/DMProject/Tests/List/worldcontents.dm index fef11deb97..0b19892e6b 100644 --- a/Content.Tests/DMProject/Tests/List/worldcontents.dm +++ b/Content.Tests/DMProject/Tests/List/worldcontents.dm @@ -6,7 +6,9 @@ /proc/RunTest() new /obj/one(locate(1,1,1)) new /obj/one(locate(1,1,1)) - var/start_world_count = length(world.contents) + var/start_world_count = 0 + for(var/obj/O in world) + start_world_count++ var/obj/onehandle = new /obj/one(locate(1,1,1)) onehandle.name = "one \ref[onehandle]" From 7887b15d7130f1783ebdfa273a4e9fb398b60bf9 Mon Sep 17 00:00:00 2001 From: amylizzle Date: Fri, 8 Sep 2023 17:36:40 +0100 Subject: [PATCH 11/13] is this it? --- Content.Tests/DMProject/Tests/List/worldcontents.dm | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/Content.Tests/DMProject/Tests/List/worldcontents.dm b/Content.Tests/DMProject/Tests/List/worldcontents.dm index 0b19892e6b..8f4b1f364f 100644 --- a/Content.Tests/DMProject/Tests/List/worldcontents.dm +++ b/Content.Tests/DMProject/Tests/List/worldcontents.dm @@ -6,12 +6,15 @@ /proc/RunTest() new /obj/one(locate(1,1,1)) new /obj/one(locate(1,1,1)) - var/start_world_count = 0 + + var/start_world_count = length(world.contents) + var/start_world_object_count = 0 for(var/obj/O in world) - start_world_count++ + start_world_object_count++ + var/obj/onehandle = new /obj/one(locate(1,1,1)) onehandle.name = "one \ref[onehandle]" - + ASSERT(length(world.contents) == (start_world_count + 1) && "sanity check") var/count = 0 for(var/obj/O in world) @@ -20,7 +23,7 @@ for(var/obj/O in world) count++ - ASSERT(count == (start_world_count + 1) && "more than one object in world") + ASSERT(count == (start_world_object_count + 1) && "more than one object in world") ASSERT(length(world.contents) == (start_world_count + 1) && "iterating over world editted world.contents") count = 0 @@ -29,7 +32,7 @@ new /obj/two(locate(1,1,1)) count++ ASSERT(length(world.contents) == (start_world_count + 1) && "length changed during buffered add") //while iterating over world.contents, adding a new object should be buffered still - ASSERT(count == (start_world_count + 1) && "more than one object in world during add") + ASSERT(count == (start_world_object_count + 1) && "more than one object in world during add") ASSERT(length(world.contents) == (start_world_count + 2) && "length didn't change after buffered add") //it should no longer be buffered count = 0 From c8f5c866ec20df11901b96ce26d8223c2edeb89c Mon Sep 17 00:00:00 2001 From: amy Date: Sun, 15 Oct 2023 22:38:11 +0100 Subject: [PATCH 12/13] break test for nested loops --- Content.Tests/DMProject/Tests/List/worldcontents.dm | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/Content.Tests/DMProject/Tests/List/worldcontents.dm b/Content.Tests/DMProject/Tests/List/worldcontents.dm index 8f4b1f364f..4471cccd1f 100644 --- a/Content.Tests/DMProject/Tests/List/worldcontents.dm +++ b/Content.Tests/DMProject/Tests/List/worldcontents.dm @@ -44,3 +44,16 @@ ASSERT(worldcontlen == length(world.contents) && "length changed during buffered remove") //removing an object should be buffered ASSERT(worldcontlen == length(world.contents)+1 && "length didn't change after buffered remove") //it should no longer be buffered + // nested loop over world + count = 0 + worldcontlen = length(world.contents) + for(var/obj/O in world) + for(var/obj/O2 in world) + if(count == 0) + new /obj/two(locate(1,1,1)) + ASSERT(length(world.contents) == worldcontlen && "nested: length changed during buffered add") //while iterating over world.contents, adding a new object should be buffered still + if(count >= worldcontlen) //on the second loop, the new object should be in the inner list + ASSERT(length(world.contents) == (worldcontlen+1) && "nested:length didn't change after buffered add") //while iterating over world.contents, adding a new object should be buffered still + count++ + ASSERT(length(world.contents) == worldcontlen && "nested: length changed during buffered add") //it should be buffered in the outer loop still + \ No newline at end of file From c796a1ed2cc196d54d922604e28066ef0b777836 Mon Sep 17 00:00:00 2001 From: amy Date: Thu, 19 Oct 2023 22:12:48 +0100 Subject: [PATCH 13/13] try for byond-like behaviour --- .../DMProject/Tests/List/worldcontents.dm | 72 +++------------ .../DMProject/Tests/List/worldcontents2.dm | 15 +++ .../DMProject/Tests/List/worldcontents3.dm | 12 +++ OpenDreamRuntime/AtomManager.cs | 66 ++++---------- OpenDreamRuntime/Procs/DreamEnumerators.cs | 91 ++++++++++++++----- 5 files changed, 125 insertions(+), 131 deletions(-) create mode 100644 Content.Tests/DMProject/Tests/List/worldcontents2.dm create mode 100644 Content.Tests/DMProject/Tests/List/worldcontents3.dm diff --git a/Content.Tests/DMProject/Tests/List/worldcontents.dm b/Content.Tests/DMProject/Tests/List/worldcontents.dm index 4471cccd1f..a60b3d1e76 100644 --- a/Content.Tests/DMProject/Tests/List/worldcontents.dm +++ b/Content.Tests/DMProject/Tests/List/worldcontents.dm @@ -1,59 +1,15 @@ -/obj/one - name = "one" -/obj/two - name = "two" - /proc/RunTest() - new /obj/one(locate(1,1,1)) - new /obj/one(locate(1,1,1)) - - var/start_world_count = length(world.contents) - var/start_world_object_count = 0 - for(var/obj/O in world) - start_world_object_count++ - - var/obj/onehandle = new /obj/one(locate(1,1,1)) - onehandle.name = "one \ref[onehandle]" - - ASSERT(length(world.contents) == (start_world_count + 1) && "sanity check") - var/count = 0 - for(var/obj/O in world) - count++ - count = 0 - for(var/obj/O in world) - count++ - - ASSERT(count == (start_world_object_count + 1) && "more than one object in world") - ASSERT(length(world.contents) == (start_world_count + 1) && "iterating over world editted world.contents") - - count = 0 - for(var/obj/O in world) - if(count == 0) - new /obj/two(locate(1,1,1)) - count++ - ASSERT(length(world.contents) == (start_world_count + 1) && "length changed during buffered add") //while iterating over world.contents, adding a new object should be buffered still - ASSERT(count == (start_world_object_count + 1) && "more than one object in world during add") - ASSERT(length(world.contents) == (start_world_count + 2) && "length didn't change after buffered add") //it should no longer be buffered - - count = 0 - var/worldcontlen = length(world.contents) - for(var/obj/O in world) - if(!isnull(onehandle)) - del(onehandle) - count++ - ASSERT(worldcontlen == length(world.contents) && "length changed during buffered remove") //removing an object should be buffered - ASSERT(worldcontlen == length(world.contents)+1 && "length didn't change after buffered remove") //it should no longer be buffered - - // nested loop over world - count = 0 - worldcontlen = length(world.contents) - for(var/obj/O in world) - for(var/obj/O2 in world) - if(count == 0) - new /obj/two(locate(1,1,1)) - ASSERT(length(world.contents) == worldcontlen && "nested: length changed during buffered add") //while iterating over world.contents, adding a new object should be buffered still - if(count >= worldcontlen) //on the second loop, the new object should be in the inner list - ASSERT(length(world.contents) == (worldcontlen+1) && "nested:length didn't change after buffered add") //while iterating over world.contents, adding a new object should be buffered still - count++ - ASSERT(length(world.contents) == worldcontlen && "nested: length changed during buffered add") //it should be buffered in the outer loop still - \ No newline at end of file + var/obj/object2 = null + for(var/i in 1 to 3) + var/obj/O = new /obj(locate(1,1,1)) + O.name = "object [i]" + if(i==2) + object2 = O + var/startcount = length(world.contents) + var/i = 0 + for(var/O in world) + if(i==0) + del(object2) + ASSERT(length(world.contents) == startcount-1) + i++ + ASSERT(i == startcount-1) \ No newline at end of file diff --git a/Content.Tests/DMProject/Tests/List/worldcontents2.dm b/Content.Tests/DMProject/Tests/List/worldcontents2.dm new file mode 100644 index 0000000000..5a23f5abc4 --- /dev/null +++ b/Content.Tests/DMProject/Tests/List/worldcontents2.dm @@ -0,0 +1,15 @@ +/proc/RunTest() + var/obj/object2 = null + for(var/i in 1 to 3) + var/obj/O = new /obj(locate(1,1,1)) + O.name = "object [i]" + if(i==2) + object2 = O + var/startcount = length(world.contents) + var/i = 0 + for(var/O in world) + if(i==3) + del(object2) + ASSERT(length(world.contents) == startcount-1) + i++ + ASSERT(i == startcount) \ No newline at end of file diff --git a/Content.Tests/DMProject/Tests/List/worldcontents3.dm b/Content.Tests/DMProject/Tests/List/worldcontents3.dm new file mode 100644 index 0000000000..28a427f94f --- /dev/null +++ b/Content.Tests/DMProject/Tests/List/worldcontents3.dm @@ -0,0 +1,12 @@ +/proc/RunTest() + for(var/j in 1 to 3) + var/obj/O = new /obj(locate(1,1,1)) + O.name = "object [j]" + var/startcount = length(world.contents) + var/i = 0 + for(var/O in world) + if(i==1) + new /obj(locate(1,1,1)) + ASSERT(length(world.contents) == startcount+1) + i++ + ASSERT(i == startcount+1) \ No newline at end of file diff --git a/OpenDreamRuntime/AtomManager.cs b/OpenDreamRuntime/AtomManager.cs index e704e7ed50..20fbc2bb67 100644 --- a/OpenDreamRuntime/AtomManager.cs +++ b/OpenDreamRuntime/AtomManager.cs @@ -12,34 +12,11 @@ namespace OpenDreamRuntime { /// - /// A list that can be buffered to prevent changes from being applied until FinishBuffering is called. + /// A list that has special behaviour so we can enumerate world.contents without copying the entire list. /// /// - public sealed class BufferedList : List { - private bool _isBuffering = false; - private List _bufferedAdds = new(); - private List _bufferedRemoves = new(); - public new void Add(T Value) { - if (!_isBuffering) { - base.Add(Value); - return; - } - if(!_bufferedRemoves.Remove(Value)) //if we don't already have a remove queued, add it, otherwise we're just undoing a remove - _bufferedAdds.Add(Value); - } - - public new void AddRange(IEnumerable collection) { - if (!_isBuffering) { - base.AddRange(collection); - return; - } - foreach (var item in collection) - { - if(!_bufferedRemoves.Remove(item)) //if we don't already have a remove queued, add it, otherwise we're just undoing a remove - _bufferedAdds.Add(item); - } - } - + public sealed class WorldContentList : List { + private int _isEnumerating = 0; //keep count of how many enumerations are active (for nested iterations of world.contents) /// /// Removes the first instance of the given value, replacing it with the last value in the list. This is done to avoid shifting the entire list. /// @@ -57,37 +34,30 @@ private bool RemoveSwapLast(T Value) { } public new void Remove(T Value) { - if (!_isBuffering) { + if (_isEnumerating == 0) { RemoveSwapLast(Value); return; + } else { + base.Remove(Value); } - _bufferedAdds.Remove(Value); - _bufferedRemoves.Add(Value); } - public void StartBuffering() { - _isBuffering = true; + public void StartEnumeration() { + _isEnumerating++; } - public void FinishBuffering() { - _isBuffering = false; - foreach (var item in _bufferedAdds) - { - base.Add(item); - } - foreach (var item in _bufferedRemoves) - { - RemoveSwapLast(item); - } - _bufferedAdds.Clear(); - _bufferedRemoves.Clear(); + public void FinishEnumeration() { + if(_isEnumerating > 0) + _isEnumerating--; + else + throw new Exception("FinishEnumeration called without StartEnumeration"); } } public sealed class AtomManager { - public BufferedList Areas { get; } = new(); - public BufferedList Turfs { get; } = new(); - public BufferedList Movables { get; } = new(); - public BufferedList Objects { get; } = new(); - public BufferedList Mobs { get; } = new(); + public WorldContentList Areas { get; } = new(); + public WorldContentList Turfs { get; } = new(); + public WorldContentList Movables { get; } = new(); + public WorldContentList Objects { get; } = new(); + public WorldContentList Mobs { get; } = new(); public int AtomCount => Areas.Count + Turfs.Count + Movables.Count + Objects.Count + Mobs.Count; [Dependency] private readonly IEntityManager _entityManager = default!; diff --git a/OpenDreamRuntime/Procs/DreamEnumerators.cs b/OpenDreamRuntime/Procs/DreamEnumerators.cs index c936d0d3af..31d545c5e9 100644 --- a/OpenDreamRuntime/Procs/DreamEnumerators.cs +++ b/OpenDreamRuntime/Procs/DreamEnumerators.cs @@ -136,32 +136,73 @@ sealed class WorldContentsEnumerator : IDreamValueEnumerator { private readonly AtomManager _atomManager; private readonly TreeEntry? _filterType; private int _current = -1; + private int _startCount = 0; public WorldContentsEnumerator(AtomManager atomManager, TreeEntry? filterType) { _atomManager = atomManager; _filterType = filterType; if(filterType is not null) - if(filterType.ObjectDefinition.IsSubtypeOf(filterType.ObjectDefinition.ObjectTree.Area)) - atomManager.Areas.StartBuffering(); - else if(filterType.ObjectDefinition.IsSubtypeOf(filterType.ObjectDefinition.ObjectTree.Mob)) - atomManager.Mobs.StartBuffering(); - else if(filterType.ObjectDefinition.IsSubtypeOf(filterType.ObjectDefinition.ObjectTree.Obj)) - atomManager.Objects.StartBuffering(); - else if(filterType.ObjectDefinition.IsSubtypeOf(filterType.ObjectDefinition.ObjectTree.Turf)) - atomManager.Turfs.StartBuffering(); - else if(filterType.ObjectDefinition.IsSubtypeOf(filterType.ObjectDefinition.ObjectTree.Movable)) - atomManager.Movables.StartBuffering(); + if(filterType.ObjectDefinition.IsSubtypeOf(filterType.ObjectDefinition.ObjectTree.Area)) { + atomManager.Areas.StartEnumeration(); + _startCount = atomManager.Areas.Count; + } else if(filterType.ObjectDefinition.IsSubtypeOf(filterType.ObjectDefinition.ObjectTree.Mob)) { + atomManager.Mobs.StartEnumeration(); + _startCount = atomManager.Mobs.Count; + } else if(filterType.ObjectDefinition.IsSubtypeOf(filterType.ObjectDefinition.ObjectTree.Obj)) { + atomManager.Objects.StartEnumeration(); + _startCount = atomManager.Objects.Count; + } else if(filterType.ObjectDefinition.IsSubtypeOf(filterType.ObjectDefinition.ObjectTree.Turf)) { + atomManager.Turfs.StartEnumeration(); + _startCount = atomManager.Turfs.Count; + } else if(filterType.ObjectDefinition.IsSubtypeOf(filterType.ObjectDefinition.ObjectTree.Movable)) { + atomManager.Movables.StartEnumeration(); + _startCount = atomManager.Movables.Count; + } else { - atomManager.Areas.StartBuffering(); - atomManager.Mobs.StartBuffering(); - atomManager.Objects.StartBuffering(); - atomManager.Turfs.StartBuffering(); - atomManager.Movables.StartBuffering(); + atomManager.Areas.StartEnumeration(); + atomManager.Mobs.StartEnumeration(); + atomManager.Objects.StartEnumeration(); + atomManager.Turfs.StartEnumeration(); + atomManager.Movables.StartEnumeration(); + _startCount = atomManager.AtomCount; } } public bool Enumerate(DMProcState state, DreamReference? reference) { do { + if(_filterType is null){ + if(_startCount != _atomManager.AtomCount){ + _current = _current - Math.Max(0, _startCount - _atomManager.AtomCount); //if we got smaller, we need to adjust our current index + _startCount = _atomManager.AtomCount; + } + } else { + if(_filterType.ObjectDefinition.IsSubtypeOf(_filterType.ObjectDefinition.ObjectTree.Area)) { + if(_startCount != _atomManager.Areas.Count){ + _current = _current - Math.Max(0, _startCount - _atomManager.Areas.Count); //if we got smaller, we need to adjust our current index + _startCount = _atomManager.Areas.Count; + } + } else if(_filterType.ObjectDefinition.IsSubtypeOf(_filterType.ObjectDefinition.ObjectTree.Mob)) { + if(_startCount != _atomManager.Mobs.Count){ + _current = _current - Math.Max(0, _startCount - _atomManager.Mobs.Count); //if we got smaller, we need to adjust our current index + _startCount = _atomManager.Mobs.Count; + } + } else if(_filterType.ObjectDefinition.IsSubtypeOf(_filterType.ObjectDefinition.ObjectTree.Obj)) { + if(_startCount != _atomManager.Objects.Count){ + _current = _current - Math.Max(0, _startCount - _atomManager.Objects.Count); //if we got smaller, we need to adjust our current index + _startCount = _atomManager.Objects.Count; + } + } else if(_filterType.ObjectDefinition.IsSubtypeOf(_filterType.ObjectDefinition.ObjectTree.Turf)) { + if(_startCount != _atomManager.Turfs.Count){ + _current = _current - Math.Max(0, _startCount - _atomManager.Turfs.Count); //if we got smaller, we need to adjust our current index + _startCount = _atomManager.Turfs.Count; + } + } else if(_filterType.ObjectDefinition.IsSubtypeOf(_filterType.ObjectDefinition.ObjectTree.Movable)) { + if(_startCount != _atomManager.Movables.Count){ + _current = _current - Math.Max(0, _startCount - _atomManager.Movables.Count); //if we got smaller, we need to adjust our current index + _startCount = _atomManager.Movables.Count; + } + } + } _current++; if (_current >= _atomManager.AtomCount) { if (reference != null) @@ -181,21 +222,21 @@ public bool Enumerate(DMProcState state, DreamReference? reference) { void IDreamValueEnumerator.EndEnumeration() { if(_filterType is not null) if(_filterType.ObjectDefinition.IsSubtypeOf(_filterType.ObjectDefinition.ObjectTree.Area)) - _atomManager.Areas.FinishBuffering(); + _atomManager.Areas.FinishEnumeration(); else if(_filterType.ObjectDefinition.IsSubtypeOf(_filterType.ObjectDefinition.ObjectTree.Mob)) - _atomManager.Mobs.FinishBuffering(); + _atomManager.Mobs.FinishEnumeration(); else if(_filterType.ObjectDefinition.IsSubtypeOf(_filterType.ObjectDefinition.ObjectTree.Obj)) - _atomManager.Objects.FinishBuffering(); + _atomManager.Objects.FinishEnumeration(); else if(_filterType.ObjectDefinition.IsSubtypeOf(_filterType.ObjectDefinition.ObjectTree.Turf)) - _atomManager.Turfs.FinishBuffering(); + _atomManager.Turfs.FinishEnumeration(); else if(_filterType.ObjectDefinition.IsSubtypeOf(_filterType.ObjectDefinition.ObjectTree.Movable)) - _atomManager.Movables.FinishBuffering(); + _atomManager.Movables.FinishEnumeration(); else { - _atomManager.Areas.FinishBuffering(); - _atomManager.Mobs.FinishBuffering(); - _atomManager.Objects.FinishBuffering(); - _atomManager.Turfs.FinishBuffering(); - _atomManager.Movables.FinishBuffering(); + _atomManager.Areas.FinishEnumeration(); + _atomManager.Mobs.FinishEnumeration(); + _atomManager.Objects.FinishEnumeration(); + _atomManager.Turfs.FinishEnumeration(); + _atomManager.Movables.FinishEnumeration(); } } }