Opened 8 years ago

Closed 3 years ago

#3120 closed defect (fixed)

[PATCH] In Match Setup screen, selecting a map shouldn't change the selected civilizations

Reported by: dg1727 Owned by: elexis
Priority: Should Have Milestone: Alpha 24
Component: UI – Game setup Keywords:
Cc: Patch: Phab:D2483

Description (last modified by elexis)

In the Match Setup screen, if you first select one or more civilizations, then change the map (even to one with the same # of players), the selected civilization(s) revert to Random.

Instead, the civilizations should stay at what you selected. This should include if you change to a map with a smaller number of players. If you change to a map with a larger number of players (e.g., from 2 to 4), then (e.g.) the first 2 civilizations should stay at what you selected.

This issue was pointed out in a video by YouTube user WretchedJ, although when he did the video, he didn't notice that the civs had reverted to Random - he was only frustrated that when he began the game, his civ was different than he had selected.

I suppose many players would prefer that if they select 4 specific civs, then mistakenly change to a 2-player map, then change to a different 4-player map, the 3rd & 4th civs should reappear as the values that were just selected, instead of reverting to Random.

Attachments (10)

fix.zip (17.9 KB ) - added by Jared Ryan Bills 7 years ago.
first commit and attempt to fix the problem
diff.txt (1.5 KB ) - added by Jared Ryan Bills 7 years ago.
There, I tried making a proper patch.
3120.patch (1.5 KB ) - added by Jared Ryan Bills 7 years ago.
One last detail
3120.v2.patch (1.4 KB ) - added by Jared Ryan Bills 7 years ago.
Updated according to review
3120.v3.patch (2.1 KB ) - added by Jared Ryan Bills 7 years ago.
Made function applying the fix and applied in 2 different places
3120.v4.patch (2.1 KB ) - added by Jared Ryan Bills 7 years ago.
Fixed one last bug
3120.v5.patch (3.2 KB ) - added by Jared Ryan Bills 7 years ago.
Cleaned up code and moved player (un)assignment into function.
3120.v6.patch (3.3 KB ) - added by Jared Ryan Bills 7 years ago.
Added changes from IRC review.
3120.v7.patch (3.4 KB ) - added by Jared Ryan Bills 7 years ago.
Fixed bug related to gaia player in skirmish maps.
3120.v8.patch (3.3 KB ) - added by Jared Ryan Bills 7 years ago.
Moved sanitizePlayerData above resizePlayerData.

Download all attachments as: .zip

Change History (34)

comment:1 by elexis, 7 years ago

Keywords: simple added

Already the case for random maps, but not skrimish maps (scenario maps have fixed civs).

by Jared Ryan Bills, 7 years ago

Attachment: fix.zip added

first commit and attempt to fix the problem

comment:2 by Jared Ryan Bills, 7 years ago

Resolution: fixed
Status: newclosed
Summary: In Match Setup screen, selecting a map shouldn't change the selected civilizations[Patch] In Match Setup screen, selecting a map shouldn't change the selected civilizations

I attempted to fix the problem, please review.

comment:3 by elexis, 7 years ago

Keywords: patch review added
Milestone: BacklogAlpha 21
Resolution: fixed
Status: closedreopened

by Jared Ryan Bills, 7 years ago

Attachment: diff.txt added

There, I tried making a proper patch.

by Jared Ryan Bills, 7 years ago

Attachment: 3120.patch added

One last detail

comment:4 by Palaxin, 7 years ago

Regarding the credits: the names are sorted according to the nickname, not the real name. Thus you need to look for "O". However, if you don't provide a nickname, then sort according to your surname. Thx

Last edited 7 years ago by Palaxin (previous) (diff)

comment:5 by elexis, 7 years ago

Keywords: review removed

Review:

  • As mentioned on IRC: unneeded parnthesis, newlines after ? and : and don't create variables for the length properties. If you use pData as a variable name for the current player data, it will stay below the 80 character limit suggested in wiki:Coding_Conventions
  • selectNumPlayers has the same logic, it should use a common function
  • As mentioned by Palaxin the contributors entry should be sorted correctly.

If you fix these points, I would commit that.

The patch doesn't address the last point of the ticket description. In order to accomplish that, the civs would have to be saved in g_PlayerAssignments and copied to the player data when starting the game. This however will mean that a lot of things will have to be changed, even the network protocol. Alternatively the assignments could also be stored in a global player data array. This however will introduce a lot of duplicate logic, as that array will have to be updated with every change of the assignments and on join/leave etc. Not completely convinced we should do that, but if someone comes up with an elegant patch for that...

#4037 would help with not killing the currently selected players / civs / teams.

by Jared Ryan Bills, 7 years ago

Attachment: 3120.v2.patch added

Updated according to review

in reply to:  5 comment:6 by Jared Ryan Bills, 7 years ago

Keywords: review added
Resolution: fixed
Status: reopenedclosed
Summary: [Patch] In Match Setup screen, selecting a map shouldn't change the selected civilizations[PATCH] In Match Setup screen, selecting a map shouldn't change the selected civilizations

Very well, I added the latest patch according to the latest criticism. I however contest that I should create variables for the .length properties because calling .length parses for string length each time. Though I didn't actually keep the variables to suit your demands.

comment:7 by Jared Ryan Bills, 7 years ago

Resolution: fixed
Status: closedreopened

comment:8 by Lionkanzen, 7 years ago

You can't close the ticket. Need reviewed and committed , so the developer will close this.

comment:9 by Jared Ryan Bills, 7 years ago

Thanks, that's why I reopened it.

comment:10 by elexis, 7 years ago

  • Since that code is only called when selecting a map and getting the string length takes some microseconds, the performance won't matter in comparison to readability.
  • The code hasn't been merged with the similar code in selectNumPlayers (if you don't understand my comments, just ask instead of doing like that comment wasn't posted ;) ). source:ps/trunk/binaries/data/mods/public/gui/gamesetup/gamesetup.js#L1054 Even if it's just one statement on three lines, the reader would probably appreciate an abbreviation so that he has to understand that code only once. You could call that function resizePlayerData and pass the desired length and the replacement playerdata, which is the maps playerdata in one case and the player defaults in the other.
  • Also a space after //

by Jared Ryan Bills, 7 years ago

Attachment: 3120.v3.patch added

Made function applying the fix and applied in 2 different places

by Jared Ryan Bills, 7 years ago

Attachment: 3120.v4.patch added

Fixed one last bug

comment:11 by elexis, 7 years ago

Keywords: review removed

Thanks for the update!

  • Unneeded newline in resizePlayerData
  • It seems cleaner to not use a default argument there, but just pass the length in both cases
  • Notice everytime the playerdata is resized, some players might become unassigned. Both selectNumPlayers and selectMap have a check to unassign those players in multiplayer and in singleplayer reassign the player to slot 1 in case he became unassigned. So it would be really great for the code to also move that logic to the new function.

by Jared Ryan Bills, 7 years ago

Attachment: 3120.v5.patch added

Cleaned up code and moved player (un)assignment into function.

comment:12 by Jared Ryan Bills, 7 years ago

Keywords: review added

by Jared Ryan Bills, 7 years ago

Attachment: 3120.v6.patch added

Added changes from IRC review.

by Jared Ryan Bills, 7 years ago

Attachment: 3120.v7.patch added

Fixed bug related to gaia player in skirmish maps.

comment:13 by elexis, 7 years ago

Keywords: review removed

Nice, you found a non-obvious bug! :) Now take a look at what sanitizePlayerData does and notice you just have to move the resizePlayerData call one line below instead of duplicating the gaia check.

