ActorMap, optmizations.

[anvilvapre]

• fixes potiential bug in bleed, both GetActorsAt(CPos, [SubCell]) function did not check if Actor.IsInWorld is true. causing below crash to become obvious - which only occurs after the first attempt implementation.

• first attempt, this won't work in practice: use bin, actor pair to remove actor positions. avoiding the need to traverse all x (100) bins, to search all actors in the bin to try to match all the actors that need to be removed. this does not work since center position may have changed upon removal. potentionally leaving actors behind on the actor map.

• second/final attempt: let actor mantain a IActorMapPositionKey member (hacky) which holds the internal actor map bin object in which the position of the actor is maintained on the actor map. to avoid actor map having to maintain an expensive dictionary that maps actors to bins. this offers the same stable functionality as in bleed (remove by actor, not by position). but offers faster performance. if there can only be one IOccupySpace for an actor and therefore one IPositionable, then the position key could also be maintained here. only the key has a one on one relation with the actor. although if only actors with an IPositionable trait are maintained on the actor map, then it might be better to store the key/bin here.

• in find free cell, avoid potential 3 times lookup of influence node in influence cell layer, by pasing the head influence node directly.

• do no call function recursively to remove element from list. avoid cell layer cell update if head element did not change.

• avoid trait dictionary lookup of IPositionable - by rather try to cast Actor.OccupiesSpace.

(Seems a bit buggy that actors can be removed, their center positions updated and end up being out of sync with position known to the actor map. Also that Mobile does not call UpdateMaps when IsInWorld is false, but the other IPositionables do.)

---

stack trace, due to crate not yet, or no longer being in world, while being queried:
crash only occurs in this branch using the first attempt, not in bleed, likely because the center position of the actor was not in sync with the actor map. likely because IsInWorld is false, while World.Add|Update|RemoveMaps was called causing Add|Update|RemovePosition not to be called.

calling UpdatePosition even if the actor is not in the world is not possible. As IsInWorld is both true during creation and after removal. The first resulting in a null pointer exception because occupyspace is not yet set, when mobile is created.

