Opened 8 years ago

Last modified 23 months ago

#3987 new defect

Implement options to randomly choose from a subset of civilisations during gamesetup

Reported by: elexis Owned by:
Priority: Should Have Milestone: Backlog
Component: UI – Game setup Keywords:
Cc: O.Davoodi, s0600204, Krinkle Patch:

Description (last modified by marder)

Since r12704, the current gamesetup code in launchGame() first picks a random culture (like hellenic or celtic) and then picks a random civ of that culture.

That however means that some civs are over/underrepresented. For example the chance to get Ptolemies is three times bigger than the chance to get Athens (as there are three hellenic civs).

Here the cultures of the available civs (binaries/data/mods/public/simulation/data/civs):

spart.json:	"Culture":"hele",
mace.json:	"Culture":"hele",
athen.json:	"Culture":"hele",
gaul.json:	"Culture": "celt",
brit.json:	"Culture": "celt",
rome.json:	"Culture": "rome",
sele.json:	"Culture":"sele",
ptol.json:	"Culture":"ptol",
maur.json:	"Culture": "maur",
pers.json:	"Culture": "pers",
cart.json:	"Culture": "cart",
iber.json:	"Culture": "iber",

In my humble opinion, this should be reverted, so that all civs have the same chance of appearing. (Just because they share some of the same buildings and stats doesn't mean they should be picked less often.)

While the original ticket has been fixed in https://trac.wildfiregames.com/changeset/26954

it would still be nice to have advanced options to choose randomly from a specified subset of civilizations.

As this ticket contains quite some discussion and information, I'm leaving it open for the future.

Attachments (8)

trulyrandomcivs.js.patch (1.2 KB ) - added by Sandarac 8 years ago.
random_cultures_option.patch (4.2 KB ) - added by Sandarac 8 years ago.
Adds check box in More Options
civselection_and_randomcivoption.png (259.5 KB ) - added by Sandarac 8 years ago.
I have updated s0600204's civ selection menu and added the Truly Random Civs option to it, is this what is wanted?
civselection_randomcivoption.patch (54.1 KB ) - added by Sandarac 8 years ago.
All credit goes to s0600204 for the civ selection menu. If the user has Random Cultures enabled (default), then "Random All" will select random civs from random cultures, otherwise truly random.
civselection_randomcivoptionWIP.patch (57.6 KB ) - added by Sandarac 8 years ago.
Fixes issues pointed out by mimo, and some other issues, but the code could be cleaner. Having the civ/grouping name displayed on the right panel and as a tooltip should be enough. Adds double click functionality for selecting civs/groupings.
civselection_randomcivoptionWIP_v2.patch (56.0 KB ) - added by Sandarac 8 years ago.
More Fixes.
civselection_randomcivoptionWIP_v3.patch (60.8 KB ) - added by Sandarac 8 years ago.
Still needs fixes and code quality improvements.
civselection_randomcivoption_v3.1.patch (61.8 KB ) - added by Sandarac 8 years ago.

Download all attachments as: .zip

Change History (31)

by Sandarac, 8 years ago

Attachment: trulyrandomcivs.js.patch added

comment:1 by Sandarac, 8 years ago

Keywords: review patch added
Milestone: BacklogAlpha 21
Summary: Chosing a random civilization should mean that all civs have the same chance of being picked[PATCH] Chosing a random civilization should mean that all civs have the same chance of being picked

comment:2 by elexis, 8 years ago

  • Remove those braces in L1255 L1260 as they are not needed anymore.
  • The comments are not really useful anymore in L1246 and L1253, so remove them too
  • Don't forget to add yourself to programming.json

comment:3 by mimo, 8 years ago

Is this patch really something we want ? i prefer the current way which gives more variety to games with a reduced chance to have opponents with buildings with the same look. Maybe a solution to please everyone would be to add a player option to choose between the current way (based on culture, which i think should be the default) and the fully random one.

by Sandarac, 8 years ago

Adds check box in More Options

comment:4 by fatherbushido, 8 years ago

From the formal point of view : L1267 L1269, you can/must remove the { }. Moreover, you can just remove the else part and just put L1264 after the }

comment:5 by elexis, 8 years ago

But we also need a clear design decision on whether we should go with the first, second or original approach before you do any further work that might become rejected.

As stated above, mimo prefers the culture distinction, while I'd prefer to give all civs the same chance and not complicate this (Would players actually understand what that option is used for? Wouldn't we spam the options area a bit?). I wonder where we should get a clear decision from, since most developers are busy / not-available right now and those that are disagree or don't have a strong opinion x) So I wonder whether we should make a forum thread about this and ask the actual players what they prefer.

