Dynamically generate lobby Map Options UI. #14359

pchote · 2017-11-14T17:45:35Z

This PR completes the work the work started with #11364 by dynamically generating the map options bin based on the selected map/mod rules.

Custom maps can remove/rearrange existing options by adjusting the *Visible and *DisplayOrder yaml fields, or add their own (see ScriptLobbyDropdown/#14325/#14245).

This also adds descriptive tooltips to the default options.

reaperrr · 2017-11-15T15:57:46Z

c744b13 is a bit out of my league, but the other commits and the in-game result look fine, so 👍

reaperrr · 2017-11-17T12:02:40Z

I think this should go into the playtest, moving to milestone.

abcdefg30

Changing from Exodus to Infiltration and changing the difficulty and speed dropdowns while the rules have not loaded leads to a crash (which I could not reproduce on bleed):

OpenRA engine version {DEV_VERSION}
Red Alert mod version {DEV_VERSION}
Date: 2017-11-19 00:09:19Z
Operating System: Windows (Microsoft Windows NT 6.2.9200.0)
Runtime Version: .NET CLR 4.0.30319.42000
Exception of type `System.InvalidOperationException`: Collection was modified. Enumeration operation may not execute. (Die Sammlung wurde geändert. Der Enumerationsvorgang kann möglicherweise nicht ausgeführt werden.)
   at System.ThrowHelper.ThrowInvalidOperationException(ExceptionResource resource)
   at System.Collections.Generic.List`1.Enumerator.MoveNextRare()
   at System.Collections.Generic.List`1.Enumerator.MoveNext()
   at OpenRA.Widgets.Widget.TickOuter() in \OpenRA\OpenRA.Game\Widgets\Widget.cs:Line 463.
   at OpenRA.Widgets.Ui.Tick() in \OpenRA\OpenRA.Game\Widgets\Widget.cs:Line 88.
   at OpenRA.Sync.<>c__DisplayClass5.<CheckSyncUnchanged>b__4() in \OpenRA\OpenRA.Game\Sync.cs:Line 166.
   at OpenRA.Sync.CheckSyncUnchanged[T](World world, Func`1 fn) in \OpenRA\OpenRA.Game\Sync.cs:Line 174.
   at OpenRA.Sync.CheckSyncUnchanged(World world, Action fn) in \OpenRA\OpenRA.Game\Sync.cs:Line 166.
   at OpenRA.Game.InnerLogicTick(OrderManager orderManager) in \OpenRA\OpenRA.Game\Game.cs:Line 566.
   at OpenRA.Game.LogicTick() in \OpenRA\OpenRA.Game\Game.cs:Line 633.
   at OpenRA.Game.Loop() in \OpenRA\OpenRA.Game\Game.cs:Line 763.
   at OpenRA.Game.Run() in \OpenRA\OpenRA.Game\Game.cs:Line 803.
   at OpenRA.Program.Run(String[] args) in \OpenRA\OpenRA.Game\Support\Program.cs:Line 136.
   at OpenRA.Program.Main(String[] args) in \OpenRA\OpenRA.Game\Support\Program.cs:Line 40.

abcdefg30 · 2017-11-18T23:58:35Z

OpenRA.Mods.Common/Widgets/Logic/Lobby/LobbyOptionsLogic.cs

+					row.Bounds.Y = optionsContainer.Bounds.Height;
+					optionsContainer.Bounds.Height += row.Bounds.Height;
+					foreach (var child in row.Children)
+						if (child is CheckboxWidget)


Any reason to use an is and a second cast instead of as and a null check here and below?

It saves having to define intermediate variables and adding a bunch of {}s.

abcdefg30 · 2017-11-18T23:59:25Z

mods/cnc/rules/player.yaml

@@ -9,9 +9,13 @@ Player:
 	PowerManager:
 	AllyRepair:
 	PlayerResources:
+		DefaultCashDisplayOrder: 0


Not sure if we want to restate the default. We tend to avoid that in other places.

pchote · 2017-11-19T01:00:03Z

Fixed.

MustaphaTR · 2017-11-19T09:38:16Z

OpenRA.Game/Traits/Player/PlayerResources.cs

@@ -37,7 +43,10 @@ IEnumerable<LobbyOption> ILobbyOptions.LobbyOptions(Ruleset rules)
 			var startingCash = SelectableCash.ToDictionary(c => c.ToString(), c => "$" + c.ToString());

 			if (startingCash.Any())
-				yield return new LobbyOption("startingcash", "Starting Cash", new ReadOnlyDictionary<string, string>(startingCash), DefaultCash.ToString(), DefaultCashLocked);
+				yield return new LobbyOption("startingcash", "Starting Cash", "Change the amount of cash that you start with",


Maybe move those descriptions to .yaml too. Not sure about others, but for crates at least it says random bonuses or penalties but some mods may have only bonuses for example. Or just wanna change the wording a bit.

PR updated

penev92 · 2017-12-04T02:15:08Z

All the random UI-related properties in random TraitInfos really don't sit well with me.
Can we have them as part of the ILobbyOptions interface and give them proper names like LobbyVisible/LobbyDescription/LobbyLocked/etc. ?
And since that's still not quite "proper", I'd actually prefer to see them gone from the respective traits and into a specialized one if possible (but too much of a scope creep for this PR).

penev92 · 2017-12-04T02:04:44Z

OpenRA.Game/Traits/Player/Shroud.cs

+		public readonly bool FogLocked = false;
+
+		[Desc("Display the fog checkbox in the lobby.")]
+		public readonly bool FogVisible = true;


This is going to be so confusing...

Have to agree here, can't we call those FogCheckboxVisible, ShroudCheckboxVisible and so on?

Not without renaming all the other fields related to these across all the traits that use them + associated upgrade rules. Considering that this PR has been sitting cold for three weeks, and that I have a big rework of the upgrade rules in the works, i'm not willing to do this myself. If this is a blocker for this PR, then please push an amendment over this if you want to address it yourself, or we can close it.

If you close this then please also revert the superstupid "hey-we're-overlooking-how-hacky-these-are-cuz-guy-is-newcomer" checkbox PRs.

If this is a blocker for this PR, then please push an amendment over this if you want to address it yourself, or we can close it.

It would have been nicer, but it's not a blocker to me, and Fog + Shroud are the only ambiguous cases anyway. My +1 still stands.

pchote · 2017-12-04T08:47:32Z

All the random UI-related properties in random TraitInfos really don't sit well with me.
Can we have them as part of the ILobbyOptions interface and give them proper names like LobbyVisible/LobbyDescription/LobbyLocked/etc. ?
And since that's still not quite "proper", I'd actually prefer to see them gone from the respective traits and into a specialized one if possible (but too much of a scope creep for this PR).

The whole point is to allow individual traits to control their visibility in the options bin via yaml. Making them part of the interface would break FieldLoader/yaml integration, and moving them to another trait would require a new set of plumbing to link them to the traits they control.

penev92

Everything seems to work, but I have a bunch of style nits.

P.S.: Eh, I thought I'd do the upgrade rules but ... just rename the bool properties that you're adding now, leave the old ones.

penev92 · 2017-12-07T18:59:46Z

OpenRA.Game/Traits/Player/PlayerResources.cs

+
+		[Translate]
+		[Desc("Description of the starting cash option in the lobby.")]
+		public readonly string Description = "Change the amount of cash that you start with";


... that PLAYERS start with.

penev92 · 2017-12-07T19:00:41Z

OpenRA.Game/Traits/Player/PlayerResources.cs

@@ -26,6 +34,12 @@ public class PlayerResourcesInfo : ITraitInfo, ILobbyOptions
 		[Desc("Force the DefaultCash option by disabling changes in the lobby.")]
 		public readonly bool DefaultCashLocked = false;

+		[Desc("Display the DefaultCash option in the lobby.")]
+		public readonly bool DefaultCashVisible = true;


IsDefaultCashVisible, please.

penev92 · 2017-12-07T19:01:35Z

OpenRA.Game/Traits/Player/PlayerResources.cs

@@ -26,6 +34,12 @@ public class PlayerResourcesInfo : ITraitInfo, ILobbyOptions
 		[Desc("Force the DefaultCash option by disabling changes in the lobby.")]
 		public readonly bool DefaultCashLocked = false;

+		[Desc("Display the DefaultCash option in the lobby.")]


Umm... "Whether to display", to sound a bit better?

penev92 · 2017-12-07T19:06:00Z

OpenRA.Game/Traits/Player/Shroud.cs

@@ -17,22 +17,52 @@ namespace OpenRA.Traits
 	[Desc("Required for shroud and fog visibility checks. Add this to the player actor.")]
 	public class ShroudInfo : ITraitInfo, ILobbyOptions
 	{
+		[Translate]
+		[Desc("Label for the fog checkbox in the lobby.")]


The one in PlayerResources starts with "Display name for ... ". Would be nice for consistency.

penev92 · 2017-12-07T19:16:07Z

OpenRA.Game/Traits/Player/Shroud.cs

 		[Desc("Default value of the fog checkbox in the lobby.")]
-		public bool FogEnabled = true;
+		public readonly bool FogEnabled = true;


IsFogEnabled, please.

penev92 · 2017-12-07T19:49:44Z

OpenRA.Mods.Common/Widgets/Logic/Lobby/LobbyOptionsLogic.cs

+
+			Widget row = null;
+			Queue<CheckboxWidget> checkboxColumns = new Queue<CheckboxWidget>();
+			Queue<DropDownButtonWidget> dropdownColumns = new Queue<DropDownButtonWidget>();


var for the two queues, please.

penev92 · 2017-12-07T19:57:07Z

OpenRA.Mods.Common/Widgets/Logic/Lobby/LobbyOptionsLogic.cs

+			{
+				if (!checkboxColumns.Any())
+				{
+					row = checkboxTemplate.Clone() as Widget;


as Widget is redundant.

penev92 · 2017-12-07T20:03:57Z

OpenRA.Mods.Common/Widgets/Logic/Lobby/LobbyOptionsLogic.cs

+			optionsContainer = widget.Get("LOBBY_OPTIONS");
+			optionsContainer.IsVisible = () => validOptions;
+			checkboxTemplate = optionsContainer.Get("CHECKBOX_ROW_TEMPLATE");
+			dropdownTemplate = optionsContainer.Get("DROPDOWN_ROW_TEMPLATE");


Please rename these to *RowTemplate for easier readability (I may be sleepy, but it took more than it should have to understand the below code).

penev92 · 2017-12-07T20:08:16Z

OpenRA.Mods.Common/Widgets/Logic/Lobby/LobbyOptionsLogic.cs

+				checkbox.GetText = () => option.Name;
+				if (option.Description != null)
+					checkbox.GetTooltipText = () => option.Description;
+				checkbox.IsVisible = () => true;


Missing empty line after if.

penev92 · 2017-12-07T20:08:52Z

OpenRA.Mods.Common/Widgets/Logic/Lobby/LobbyOptionsLogic.cs

+				checkbox.IsVisible = () => true;
+				checkbox.IsChecked = () => optionValue.Update(orderManager.LobbyInfo.GlobalSettings).Enabled;
+				checkbox.IsDisabled = () => configurationDisabled() ||
+					optionValue.Update(orderManager.LobbyInfo.GlobalSettings).Locked;


Spurious line break?

pchote · 2017-12-09T21:51:12Z

There have been major changes, so @reaperrr please reconfirm your 👍

pchote · 2017-12-11T19:20:43Z

Rebased.

Changes have been addressed.

pchote · 2017-12-12T20:24:40Z

Rebased.

abcdefg30

(PR was only rebased.)

… height. This also enables an extra row to be shown before triggering the scroll bar.

reaperrr · 2017-12-12T21:46:01Z

https://github.com/OpenRA/OpenRA/wiki/Changelog-(bleed)/_compare/de7c848c6ac0a75fe2c54187b614fd25ceebfaa0...58e43ccdfeada349efd6f41cd5413a1f4fba000d

pchote · 2017-12-13T19:37:59Z

I suspect that something went wrong with the rebase or was maybe just missed with the last round of testing. The ordering of the lobby options has regressed on all mods, and the debug menu is no longer enabled by default in TS.

pchote changed the title ~~Unhardcode lobby checkboxes~~ Dynamically generate lobby Map Options UI. Nov 14, 2017

pchote force-pushed the unhardcode-lobby-checkboxes branch 2 times, most recently from 25c2554 to c744b13 Compare November 14, 2017 17:58

pchote mentioned this pull request Nov 15, 2017

Implement bounty checkbox #14245

Closed

reaperrr previously approved these changes Nov 15, 2017

View reviewed changes

reaperrr added the PR: Needs +2 label Nov 15, 2017

pchote mentioned this pull request Nov 15, 2017

Add Kill Bounties checkbox to RA lobby options #14363

Merged

abcdefg30 added the PR: Is Dependency label Nov 15, 2017

reaperrr added this to the Next + 1 milestone Nov 17, 2017

abcdefg30 previously requested changes Nov 19, 2017

View reviewed changes

pchote added the PR: Changes requested label Nov 19, 2017

pchote dismissed reaperrr’s stale review via 7714264 November 19, 2017 00:59

pchote force-pushed the unhardcode-lobby-checkboxes branch from c744b13 to 7714264 Compare November 19, 2017 00:59

pchote removed the PR: Changes requested label Nov 19, 2017

MustaphaTR reviewed Nov 19, 2017

View reviewed changes

reaperrr modified the milestones: Next + 1, Next release Nov 19, 2017

reaperrr removed the PR: Needs +2 label Nov 22, 2017

reaperrr previously approved these changes Dec 2, 2017

View reviewed changes

reaperrr added the PR: Needs +2 label Dec 2, 2017

penev92 reviewed Dec 4, 2017

View reviewed changes

penev92 previously requested changes Dec 7, 2017

View reviewed changes

penev92 added the PR: Changes requested label Dec 7, 2017

pchote dismissed reaperrr’s stale review via 90f3197 December 9, 2017 20:29

pchote removed the PR: Needs +2 label Dec 9, 2017

pchote force-pushed the unhardcode-lobby-checkboxes branch from 5fe4829 to 994a8b5 Compare December 11, 2017 19:20

abcdefg30 previously approved these changes Dec 11, 2017

View reviewed changes

abcdefg30 added the PR: Needs +2 label Dec 11, 2017

pchote dismissed abcdefg30’s stale review via 5086756 December 12, 2017 20:23

pchote force-pushed the unhardcode-lobby-checkboxes branch 2 times, most recently from 5086756 to 35275fb Compare December 12, 2017 20:23

abcdefg30 previously approved these changes Dec 12, 2017

View reviewed changes

pchote added 9 commits December 12, 2017 21:27

Add additional metadata to lobby options.

66d64ec

Make DeveloperInfo fields readonly.

08b0472

Make ShroudInfo fields readonly.

9bb9b81

Make SpawnMPUnitsInfo fields readonly.

acb7d18

Move hardcoded list of lobby options into yaml.

e708308

Expose default UI labels and tooltips to yaml.

daacd0c

Add Checkbox/Dropdown to lobby options yaml fields.

0bafbec

Rename boolean LobbyOption fields.

ee6d5cc

Center the vertical alignment of lobby options if less than the panel…

c04e440

… height. This also enables an extra row to be shown before triggering the scroll bar.

pchote dismissed abcdefg30’s stale review via c04e440 December 12, 2017 21:30

pchote force-pushed the unhardcode-lobby-checkboxes branch from 35275fb to c04e440 Compare December 12, 2017 21:30

reaperrr approved these changes Dec 12, 2017

View reviewed changes

reaperrr merged commit 3ad6a87 into OpenRA:bleed Dec 12, 2017

pchote mentioned this pull request Dec 16, 2017

Fix incorrectly named lobby option properties. #14532

Merged

pchote mentioned this pull request Jan 2, 2018

Replace mod upgrade rules with a fit for purpose alternative. #14600

Closed

pchote deleted the unhardcode-lobby-checkboxes branch April 28, 2018 14:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dynamically generate lobby Map Options UI. #14359

Dynamically generate lobby Map Options UI. #14359

pchote commented Nov 14, 2017 •

edited

reaperrr commented Nov 15, 2017

reaperrr commented Nov 17, 2017

abcdefg30 left a comment •

edited

abcdefg30 Nov 18, 2017

pchote Nov 19, 2017 •

edited

abcdefg30 Nov 18, 2017

pchote commented Nov 19, 2017

MustaphaTR Nov 19, 2017

pchote Nov 19, 2017

penev92 commented Dec 4, 2017

penev92 Dec 4, 2017

reaperrr Dec 5, 2017

pchote Dec 5, 2017 •

edited

GraionDilach Dec 6, 2017

reaperrr Dec 6, 2017

pchote commented Dec 4, 2017 •

edited by phrohdoh

penev92 left a comment •

edited

penev92 Dec 7, 2017

penev92 Dec 7, 2017

penev92 Dec 7, 2017

penev92 Dec 7, 2017

penev92 Dec 7, 2017

penev92 Dec 7, 2017

penev92 Dec 7, 2017

penev92 Dec 7, 2017

penev92 Dec 7, 2017

penev92 Dec 7, 2017

pchote commented Dec 9, 2017

pchote commented Dec 11, 2017

pchote commented Dec 12, 2017

abcdefg30 left a comment

reaperrr commented Dec 12, 2017

pchote commented Dec 13, 2017 •

edited

Dynamically generate lobby Map Options UI. #14359

Dynamically generate lobby Map Options UI. #14359

Conversation

pchote commented Nov 14, 2017 • edited

reaperrr commented Nov 15, 2017

reaperrr commented Nov 17, 2017

abcdefg30 left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pchote Nov 19, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pchote commented Nov 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

penev92 commented Dec 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pchote Dec 5, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pchote commented Dec 4, 2017 • edited by phrohdoh

penev92 left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pchote commented Dec 9, 2017

pchote commented Dec 11, 2017

pchote commented Dec 12, 2017

abcdefg30 left a comment

Choose a reason for hiding this comment

reaperrr commented Dec 12, 2017

pchote commented Dec 13, 2017 • edited

pchote commented Nov 14, 2017 •

edited

abcdefg30 left a comment •

edited

pchote Nov 19, 2017 •

edited

pchote Dec 5, 2017 •

edited

pchote commented Dec 4, 2017 •

edited by phrohdoh

penev92 left a comment •

edited

pchote commented Dec 13, 2017 •

edited