From 5765e51c56aa7bc99646474ddf3fbe9f10e69d27 Mon Sep 17 00:00:00 2001 From: RoosterDragon Date: Mon, 19 Sep 2022 09:48:35 +0100 Subject: [PATCH] Fix crushables and crates causing HPF to crash. When crushables and crates change their Location/TopLeft, their crushability is cached, but when their CenterPosition is changed, their cached crushability is not refreshed. Since their CrushableBy functions depends on IsAtGroundLevel, which depends on the CenterPosition, this means that when the crushability is cached it will depend on the current height of the object. If the height of the object changes, the cache is not refreshed and now contains out of date information. The Locomotor cache and the HPF both cache this same information, but at different times. HPF caches immediately, but Locomotor caches on demand which means there can be a delay. This means they can have inconsistent, differing views of the crushability information. This eventually surfaces in a "The abstract path should never be searched for an unreachable point." error from HPF when it detects the inconsistency. The bug is that Locomotor was caching information without refreshing it when required. Fixing this to refresh the cache when the CenterPosition changes is likely to have negative performance impacts. As would removing crushability from the cache. These would both be fixes that address the underlying bug. The high impacts of a proper fix lead us to a workaround instead. If we set the CenterPosition before setting the Location, then when the Location is set and the caches are refreshed, the new CenterPosition is available when caching the crushability information. This means logic depending on IsAtGroundLevel will get the new information and cache a more up-to-date view of things. This means when changing both the CenterPosition and Location together we now cache correct information. However calls that set only the CenterPosition and not the Location can still result in a bad cache state. Although this is imperfect it is an improvement over current affairs, and has less impact. --- OpenRA.Mods.Common/Activities/UnloadCargo.cs | 5 ++++- .../Scripting/Properties/MobileProperties.cs | 5 ++++- OpenRA.Mods.Common/Traits/Crates/Crate.cs | 10 ++++++++-- OpenRA.Mods.Common/Traits/Mobile.cs | 15 ++++++++++++--- OpenRA.Mods.Common/Traits/ParaDrop.cs | 6 ++++-- 5 files changed, 32 insertions(+), 9 deletions(-) diff --git a/OpenRA.Mods.Common/Activities/UnloadCargo.cs b/OpenRA.Mods.Common/Activities/UnloadCargo.cs index b76e16551a..9723c8d040 100644 --- a/OpenRA.Mods.Common/Activities/UnloadCargo.cs +++ b/OpenRA.Mods.Common/Activities/UnloadCargo.cs @@ -120,8 +120,11 @@ namespace OpenRA.Mods.Common.Activities var move = actor.Trait(); var pos = actor.Trait(); - pos.SetPosition(actor, exitSubCell.Value.Cell, exitSubCell.Value.SubCell); + // HACK: Call SetCenterPosition before SetPosition + // So when SetPosition calls ActorMap.CellUpdated + // the listeners see the new CenterPosition. pos.SetCenterPosition(actor, spawn); + pos.SetPosition(actor, exitSubCell.Value.Cell, exitSubCell.Value.SubCell); actor.CancelActivity(); w.Add(actor); diff --git a/OpenRA.Mods.Common/Scripting/Properties/MobileProperties.cs b/OpenRA.Mods.Common/Scripting/Properties/MobileProperties.cs index 1e718cf4bf..702e6b5310 100644 --- a/OpenRA.Mods.Common/Scripting/Properties/MobileProperties.cs +++ b/OpenRA.Mods.Common/Scripting/Properties/MobileProperties.cs @@ -46,9 +46,12 @@ namespace OpenRA.Mods.Common.Scripting [Desc("Moves from outside the world into the cell grid.")] public void MoveIntoWorld(CPos cell) { + // HACK: Call SetCenterPosition before SetPosition + // So when SetPosition calls ActorMap.CellUpdated + // the listeners see the new CenterPosition. var pos = Self.CenterPosition; - mobile.SetPosition(Self, cell); mobile.SetCenterPosition(Self, pos); + mobile.SetPosition(Self, cell); Self.QueueActivity(mobile.ReturnToCell(Self)); } diff --git a/OpenRA.Mods.Common/Traits/Crates/Crate.cs b/OpenRA.Mods.Common/Traits/Crates/Crate.cs index e591880566..4e5f888246 100644 --- a/OpenRA.Mods.Common/Traits/Crates/Crate.cs +++ b/OpenRA.Mods.Common/Traits/Crates/Crate.cs @@ -183,16 +183,22 @@ namespace OpenRA.Mods.Common.Traits // Sets the location (Location) and position (CenterPosition) public void SetPosition(Actor self, WPos pos) { + // HACK: Call SetCenterPosition before SetLocation + // So when SetLocation calls ActorMap.CellUpdated + // the listeners see the new CenterPosition. var cell = self.World.Map.CellContaining(pos); - SetLocation(self, cell); SetCenterPosition(self, self.World.Map.CenterOfCell(cell) + new WVec(WDist.Zero, WDist.Zero, self.World.Map.DistanceAboveTerrain(pos))); + SetLocation(self, cell); } // Sets the location (Location) and position (CenterPosition) public void SetPosition(Actor self, CPos cell, SubCell subCell = SubCell.Any) { - SetLocation(self, cell); + // HACK: Call SetCenterPosition before SetLocation + // So when SetLocation calls ActorMap.CellUpdated + // the listeners see the new CenterPosition. SetCenterPosition(self, self.World.Map.CenterOfCell(cell)); + SetLocation(self, cell); } // Sets only the CenterPosition diff --git a/OpenRA.Mods.Common/Traits/Mobile.cs b/OpenRA.Mods.Common/Traits/Mobile.cs index f776b1703a..b629934e98 100644 --- a/OpenRA.Mods.Common/Traits/Mobile.cs +++ b/OpenRA.Mods.Common/Traits/Mobile.cs @@ -468,22 +468,28 @@ namespace OpenRA.Mods.Common.Traits public void SetPosition(Actor self, CPos cell, SubCell subCell = SubCell.Any) { subCell = GetValidSubCell(subCell); - SetLocation(cell, subCell, cell, subCell); var position = cell.Layer == 0 ? self.World.Map.CenterOfCell(cell) : self.World.GetCustomMovementLayers()[cell.Layer].CenterOfCell(cell); + // HACK: Call SetCenterPosition before SetLocation + // So when SetLocation calls ActorMap.CellUpdated + // the listeners see the new CenterPosition. var subcellOffset = self.World.Map.Grid.OffsetOfSubCell(subCell); SetCenterPosition(self, position + subcellOffset); + SetLocation(cell, subCell, cell, subCell); FinishedMoving(self); } // Sets the location (fromCell, toCell, FromSubCell, ToSubCell) and CenterPosition public void SetPosition(Actor self, WPos pos) { + // HACK: Call SetCenterPosition before SetLocation + // So when SetLocation calls ActorMap.CellUpdated + // the listeners see the new CenterPosition. var cell = self.World.Map.CellContaining(pos); - SetLocation(cell, FromSubCell, cell, FromSubCell); SetCenterPosition(self, self.World.Map.CenterOfSubCell(cell, FromSubCell) + new WVec(0, 0, self.World.Map.DistanceAboveTerrain(pos).Length)); + SetLocation(cell, FromSubCell, cell, FromSubCell); FinishedMoving(self); } @@ -686,8 +692,11 @@ namespace OpenRA.Mods.Common.Traits subCell = self.World.Map.Grid.DefaultSubCell; // Reserve the exit cell - mobile.SetPosition(self, cell, subCell); + // HACK: Call SetCenterPosition before SetPosition + // So when SetPosition calls ActorMap.CellUpdated + // the listeners see the new CenterPosition. mobile.SetCenterPosition(self, pos); + mobile.SetPosition(self, cell, subCell); if (delay > 0) QueueChild(new Wait(delay)); diff --git a/OpenRA.Mods.Common/Traits/ParaDrop.cs b/OpenRA.Mods.Common/Traits/ParaDrop.cs index 7cef409f62..049f70ba9b 100644 --- a/OpenRA.Mods.Common/Traits/ParaDrop.cs +++ b/OpenRA.Mods.Common/Traits/ParaDrop.cs @@ -102,10 +102,12 @@ namespace OpenRA.Mods.Common.Traits self.World.AddFrameEndTask(w => { - dropPositionable.SetPosition(dropActor, dropCell, dropSubCell); - + // HACK: Call SetCenterPosition before SetPosition + // So when SetPosition calls ActorMap.CellUpdated + // the listeners see the new CenterPosition. var dropPosition = dropActor.CenterPosition + new WVec(0, 0, self.CenterPosition.Z - dropActor.CenterPosition.Z); dropPositionable.SetCenterPosition(dropActor, dropPosition); + dropPositionable.SetPosition(dropActor, dropCell, dropSubCell); w.Add(dropActor); });