Here you can read the chatlog (to 17:23 to 18:34): http://irclogs.wildfiregames.com/2016-05-23-QuakeNet-%230ad-dev.log

Of course you are always welcome to join #0ad-dev for asking us anything directly: http://irc.lc/quakenet/0ad-dev/

Thanks for your patience.

comment:6 by mimo, 8 years ago

To answer elexis remarks first, i think that for players to understand what all this means, it is only up to us to give the right tooltips. And i believe the final game should be much more customizable than it is now, with a more complete option panel to adapt it to the player taste. But i agree with him to advice you to drop by on IRC to discuss it a bit before doing more work on this ticket.

Then why is the culture choice relevant: we already have 3 hele civs which share most of the buildings, and if you use a mod like delenda, it even has 5 such hele civs. Of course, a core player who has spent time to compare the different unit stats will see that they play a bit differently, but a casual player who cares more about the immersion and the look rather than the unit stats could have the impression to always play the same civ. So having a random culture by default for new players looks much better to me. Then the core players can change this option to random civ once they know the game better (although after quite a long time, I still prefer a random culture).

Finaly going back to the patch, I think this option should be made more persistent, as it is something a player will rarely/never change. We should rather add it in binaries/data/config/default.cfg with a dropdown in gui/options/options.json. If doing that, we could regroup all options affecting gamesettings: persistmatchsettings could be changed to something like gamesettings.persistsettings and we could add gamesettings.civchoice

comment:7 by elexis, 8 years ago

That sounds fine to me (especially not having it in the gamesetup but in the settings). Thanks for elaborating :)

comment:8 by elexis, 8 years ago

Cc: s0600204 added
Keywords: simple review removed

As noticed by leper, this could be solved on a broader scale. The host could select the civilizations that the randomization would chose from.

A checkbox would be added to that dialog in order to please both preferences with regards to culture-based randomization.

There is a constructionsite for alpha 18 by s0600204 doing something like that: https://wildfiregames.com/forum/index.php?/topic/19724-design-civillization-name/#comment-306957 https://github.com/s0600204/0ad-civselection-mod http://i.imgur.com/aVTPEuv.png

The new dialog would be the right place to move the disputed checkbox.

Taking out of the review queue as the more-options window is already filled with many options and it's likely an option which will be used rarely. A general option (main menu -> settings) should be considered a temporary solution in case noone works on the real deal.

comment:9 by Sandarac, 8 years ago

Keywords: review added

by Sandarac, 8 years ago

I have updated s0600204's civ selection menu and added the Truly Random Civs option to it, is this what is wanted?

comment:10 by elexis, 8 years ago

That certainly looks interesting, but to judge we need the patch (and maybe s0600204 wants to update his repository too if we don't accept it for any reason).

comment:11 by s0600204, 8 years ago

I'll have a look at a patch if one's provided, but I'm not merging it into that repo. For the simple reason that I migrated the mod to a branch on my fork of 0AD on GitHub months ago (https://github.com/s0600204/0ad/tree/civselect) for ease of maintenance and development.

Also, if you hadn't already noticed, please note that the civselect repo/branch alters the Culture attribute for all civs, so as to aid grouping on the page. Thus, the civ-to-culture list in the description of this ticket is not true if you have the civselect mod/branch active.

by Sandarac, 8 years ago

All credit goes to s0600204 for the civ selection menu. If the user has Random Cultures enabled (default), then "Random All" will select random civs from random cultures, otherwise truly random.

comment:12 by mimo, 8 years ago

Thanks for the new patch. It seems to me that it has a lot of potentials, but also is not easy to use. Some additional tooltips or a better disposition of the controls could help.

  • I would have expected that the civ tooltip (displayed on the right) is also visible (as a true tooltip maybe to differentiate it from the selected civ) on mouseOver and not only onClick (having to click on each icon is a pain).
  • In addition, as new players are not used to the civ icon and do not recognize them, having the name written below the icon would help.
  • why is the "select all" checkbox only visible when "ungrouped" ?.And the position of the different checkboxes is not clear. I would have expected that the "random cultures" is dependent on the "select all" one (i.e. disabled when the select all is not on), and grouped together and not as now on different lines and columns which give the impression they have nothing to do together).
  • if i select the european civs for example (in the group by geography) and then switch to ungrouped or group by culture views, i would have expected the european civs to be shown as selected

by Sandarac, 8 years ago

Fixes issues pointed out by mimo, and some other issues, but the code could be cleaner. Having the civ/grouping name displayed on the right panel and as a tooltip should be enough. Adds double click functionality for selecting civs/groupings.

by Sandarac, 8 years ago

More Fixes.

comment:13 by wraitii, 8 years ago