OpenRA engine version {DEV_VERSION}
Red Alert mod version {DEV_VERSION}
on map e62ae4cfa095d20666b5e855cca710be0b13f114 (Pool Party by PizzaAtomica).
Date: 2022-10-05 20:05:21Z
Operating System: Linux (Unix 5.4.0.126)
Runtime Version: .NET CLR 6.0.8
Exception of type System.InvalidOperationException: Attempted to get trait from destroyed object (crate 333 (not in world))
at OpenRA.TraitDictionary.CheckDestroyed(Actor actor) in /home/abc/source/cs/OpenRA/OpenRA.Game/TraitDictionary.cs:line 84
at OpenRA.TraitDictionary.WithInterface[T](Actor actor) in /home/abc/source/cs/OpenRA/OpenRA.Game/TraitDictionary.cs:line 101
at OpenRA.Actor.TraitsImplementingT in /home/abc/source/cs/OpenRA/OpenRA.Game/Actor.cs:line 392
at OpenRA.Mods.Common.Traits.Locomotor.UpdateCellBlocking(CPos cell) in /home/abc/source/cs/OpenRA/OpenRA.Mods.Common/Traits/World/Locomotor.cs:line 488
at OpenRA.Mods.Common.Traits.Locomotor.GetCache(CPos cell) in /home/abc/source/cs/OpenRA/OpenRA.Mods.Common/Traits/World/Locomotor.cs:line 421
at OpenRA.Mods.Common.Traits.Locomotor.CanMoveFreelyInto(Actor actor, CPos cell, SubCell subCell, BlockedByActor check, Actor ignoreActor) in /home/abc/source/cs/OpenRA/OpenRA.Mods.Common/Traits/World/Locomotor.cs:line 230
at OpenRA.Mods.Common.Traits.Locomotor.CanMoveFreelyInto(Actor actor, CPos cell, BlockedByActor check, Actor ignoreActor) in /home/abc/source/cs/OpenRA/OpenRA.Mods.Common/Traits/World/Locomotor.cs:line 221
at OpenRA.Mods.Common.Traits.Locomotor.MovementCostToEnterCell(Actor actor, CPos srcNode, CPos destNode, BlockedByActor check, Actor ignoreActor) in /home/abc/source/cs/OpenRA/OpenRA.Mods.Common/Traits/World/Locomotor.cs:line 211
at OpenRA.Mods.Common.Pathfinder.DensePathGraph.GetPathCostToNode(CPos srcNode, CPos destNode, CVec direction) in /home/abc/source/cs/OpenRA/OpenRA.Mods.Common/Pathfinder/DensePathGraph.cs:line 180
at OpenRA.Mods.Common.Pathfinder.DensePathGraph.GetConnections(CPos position) in /home/abc/source/cs/OpenRA/OpenRA.Mods.Common/Pathfinder/DensePathGraph.cs:line 131
at OpenRA.Mods.Common.Pathfinder.PathSearch.Expand() in /home/abc/source/cs/OpenRA/OpenRA.Mods.Common/Pathfinder/PathSearch.cs:line 221
at OpenRA.Mods.Common.Pathfinder.PathSearch.FindPath() in /home/abc/source/cs/OpenRA/OpenRA.Mods.Common/Pathfinder/PathSearch.cs:line 290
at OpenRA.Mods.Common.Traits.PathFinder.FindPathToTargetCellByPredicate(Actor self, IEnumerable1 sources, Func2 targetPredicate, BlockedByActor check, Func`2 customCost, Actor ignoreActor, Boolean laneBias) in /home/abc/source/cs/OpenRA/OpenRA.Mods.Common/Traits/World/PathFinder.cs:line 158
at OpenRA.Mods.Common.Activities.FindAndDeliverResources.ClosestHarvestablePos(Actor self) in /home/abc/source/cs/OpenRA/OpenRA.Mods.Common/Activities/FindAndDeliverResources.cs:line 185
at OpenRA.Mods.Common.Activities.FindAndDeliverResources.Tick(Actor self) in /home/abc/source/cs/OpenRA/OpenRA.Mods.Common/Activities/FindAndDeliverResources.cs:line 112
at OpenRA.Activities.Activity.TickOuter(Actor self) in /home/abc/source/cs/OpenRA/OpenRA.Game/Activities/Activity.cs:line 114

[PunkPun]

fixes potiential bug in bleed, both GetActorsAt(CPos, [SubCell]) function did not check if Actor.IsInWorld is true. causing below crash to become obvious - which only occurs after the first attempt implementation.

ActorMap should not be responsible for that. Actors should remove themselves from ActorMap on having left world

this does not work since center position may have changed upon removal

ActorMap should not care about center position (WPos), for occupying space we use Location (CPos).
#20312 was a mistake and is being reverted

[anvilvapre]

fixes potiential bug in bleed, both GetActorsAt(CPos, [SubCell]) function did not check if Actor.IsInWorld is true. causing below crash to become obvious - which only occurs after the first attempt implementation.

ActorMap should not be responsible for that. Actors should remove themselves from ActorMap on having left world

It already did in ActorsInBox. Likely a past crash fix.

this does not work since center position may have changed upon removal

ActorMap should not care about center position (WPos), for occupying space we use Location (CPos). #20312 was a mistake and is being reverted

Not sure if it matters if it's CPos or WPos. The issue is that World.UpdateMaps is called - but ActorMap.UpdatePosition not if the actor is not yet, or no longer in World. Causing (center) position to be updated, but not on the actor map.

An improvement might be to remove the actor from the actor map before setting IsInWorld to false.

[PunkPun]

Not sure if it matters if it's CPos or WPos. The issue is that World.UpdateMaps is called - but ActorMap.UpdatePosition not if the actor is not yet, or no longer in World. Causing (center) position to be updated, but not on the actor map.

Yes it matters. We deliberately avoid using center position when occupying space. Changing center position should NOT affect actor's position on the actor map. For that we use location (CPos)

[abcdefg30]

Removing the IsInWorld (Disposed) checks in GetActorsAt was intentional, since actors that are not in the world should not be registered in the actor map at all (which indicates a bug at another place, which doesn't properly remove actors from the map). See d2a3659.

[anvilvapre]

Removing the IsInWorld (Disposed) checks in GetActorsAt was intentional, since actors that are not in the world should not be registered in the actor map at all (which indicates a bug at another place, which doesn't properly remove actors from the map). See d2a3659.

The check was already being made elsewhere in ActorMap, I believe in ActorsInBox.

It might be better to remove the actor already from the actor map before removing it from the World, if posssible.

[abcdefg30]

It might be better to remove the actor already from the actor map before removing it from the World, if posssible.

That sounds best.

[anvilvapre]

Reverted the change of IsInWorld check in GetActorsAt. This results in past behavior.

I can make a seperate pull request where World.RemoveFromMaps is already called for an actor prior to INotifyRemovedFromWorld.RemovedFromWorld is being called. It requires that all implementation do not also call RemoveFromMaps themselves. Seems best to not include that in this pull request.

Storing the position key as an actor field has a has a big performance benefit. The actor map is updates often a remove and add takes place for each position update.

[anvilvapre]

closing.
splitting up in multiple seperate pull request. one for each - excluding the top two items in the initial comment.