[PATCH] OOS on rejoin, moving through new obstructions with hierarchichal pathfinder

[elexis]

Units can go through obstructions, as reported in #3410. This ticket should deal with one particular case.

Reproduce:

1. Build something
2. Create a big unit (siege or ship)
3. Tell the big unit to move inside the new building (make sure it's a longpath)

By showing the pathfinder overlay in the developer menu, you see that the returned longpath has a goal inside the obstruction. The unit will follow that longpath and enter the building.

Notice the bug only applies to obstructions that are added after the game has started. If the building existed when the game started, the goal of the longpath will be outside the obstruction and the unit will not enter the building (besides #3532)

(Units in formation count as a single big unit (clearance 4) and also get a goal point inside the obstruction. But they disband when hitting the obstruction and don't enter it.)

---

Together with #3532, this bug accounts for the "ship on land" thing, which has been reported here ​https://wildfiregames.com/forum/index.php?showtopic=20083 and happened to me in some games (uploaded in #3513 and #3471). Replaying those games showed that ships always entered the land by moving through the obstructions of docks.

The bug occurs with all obstructions that are added after the game started:

While obstructions that exist when the game started are handled correctly:

Result ingame:

---

Sometimes the chosen path is too long (hopefully only occurs when having this obstruction bug. The catapult started near the fortress and was ordered to move near the center of the temple, see attachment:commands_path_too_long.txt​):

[elexis]

The ship receives a longpath with the goal inside the obstruction (both goals inisde the clearance and inside the building are available).

[elexis]

But if the dock existed when the game started, it works as expected (besides #3532).

[elexis]

Minimal replay for r17144 where a catapult was ordered to move near to the center of the temple. The longpathfinder returns a path allowing to enter the obstruction. Strangely the chosen path is about 3 times as long as it should be (i.e. chosen path is not a shortest path). The catapult receives only one walk-command. Hopefully only occurs when having this obstruction bug.

[elexis]

mimo correctly guessed that this is a serialization issue. This bug occurs with the host, but not with the rejoined client.

[elexis]

This bug, i.e. getting longpaths with the goal inside new buildings and sometimes getting weird / too long paths then exists since r16751.

[elexis]

Reproduce OOS:

1. Start match with late-observer-joins enabled, enable pathfinder overlay
2. Build dock, make ship and move it away
3. Join with observer, enable pathfinder overlay
4. Order the ship to move inside the dock (must be a longpath)

Obervations

• In the pathfinder overlay you will see that
  • The host has the invalid path with the goal inside the building.
  • The rejoined client has the valid path with the goal outside of the building.
• The OOS will occur instantaneously after the rejoined client has issued a move order into a new building, but not earlier.

Related tickets:

• This ticket might be a duplicate of #3292, since this is surely one way to get different waypoints.
• Notice changing the obstructions creates the OOS in #3450 for one turn. So maybe this is related/duplicate.

Further Observations: Also notice that for the host,

• The shortpathfinder does take the new obstruction into account
• The longpathfinder also seems to take it into account, as some segments of the path are often aligned with the edge of the new obstruction

HierarchicalPathfinder::MakeGoalReachable() is called for both clients, but the code after if (reachableContainingGoal.empty()) is only called for the rejoined one!! (So we could be able to deduce from that which update is missing for the host)

[elexis]

By changing "default" to "ship" in BuildTextureRGBA of HierarchicalPathfinder.h, you can visualize the regions on the water. The dock at the left is newly built, the other one existed when the game started.

[elexis]

The dock at the left is newly built, the other one existed when the game started.

Notice though that the visualization seems inaccurate, as the region seems to end at the dock, not at the clearance (RegionContainsGoal returns true in HierarchicalPathfinder::MakeGoalReachable when issuing a longpath request with the goal near the old dock).

[elexis]

Executing m_PathfinderHier.Recompute after adding an obstruction solves the problem, but only m_PathfinderHier.Update is called from CCmpPathfinder::UpdateGrid() as m_ObstructionsDirty.globallyDirty is false when adding new obstructions.

We don't want a global recompute though, only one for the hierarchichal pathfinder, so maybe a new dirty-boolean should be introduced for the hierarchical pathfinder (or can this be solved by partially recomputing it too?)

[Josh]

A partial recompute sounds ideal, but what do you think about doing a full recompute for A19 and pushing partials to A20?

Edit: Elexis; nice job with the ticket description, I really like the gifs.

[wraitii]

In 17152:

Revert the logic change in r16998. This commit removed the checks in UnitMotion against structures, which should have been fine except the short-range pathfinder and the long-range pathfinder are not entirely compatible (check out #3532 for details). This behavior was probably slightly optimized, but it was too clever for its own good in the current state of the pathfinder, might be reintroduced later.

This resulted in ALL "units inside obstructions" issues.

Thanks to elexis for the testing.

Fixes #3532, #3450.
Refs #3538 (still OOSes), #3410 (unitmotion remains buggy for formations, but this is only aesthethic.)
Probably affects #3471 and #3505, but those are not fixed.

[wraitii]

Above patch should fix the issue. The problem was a for/if inversion, which meant that we once we processed for the "default" class all the other passability classes were skipped.

This probably caused a TON of bugs that we just never noticed, since if I'm right all large passability classes had the same issues.

[elexis]

The patch gets an F (for fixed). Well done!

[wraitii]

In 17158:

Fix a logic error in the hierarchical pathfinder that resulted in it not updating all passability classes correctly. Fixes #3538, refs #3292 (it fixes one of the cases, but not the examples).

Also features style fixes, thanks leper for noticing.

[Itms]

Awesome. Thanks a lot for finding this!