Pathfind changes cause Retail CRC Mismatch

[xezon]

helmut:

This replay is currently mismatching

00-43-03_2v2_player_fh_abed_HardAI.zip

And #2140 seems to be responsible

[Mauller]

I will check the replay later, but if it was made recently with a TSH build then it will mismatch after the crash point. since the post cashpoint pathfinding functionality has changed with #2140. So we need to ignore replays like this at that point.

If it's old and plays back in retail then i don't understand why the replay did not crash since it has triggered a crash point in the pathfinding.

[Skyaero42]

Checked replay:

• It does not crash nor mismatch in retail
• In main, it matches just after the 8 minute mark. The game is more then 20 minutes long
• There is no "pathfinding failover" message, possibly indicating this occurs before failover.

[Caball009]

It does not crash nor mismatch in retail

AFAIK you're comparing score screens to determine mismatches, so I also checked if there was an actual mismatch with retail executable and there was none.

Edit: I can confirm that the mismatch goes away without #2140, tested with 9479df5

[Mauller]

So having looked into this more, i reverted the second commit from #2140 which adds the failover codepath and the replay was okay.

I then added back the commit but removed all of the code within if(s_useFixedPathfinding) blocks and the mismatch stopped.

i then added the blocks back one at a time till i got to PathfindCell::removeObstacle and the following block

	if (s_useFixedPathfinding) {
		if (m_obstacleID != obstacle->getID()) return false;
		m_type = PathfindCell::CELL_CLEAR;
		m_obstacleID = INVALID_ID;
		m_obstacleIsFence = false;
		m_obstacleIsTransparent = false;
		return true;
	}

when this was removed the replay no longer mismatched. So i put the code back in and added a message to appear in the ingameui when entering this block. The block was never entered but the game still mistmatched.

PathfindCell::removeObstacle is not an inlined function, so i think there is some stack shenanigans with an uninitialised variable going on. And the increased size of the PathfindCell::removeObstacle function is causing it to have a different value.

when i extract the contents of the above code block into their own function, even when inlining that function it was not mismatching anymore. So something external to the pathfinding and PathfindCell::removeObstacle is being affected.

[Caball009]

#2140 likely introduced a change in the stack frame size for Pathfinder::classifyFence which exposes the issue of uninitialized variables in that function.

I looked into it and here are my findings:

1. Adding inline to the function definition of PathfindCell::removeObstacle is enough to make the mismatch go away without other changes.
2. bugfix(pathfinder): Fix uninitialized variable in Pathfinder::classifyFence() and possible source of mismatches in Retail-compatible builds #1748
   https://github.com/TheSuperHackers/GeneralsGameCode/pull/1748/files#diff-85bcd76d94e9837e81475c53abcf88af2c7e1e5f2e916cfd56b6f8e25734f866R3865-R3879
   If I change that code to the code below, the mismatch goes away:

IRegion2D cellBounds;
cellBounds.lo.x = REAL_TO_INT_FLOOR((pos->x + 0.5f)/PATHFIND_CELL_SIZE_F);
cellBounds.lo.y = REAL_TO_INT_FLOOR((pos->y + 0.5f)/PATHFIND_CELL_SIZE_F);
cellBounds.hi.x = -1;
cellBounds.hi.y = -1;

3. So I went a step further and changed the values so I could search for the instructions in assembly. The code below still works without a mismatch:

IRegion2D cellBounds;
cellBounds.lo.x = REAL_TO_INT_FLOOR((pos->x + 0.5f)/PATHFIND_CELL_SIZE_F);
cellBounds.lo.y = REAL_TO_INT_FLOOR((pos->y + 0.5f)/PATHFIND_CELL_SIZE_F);
cellBounds.hi.x = 0xDEADC0DE; // -559'038'242
cellBounds.hi.y = 0xDEC0ADDE; // -557'797'922

4. So I changed the 14 instructions for the assignments of hi.x and hi.y into NOPs, and the mismatch is back at the same frame.

0056EE70 | C745 B4 DEC0ADDE | mov dword ptr ss:[ebp-4C],DEADC0DE
0056EE77 | C745 B8 DEADC0DE | mov dword ptr ss:[ebp-48],DEC0ADDE

5. PathfindCell::removeObstacle is called downstream of Pathfinder::classifyFence. Changes to the latter function may increase the stack frame size for the former. If the stack frame size changes it means that the values of uninitialized variables may change. That's why just the code below also makes the mismatch go away:

size_t val = 0;
void Pathfinder::classifyFence( Object *obj, Bool insert )
{
	//
	// artificially increase the stack frame size to change the value of uninitialized variables
	size_t arr[32];
	arr[val & (ARRAY_SIZE(arr) - 1)] = obj->getOrientation();
	val = arr[val & (ARRAY_SIZE(arr) - 1)];
	//

Initialize your **** variables.

[xezon]

Very good investigations. I think this makes a strong case that #1748 needs merging.

Which mismatch fixes need merging now in one go?