by Jared Ryan Bills, 7 years ago

Attachment: 3120.v8.patch added

Moved sanitizePlayerData above resizePlayerData.

comment:14 by elexis, 7 years ago

In 18458:

Persist the the valid subset of chosen player settings when changing the skirmish map. Patch by Offensive ePeen, refs #3120.

comment:15 by elexis, 7 years ago

Keywords: simple removed
Milestone: Alpha 21Backlog

Thanks for the patch!

  • Always add the review keyword once you think the patch is ready to commit, otherwise it will not be considered.
  • Trailing whitespace in 4 places
  • Only reassigning the local player to 1 if it's outside of the valid range
  • Renaming ply to maxPlayers and defPData to targetPlayerData and player to playerID, since those names are a bit more in line with the rest of the code.
  • Removing one changes from the comment, that unneeded "reassign player 1" and "on map change" comment.

Keeping the ticket open in case someone comes up with an elegant approach to also solve the persisting of teams/civs in case the number of players is changed from M > N to N back to M. The g_PlayerAssignments change can just be moved to launchGame then, but the playerData settings would have to be cached somewhere (-> likely not elegant).

comment:16 by elexis, 6 years ago

Notice the commit broke the gamesetup when someone with playerID N disconnected, the host reduced the number of players to M<N, and that client rejoining the gamesetup again: #4120.

