[PATCH] Gamesetup defaults to unassigned instead of AI when switching the map

[mimo]

When playing (in sp) any skirmish map, only the two first players are assigned. For example, when playing a map named 2v2, i would expect that 4 players (i.e. 3 AIs) are assigned without having to do it manually myself. And also i would expect that the default teams correspond. In duelling cliffs 3v3, the first 3 players have no team, the fourth and sixth have team 2 and the fifth team 1. That is certainly disturbing for a new player. There is the same problem in scenario maps.

[Itms]

I think that comes, once again, from the bogus persistent match settings. Also, even though it's annoying, I think it shouldn't be put to Alpha 21, since we're starting feature freeze. New tickets are supposed to go to Backlog.

Would you agree?

[leper]

This issue popped up recently, so it seems unlikely to be caused by the persistent settings. Also breaking the game for most players (or does anyone actually think that the few people in the lobby are actually representative) does not seem like a feature.

[Itms]

Sorry, I didn't think the issue was new. Thanks for the heads up.

[mimo]

I had checked off the persistent match settings option, and removed the json file. So not coming from that. As leper said, it is a recent regression (not there in A20) and i would rather put it as a release blocker as most new players won't notice that their enemies are unassigned and will be disapointed to find that they have no enemies only after having played for some time.

[elexis]

• The issue was introduced 5 months ago in r18127. I'm aware of this issue since 3 months after someone reported it in irc (comment:6:ticket:3994). Sorry that I didn't get to solving it earlier.

• The issue was introduced by r18127 since that added the "AI" property to every g_GameAttributes.settings.PlayerData[i]. The code in selectMap however was designed to catch the missing AI property in player settings and default to petra AI if the map wouldn't specify it.

• The attached patch fixes that issue by merging the two AI defaulting mechanisms and using the right default value. The code would become a bit nicer IMO if we also replace in case the map author has specified emptystring (i.e. unassigned) to that player slot. However we presumably want to give the map author the ability to set that player as unassigned by default.

[mimo]

the patch fixes the AIs assignment, but there is still a problem with team assignments. Try for example with the team Oasis 2v2. Futhermore, this team assignment seems to depend on the previously selected maps in the session (even without the persist match option). Some missing reset when selecting a new map ?

[elexis]

• The Civilization and Team setting are persisted when switching the skirmish map, since it's the players choice (contrary to scenario maps), since random map types do it too and since it eases setup (since the map is not always the first thing to be decided upon), added in #3120 (r18458)

• Some skirmish maps like Dueling Cliffs (3v3) come with some non-default player distribution. For that map the idea was that one player of each team is placed between 2 enemies, but get's starting towers.

[mimo]

But have you tried ? the present behaviour looks stupid to me. When you select skirmish, the default map is a two player map with no team, so whatever the map you will select, the first two players will always have NONE as team even if the map had some predefined teams! that has no sense. Persistification of the user choice would be valid when there is a user choice, but not when it is the default of the previous selected map. And even then, it would look more consistent to me to keep previous choices only when you select a map with the same number of players. If you select a 2v2 map and choose players 1 and 2 in the same team, and then change your mind and select a 2 players map, you certainly not want both players in the same team.

The behaviour in A20 was more saner to my mind.

Civs choice is a bit different, and it makes sense to persistify it.

[elexis]

• Can I call that patch for the other bug reviewed?

• You are right, maps that come with teamnumbers in the playerdata break that feature. Looks like it has to be reverted, if you agree with this:

There is a conflict: Skirmish maps must be versatile enough so that the host can change arbitrary settings, while skirmish maps must also enforce some settings like team number. The enforcement is currently different (inconsistent) between scenario and skirmish, skirmish and random maps.

Instead all map types could be treated identically. If a map specifies a setting, the setting will become locked and can't be changed by the controller.

So maps that specify team numbers would mean the team dropdowns become disabled. Same goes for other settings like the victory condition f.e.. Thus persistance and disabling of settings could become independent of the maptype. (Some unneeded settings should be removed from maps then.)

This approach would mean that random maps can lockdown settings too.

[mimo]

Replying to elexis:

• Can I call that patch for the other bug reviewed?

I've tested it, and it works nicely, but I've not looked at the code.

• You are right, maps that come with teamnumbers in the playerdata break that feature. Looks like it has to be reverted, if you agree with this:

There is a conflict: Skirmish maps must be versatile enough so that the host can change arbitrary settings, while skirmish maps must also enforce some settings like team number. The enforcement is currently different (inconsistent) between scenario and skirmish, skirmish and random maps.

Instead all map types could be treated identically. If a map specifies a setting, the setting will become locked and can't be changed by the controller.

Why would we want to have them locked, as this would give less freedom to the player? couldn't we just have a minimal change compared to what we have now: if a map come with some settings, these settings are loaded when selecting the map but the user can still change them (but here also, i've not had time to look at the code, so i may miss something).

So maps that specify team numbers would mean the team dropdowns become disabled. Same goes for other settings like the victory condition f.e.. Thus persistance and disabling of settings could become independent of the maptype. (Some unneeded settings should be removed from maps then.)

This approach would mean that random maps can lockdown settings too.

[elexis]

Revert the persistance of playersettings in r18458, keeps the code simplification and removal of duplicate code. (Includes previous patch for easier testing).

[elexis]

(Doing a revert here since it would require significantly more complexity to let the code take the decision which attributes to preserve and which to overwrite; most likely going to introduce bugs, (f.e. first chosen map specifying an attribute, next chosen map not specifying that attribute and expecting it to be undefined (for example whether the ptolemian lighthouse can be built (#3883))))

[mimo]

yep, i agree that reverting is the best solution for a21. I've tested your version 2 of the patch, and it works for me.

[elexis]

In 18805:

Use the default AI instead of not assigning one when the map doesn't specify it (r18127 broke that). Tested by mimo, refs #4248.

[elexis]

In 18806:

Revert the persistance of playersettings for skirmish maps in r18458, because it overwrites potential team numbers suggested by the map. Tested by mimo, fixes #4248, refs #3120.
Keep the code simplification and removal of duplicate code.

[elexis]

Thanks for the report, explanations and testing mimo!