Paths

Table of Contentst

Differential D1555

Always call SetPassabilityCircular -- Fix OOS after rejoin on square maps
ClosedPublic
Actions

Authored by temple on Jun 2 2018, 8:17 PM.

Details

Reviewers

elexis

Commits

rP21835: Always call SetPassabilityCircular -- Fix OOS after rejoin on square maps

Trac Tickets

#5186

Summary

As described in #5186 and rP21675, the passability grid isn't correct on square maps for the initial players. A quick fix is to call the SetPassabilityCircular function (with false rather than true) so that square maps follow the same code path as circular maps.

Initial players:

Rejoiners:

It's probably better to have a more correct fix since the passability grid is all 1's (doesn't account for terrain) after changing the map size in Atlas, but that can be done later.

Test Plan

See that e.g. Latium has the correct territory (adjusts for terrain) on game start.

Diff Detail

Repository

rP 0 A.D. Public Repository

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

Event Timeline

temple created this revision.Jun 2 2018, 8:17 PM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Default...
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'if' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/helpers/Setup.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/helpers/Setup.js
|   9|   9| 		settings = {};
|  10|  10| 
|  11|  11| 	if (settings.DefaultStance)
|  12|    |-	{
|    |  12|+	
|  13|  13| 		for (let ent of Engine.GetEntitiesWithInterface(IID_UnitAI))
|  14|  14| 		{
|  15|  15| 			let cmpUnitAI = Engine.QueryInterface(ent, IID_UnitAI);
|  16|  16| 			cmpUnitAI.SwitchToStance(settings.DefaultStance);
|  17|  17| 		}
|  18|    |-	}
|    |  18|+	
|  19|  19| 
|  20|  20| 	if (settings.RevealMap)
|  21|  21| 	{

Link to build: https://jenkins.wildfiregames.com/job/differential/619/display/redirect

Harbormaster completed remote builds in B6205: Diff 6708.Jun 2 2018, 8:20 PM

elexis awarded a token.Jun 2 2018, 9:32 PM

Todays review starting with blackbox testing rejoins with the patched and unpatched version.
First I state the Observation, then I deduce the Reason why that observation occured from the patch.

Square maps:
If the host didn't patch, rejoins always fail independent on whether the client patched. That is because host resized empty grid and thus computed a wrong grid whereas rejoiner correctly initializes the grid (with and without the patch).
If the host did patch, rejoins will always succeed independent on whether the client patched. That is because the host now computes the correct grid.

Circular maps:
Rejoins always succeed with and without the patch. That is because every statement that is executed without the patch on circular maps is executed now too on circular maps and no statement that wasn't executed on circular maps is executed with the patch on circular maps.
Alternative evidence is that SetPassabilityCircular computes the correct grid and is called always for rejoiners and always when starting the game as you mentioned.

Q.E.D. correctness

Thanks for the forensics detective work again temple! One OOS bug less that puzzled us and otherwise could have been reportedmultiple times in the next release (especially on the lobby with inconclusive evidence wasting time uselessly) .

I'd suggest to close the OOS ticket, so that we have something as solid on the milestone as the issue and the fix were.
We can track the proposed ideal fix (not resizing empty grids) in the concern of the original commit and a new differential revision proposal with that patch that is quickly written but slowly verified.

binaries/data/mods/public/simulation/helpers/Setup.js
18 ↗	(On Diff #6708)	(can nuke this pair of braces if you want to please the bot)
41 ↗	(On Diff #6708)	(CircularMap should become mandatory eventually, so that default values are not spread across the entire code but are always passed when setting up the game.)
43 ↗	(On Diff #6708)	Yes

This revision is now accepted and ready to land.Jun 3 2018, 3:47 PM

Forgot to repeat the downside that one client with the patch and one client without the patch starting the game do result in an instant OOS. (If singleplayers experience any difference at all after this patch, it's one bugs less when starting the game and no difference on savegame load.)

Comparing instant OOS and OOS on rejoin, it seems better to have the instant OOS?
At least people knowing about the bug can find out that some need to update. Version check would be benefitial here in fact (and it would also inform people to update their stuff when joining a23 lobby, but meh).
People not knowing how to interpret the bug and not caring to report seem doomed either way, just at different times of the game.
As a player I might prefer to play with this patch, because disconnects can and do occur, especially when playing with many players. Late observers can and do join.
Raight?

In D1555#62868, @elexis wrote:

Forgot to repeat the downside that one client with the patch and one client without the patch starting the game do result in an instant OOS. (If singleplayers experience any difference at all after this patch, it's one bugs less when starting the game and no difference on savegame load.)

Well that's a fair observation but more or less irrelevant since different game versions tend to be incompatible anyway. It just means that the update will be mandatory, which for fixing OOS errors and timeouts was pretty much expected.

Closed by commit rP21835: Always call SetPassabilityCircular -- Fix OOS after rejoin on square maps (authored by temple). · Explain WhyJun 3 2018, 6:41 PM

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Path

Size

ps/

trunk/

binaries/

data/

mods/

public/

simulation/

helpers/

Setup.js

21 lines

Diff 6715

View Options

ps/trunk/binaries/data/mods/public/simulation/helpers/Setup.js

Always call SetPassabilityCircular -- Fix OOS after rejoin on square mapsClosedPublicActions