comment:17 by elexis, 6 years ago

In 18702:

Fix critical gamesetup bug again where the game can't be started after a previously assigned disconnected client rejoins a full game. Same as #3602, reintroduced by 18458, refs #3120. Fixes #4120.

comment:18 by elexis, 6 years ago

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.

comment:19 by Imarok, 4 years ago

Component: UI & SimulationGame setup

Move tickets to Game Setup as UI & Simulation got some sub components.

comment:20 by elexis, 3 years ago

Milestone: BacklogAlpha 24
Owner: set to elexis
Patch: Phab:D2483
Status: reopenednew

comment:21 by elexis, 3 years ago

Component: UI – Game setupAtlas editor
Description: modified (diff)
Keywords: patch removed
Milestone: Alpha 24Backlog

Atlas writes the VictoryConditions to all saved maps, and Atlas doesn't work correctly if they're not specified, so the issue is not limited to the gamesetup part (but the gamesetup part will be fixed by Phab:D2483).

comment:22 by elexis, 3 years ago

Component: Atlas editorUI – Game setup
Milestone: BacklogAlpha 24

wrong ticket (meant #4661)

comment:23 by elexis, 3 years ago

In 23279:

Clean Skirmish maps of default and invalid gamesetting values.

  • The gamesetup class rewrite in D2483 will enable Skirmish and Random maps to fix AIs and Civs by specifying them. Hence remove them from all maps that don't intend to fix them (i.e. all except Egypt 3v3).
  • Remove default values (and the few irrelevant non-default values) so that the player-chosen settings may be persisted between Skirmish map selection changes, refs #3120, D2483.
  • Remove invalid settings: mapType, AISeed and random map settings Script, Size, Seed, Nomad, BaseHeight, BaseTerrain, refs rP20726, rP21274.
  • Remove StartingCamera values since they are useless, refs #4839, D1098.
  • Add the "new" keyword to Atlas Valleys and Vesuvius from rP23278, so they appear in the "new" filter.
  • Unhide Barcania (3)

comment:24 by elexis, 3 years ago

Resolution: fixed
Status: newclosed

In 23374:

Gamesetup class rewrite, fixes #5322, refs #5387.

  • Decouples settings logically which in turn allows fixing many problems arising from previous coupling.
  • Fixes the persist-match-settings feature, refs #2963, refs #3049.
  • Improves performance of the matchsetup by rebuilding GUI objects only when necessary.

Provides groundwork for:

  • UI to control per-player handicap, such as StartingResources, PopulationCap, StartingTechnologies, DisabledTechnologies, DisabledTemplates, ..., refs #812.
  • Map specific settings (onMapChange event), refs #4838.
  • Chat notifications announcing which settings changed, refs D1195,
  • Multiple controllers setting up the game (since setting types can check for permissions in onUpdateGameAttributes without the need for a new data model or a second gamesetup data network message type), refs #3806, subsequently dedicated server, refs #3556.
  • MapBrowser (MapCache, MapTypes, onUpdateGameAttributes interface), refs D1703 and D1777,
  • Multiplayer saved games (decoupling and setting dependent unique logic), refs #1088.

Refs https://wildfiregames.com/forum/index.php?/topic/20787-paid-development-2016/ https://wildfiregames.com/forum/index.php?/topic/20789-paid-development-2016/

Enable maps to restrict setting values:

  • If a map specifies an AI or Civs for a playerslot, the controller can't assign a player/other AI or Civ to that slot, refs #3049, #3013.

Fix per player StartingResources, PopulationCap, StartingTechnologies, DisabledTechnologies, DisabledTemplates following rP12756, refs #812, fixes #4504. Use this for DisabledTechnologies on Polar Sea.

Persist user settings for Skirmish maps:

  • All user chosen settings are persisted when changing the selected map or maptype, except where the selected map overwrites the setting value and except for Scenario maps which still use the default value where the map doesn't specify the setting value.
  • Tickets relating to that Skirmish mapchange user setting persistance:
    • Selecting a map doesn't change the selected civilizations, fixes #3120 (together with r23279 removing map specified Civs).
    • Selecting a map type doesn't reset the selected settings, fixes #5372.
    • Selecting a map doesn't change the selected victory conditions, unless the map specifies those, refs #4661, #3209. (Atlas still writes VictoryConditions to every map.)
    • Consume the player color palette from Skirmish maps, refs rP17040 / #1580. Preserve the selected playercolors when switching the Skirmish/Random map by chosing the most similar colors if the map comes with a different palette.

Rated games:

  • Hide and disable Rated game setting unless there are exactly two players, fixes #3950, supersedes D2117.
  • Display conspicuous warning if the game is rated, so players are perfectly aware.

Autostarted games:

  • Allow using the gamesetup page to autostart matches with arbitrary maps, not only this one tutorial, as reported in D194 and rP19599, refs D11.

Networking:

  • Keep gamesetup page open after disconnect, allowing players to read chat messages indicating why the host stopped the server, fixes #4114.
  • The message subscription system allows new and mod settings to run custom logic on arbitrary setting changes (most importantly on map change). This removes hardcoded logic restrictions from the gamesetup option unification rewrite in rP19504/D322, refs #3994, such as the hardcoding of setting references in selectMap to biomes from rP20115/D852 and the difficulty from rP20760/D1189, RelicDuration, WonderDuration, LastManStanding, RegicideGarrison, TriggerScripts, CircularMap, Garrison, DisabledTemplates.

Checkboxes:

  • Display values of disabled checkboxes with Yes/No labels, fixes D2349, reviewed by nani.

Clean g_GameAttributes of invalid values and gamesetup GUI temporaries, refs #3049, #3883:

MapCache:

  • Refactor to MapCache class, store maps of all types and use it in the replaymenu, lobby and session as well.

SettingTabsPanel:

  • Remove hardcodings and coupling of the SettingTabsPanel with biomes/difficulties/chat UI from D1027/rP20945.

GamesetupPage.xml:

  • Restructure the page to use hierarchical object organization (topPanel, centerPanel, centerLeftPanel, bottomPanel, centerCenterPanel, centerRightPanel, bottomLeftPanel, bottomRightPanel), allowing to deduplicate object position margins and size math and ease navigation.

New defaults:

  • Check LockedTeams default in multiplayer (not only rated games).
  • Persist the rated game setting instead of defaulting to true when restarting a match, which often lead to unintentional rated games when rehosting.
  • 60 FPS in menus since they are animated

Autocomplete sorting fixed (playernames should be completed first).
Refactoring encompasses the one proposed in Polakrity and bb D1651.

Differential Revision: https://code.wildfiregames.com/D2483
Tested by: nani
Discussed with:

Emojis by: asterix, Imarok, fpre, nani, Krinkle, Stan, Angen, Freagarach

Note: See TracTickets for help on using tickets.