Some comments on what the patch should do: -You need to change the reset icon -Clicking on the gear is more difficult than clicking on the dropdown and this is annoying. I'd suggest making the whole thing a button.

-I really like the sorting by culture, but there's obviously quite a bit of wasted space. Would be nice to improve that view and make it default imo (see attached screenshot).

-Clicking on several icons using shift + click should select both and make the choice random between those. -Drag and dropping should probably select what's inside. -Clicking on the currently selected civ should imo deselect it (thus reselecting everything) instead of this very clumsy "select all". -The "group by culture" button is very impractical. My screenshot solution would allow doing it differently.

A code comment: -not a fan of your culture naming convention.

comment:14 by wraitii, 8 years ago

http://imgur.com/a/UlmDl

Imgur album with screenshots of what I suggest. Here I grouped by culture, but could switch to region. My suggestion is that you can select a group (which would show something like my big successor badge here) or specifically a civilization in a group (hovering over the group should fade it somewhat and allow choosing). Clicking anywhere on the group but not the civ selects the group.

You can then mix and match however you want and the screen remains rather nice and uncluttered.

nb those are photoshop mockups.

comment:15 by Itms, 8 years ago

Keywords: review removed

This has been reviewed by wraitii and I agree with all his comments. Additionally there is a glitch: I can't untoggle the "Select all" radio button.

I like the idea though, thanks for your work!

For you next submission, please take a look at SubmittingPatches, since we are changing some things in our review process.

by Sandarac, 8 years ago

Still needs fixes and code quality improvements.

comment:16 by elexis, 8 years ago

Hey Sandarac,

since this is huge patch and the ones reviewing certainly don't want to reread the entire patch every revision, you should start using github for that. Just create an account over there, open https://github.com/0ad/0ad/ and click on fork, create a branch for this ticket, clone your fork to your harddrive, checkout that branch, apply the patch, commit it and push it upstream. Then you can do incremental commits and if you need to fix some typos, one only has to read those lines you changed.

comment:17 by Sandarac, 8 years ago

Milestone: Alpha 21Backlog

comment:18 by Imarok, 5 years ago

Component: UI & SimulationGame setup

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

comment:19 by Krinkle, 5 years ago

Cc: Krinkle added
Description: modified (diff)

comment:20 by elexis, 4 years ago

An updated version of the civselection dialog is found here https://github.com/JustusAvramenko/delenda_est/commit/bd1e73bf133674e790aa9f686f4b41e2ccba0a43

Screenshots at https://wildfiregames.com/forum/index.php?/topic/25206-civilization-selection-screen/

For the problem originally described in the ticket: The easiest solution to please everyone could be to not have ["Random", "Civ1", ... , "CivN"] choices for the dropdown, but ["Random Civ", "Random civ of random Culture", "Civ1", ... , "CivN"]. And to options.json one could add a config option that determines which of these N+2 items is chosen by default if the player can control over their own slot (refs #3806). So players can chose romans by default, or random civ, or random civ of a random culture. This could be integrated with the civ dialog as well (providing the same N+2 choices, but with images and text).

comment:21 by elexis, 4 years ago

In 23413:

Gamesetup subpages support.

Split SetupWindow from GameSetupPage class.
Create Gamesetup/Pages subfolder, starting with the existing GameSetupPage and LoadingPage.
Run the subpage in the same GUI script context instead of separate pages.
Planned subpages are AIConfigPage (refs D2577), MapBrowser (D1703), hero selection dialog, civ selection (refs #3987), starting resources, population capacity (refs #4379, #812...).

This allows:

  • subpages to benefit from direct access and reuse of the gamesetup controlers, (f.e. gameSettingsControl, playerAssignmentsControl, mapCache, netMessages, ...), deduplication.
  • subpages to handle events and control gamesettings even if the according subpage is closed (for example deleting AIBehavior if there is no AI assigned).
  • to keep gamesettings decoupled, i.e. exactly one class per setting (for this example avoiding that both GameSetupPage/AIConfigButton and AIConfigPage/AIBehavior control the AIBehavior setting value.).

Differential Revision: https://code.wildfiregames.com/D2581
Comments By: Vladislav (http://irclogs.wildfiregames.com/2020-01/2020-01-18-QuakeNet-%230ad-dev.log), nani (PM)

comment:22 by marder, 23 months ago

In 26954:

Choose civ randomly in gamesetup

Refs: #3987

accepted by: @Stan

Differential revision: https://code.wildfiregames.com/D4703

comment:23 by marder, 23 months ago

Description: modified (diff)
Keywords: patch removed
Summary: [PATCH] Chosing a random civilization should mean that all civs have the same chance of being pickedImplement options to randomly choose from a subset of civilisations during gamesetup
Note: See TracTickets for help on using tickets.