Damnit. I cannot look into fixing this in the immediate future. Based on the things this commit changes, there is a very small chance that the wrong simulation state is the one of the rejoining player, i.e. fixing/reverting this would not change the sim state, and people with the bug and people without would not go OOS. If that is the case re-releasing with the fix would be less of a problem.

Else I am still against re-releasing a simulation-incompatible version (regardless of whether this is fixed or reverted). If after the meeting discussion I am in the minority, I cannot work on a fix myself soon...

I am as puzzled as temple regarding the square vs. circular difference. That usually comes from passability differences at the border of the map, which are masked by the circular blocker that is added. But here the whole terrain is touched, so I'd assume inconsistencies, if any, should happen on all kinds of maps. ?

Without fixing people won't be able to rejoin on square maps (also territory is wrong, since it ignores hills and water, but original players can still play it), but there's only a few square maps:

random/latium.json:		"CircularMap" : false
random/corsica.json:		"CircularMap" : false
random/phoenician_levant.json:		"CircularMap" : false
random/wall_demo.json:		"CircularMap" : false,
scenarios/Arcadia.xml:  "CircularMap": false,
scenarios/Campaign Test Map.xml:  "CircularMap": false,
skirmishes/Watering Holes (4).xml:  "CircularMap": false,

So I don't know if we necessarily need to include in an A23 rerelease.

The weirder thing is the Kunst replay on mainland (in #5162), which had similar territory problems but reversed: trees were gaia meaning probably hills were there (can only guess since the territory grid isn't serialized), except they weren't. So maybe he somehow generated the wrong terrain even though everything else was the same. Dunno.

/ps/trunk/source/simulation2/components/CCmpPathfinder.cpp
560	This creates `m_Grid` when previously it didn't exist. In helpers/Setup.js circular maps do `cmpObstructionManager.SetPassabilityCircular(true);` which does `UpdateGrid()` (in CCmpPathfinder.cpp), but square maps don't do that. Previously then square maps would do `GetPassabilityGrid()` and run `UpdateGrid()` from there since `m_Grid` didn't exist yet. But now it exists, so it returns the grid even though it's still empty. Probably different possible fixes. Maybe the best would be in Setup.js, do `cmpObstructionManager.SetPassabilityCircular(!!settings.CircularMap)` instead or something.

If our assumptions are correct, the square map bug can be fixed for alpha 23 without bumping the version number, because it can only change the simstate on square maps and rejoin is already entirely broken there.
Calling the SetCircular function invariably is my first idea too, but didn't confirm reasonability yet.

About the KunstRaucher replay, dunno,
MakeDirty() in CCmpTerritoryManager deletes m_Territories, so maybe there it was cleaned after the rejoin but not updated yet and then the zeroes ended up in the newTerritory = cmpTerritoryManager->GetOwner(m_X, m_Z); line of CCmpPosition.
(Was trying to find a reason why these trees are special, but I could only find a tiny bump below them, which the other players might not have to that extent.)

/ps/trunk/source/simulation2/components/CCmpPathfinder.cpp
560	+ an `ENSURE(m_Grid);` prior to the inserted hunk, so that this issue can't repeat?

Maybe @Clockwork-Muse knows :) After all he wrote that patch.

In rP21675#30779, @Stan wrote:

Maybe @Clockwork-Muse knows :) After all he wrote that patch.

Really? I thought Itms wrote it, no? Clockwork-Muse wrote an other resize patch AFAIK.

Ah nvm

temple added inline comments.May 29 2018, 3:43 AM

/ps/trunk/source/simulation2/components/CCmpPathfinder.cpp
560	Rather than create a new `m_Grid`, it seems better to just delete it. Then when the grid is needed it'll be created (correctly). Otherwise in particular the passability grid is wrong (all 1's) as seen on Latium as well as after changing the map size in Atlas. (Also test for `m_Grid` in the if; also the Setup.js change isn't needed (although maybe it's a good idea anyway).)