Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix crushables and parachuted crates causing HPF to crash. #20312

Merged
merged 1 commit into from Sep 24, 2022

Conversation

RoosterDragon
Copy link
Member

@RoosterDragon RoosterDragon commented Sep 19, 2022

Fixes #20307, fixes #20308, fixes #20309

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.


Sequence of events causing issue on bleed:

  • A crate is airdropped and lands
  • The crate's Location is set
  • ActorMap.CellUpdated is fired.
  • Locomotor notes the cell as dirty in the cache
  • HPF caches the crushability of the cell. As the crate is not IsAtGroundLevel, it cannot be crushed.
  • The crate's CenterPosition is set (IsAtGroundLevel would now be true)
  • ActorMap.CellUpdated does NOT get fired for CenterPosition updates. So Locomotor and HPF do nothing.
  • Later, a path search begins that goes through the cell with the crate in.
  • Locomotor notes it has a dirty cache and refreshes. It caches the crushability of the cell. As the crate IsAtGroundLevel, it can be crushed.
  • The path search tries to enter the cell, as Locomotor thinks it can be entered, because the crate can be crushed. HPF throws a wobbly because it sees the cell as unreachable - it contains an uncrushable crate.

Sequence in this PR

  • A crate is airdropped and lands
  • The crate's CenterPosition is set (IsAtGroundLevel would now be true)
  • ActorMap.CellUpdated does NOT get fired for CenterPosition updates. So Locomotor and HPF do nothing.
  • The crate's Location is set
  • ActorMap.CellUpdated is fired.
  • Locomotor notes the cell as dirty in the cache
  • HPF caches the crushability of the cell. As the crate IsAtGroundLevel, it can be crushed.
  • Later, a path search begins that goes through the cell with the crate in.
  • Locomotor notes it has a dirty cache and refreshes. It caches the crushability of the cell. As the crate IsAtGroundLevel, it can be crushed.
  • The path search tries to enter the cell, as Locomotor thinks it can be entered, because the crate can be crushed. HPF also sees the cell can be entered because the crate can be crushed. No error.

@pchote
Copy link
Member

pchote commented Sep 19, 2022

Can we please add a short comment at each code location explaining that the order is important for cache consistency?

@RoosterDragon
Copy link
Member Author

Updated with a HACK comment.

Copy link
Member

@PunkPun PunkPun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just the comments need to be updated. In some places we use SetLocation and in others SetPosition. yet the comments always say SetPosition

OpenRA.Mods.Common/Traits/Crates/Crate.cs Outdated Show resolved Hide resolved
@anvilvapre
Copy link
Contributor

If the comments occur in many places, perhaps a helper method would also make it clear i.e. PosExts.SetCenterPositionBeforePosition(...) and add the comment there once.

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.
@RoosterDragon
Copy link
Member Author

Updated to fix comments.

@Mailaender Mailaender changed the title Fix crushables and crates causing HPF to crash. Fix crushables and parachuted crates causing HPF to crash. Sep 24, 2022
@Mailaender Mailaender merged commit 5765e51 into OpenRA:bleed Sep 24, 2022
@Mailaender
Copy link
Member

Changelog

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment