-
-
Notifications
You must be signed in to change notification settings - Fork 114
Broadcast and filter game options #874
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
Fix issue with lblSkillLevel visibility Add layout constants Co-authored-by: devo1929 <devo1929@users.noreply.github.com>
Add extra param to CreateDivider to specify a custom height Add constant for spacing in icon panel
Add game options to the filters in CnCNetLobby Add broadcasted game options to the GameListBox and GameInformationPanel Adds setting to disable the game icon from the GameListBox
|
Nightly build for this pull request:
|
Metadorius
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work so far mate!
I don't see if credits are shown in the game info panel, is this intended? I thought if you're able to filter by it - it should also be visible there.
Also maybe the panel should be made two-column and use the same font as the main game info?
Also I think the icons here should be item icons, because so far per the idea icons convey the value, not what option it is. See my review comments on how to make this work.
Some of the comments that I left are more of open questions, so curious to hear your thoughts on them.
| /// <summary> | ||
| /// Gets the filter value for a checkbox game option. | ||
| /// Returns 0 for "All", 1 for "On", 2 for "Off". | ||
| /// </summary> | ||
| public int GetCheckboxFilterValue(string optionName) => SettingsIni.GetIntValue(GAME_OPTION_FILTERS, optionName, 0); | ||
|
|
||
| /// <summary> | ||
| /// Sets the filter value for a checkbox game option. | ||
| /// 0 = "All", 1 = "On", 2 = "Off". | ||
| /// </summary> | ||
| public void SetCheckboxFilterValue(string optionName, int value) => SettingsIni.SetIntValue(GAME_OPTION_FILTERS, optionName, value); | ||
|
|
||
| /// <summary> | ||
| /// Gets the filter value for a dropdown game option. | ||
| /// Returns -1 for "All", or the selected index. | ||
| /// </summary> | ||
| public int GetDropdownFilterValue(string optionName) => SettingsIni.GetIntValue(GAME_OPTION_FILTERS, optionName, -1); | ||
|
|
||
| /// <summary> | ||
| /// Sets the filter value for a dropdown game option. | ||
| /// -1 = "All", or the selected index. | ||
| /// </summary> | ||
| public void SetDropdownFilterValue(string optionName, int value) => SettingsIni.SetIntValue(GAME_OPTION_FILTERS, optionName, value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- This is a bit inconsistent, I think -1 should mean "irrelevant" in both the dropdown and the checkbox.
- I am not sure yet, but to me
int?andbool?sounds like a more obvious choice here. Curious to hear what you think about that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True - nullable is better. Fixed in dda5b0c.
| /// Whether the icon should be shown on the right side of the game list. | ||
| /// Only applies if IconShownInGameList is true. | ||
| /// </summary> | ||
| public bool IconShownInGameListOnRight { get; private set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clever customisation!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Perhaps it would be better to have both "ShownInGameListLHS" and "ShownInGameListRHS". What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure, is there a use for having both?
| /// <summary> | ||
| /// The texture name for the icon. | ||
| /// </summary> | ||
| public string Icon { get; private set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for this to be more useful, we have to have not one icon per dropdown but actually an icon per option in the dropdown. If the icon is missing -- then no icon would be shown.
This would also streamline the behavior between the checkbox and the dropdown. The icon could be then drawn as ItemTexture or however the option was called.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks and fixed.
| sb.Append(";"); // Packed checkbox values (empty for loaded lobbies) | ||
| sb.Append(";"); // Dropdown indices (empty for loaded lobbies) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it necessary to separate dropdowns and indices like this? They are essentially both the same, checkbox is an option with 0, 1 states, and dropdown has 0, 1, 2, ... etc. I've created an interface for game settings specifically to generalize this kind of behavior. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed. That stuff was mostly copied from the existing game options broadcast which had it split. See:
| // We don't gain much in most cases by packing the drop-down values |
RE the interface - makes sense. I was working on an outdated copy and didn't see those changes. Hopefully 60b23c5 sorts it as you imagined.
| var broadcastableCheckboxes = gameLobby.CheckBoxes.Where(cb => cb.BroadcastToLobby).ToList(); | ||
| if (game.BroadcastedCheckboxValues != null && broadcastableCheckboxes.Count > 0) | ||
| { | ||
| for (int i = 0; i < broadcastableCheckboxes.Count; i++) | ||
| { | ||
| if (i >= game.BroadcastedCheckboxValues.Length) | ||
| break; | ||
|
|
||
| var checkbox = broadcastableCheckboxes[i]; | ||
| int filterValue = UserINISettings.Instance.GetCheckboxFilterValue(checkbox.Name); | ||
|
|
||
| // 0 = All, 1 = On, 2 = Off | ||
| if (filterValue == 0) | ||
| continue; | ||
|
|
||
| bool isChecked = game.BroadcastedCheckboxValues[i]; | ||
|
|
||
| bool shouldBeChecked = filterValue == 1; | ||
| if (isChecked != shouldBeChecked) | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| // dropdown filters | ||
| var broadcastableDropdowns = gameLobby.DropDowns.Where(dd => dd.BroadcastToLobby).ToList(); | ||
| if (game.BroadcastedDropdownIndices != null && broadcastableDropdowns.Count > 0) | ||
| { | ||
| for (int i = 0; i < broadcastableDropdowns.Count; i++) | ||
| { | ||
| if (i >= game.BroadcastedDropdownIndices.Length) | ||
| break; | ||
|
|
||
| var dropdown = broadcastableDropdowns[i]; | ||
| int filterValue = UserINISettings.Instance.GetDropdownFilterValue(dropdown.Name); | ||
|
|
||
| // -1 = All, otherwise check for exact match | ||
| if (filterValue == -1) | ||
| continue; | ||
|
|
||
| int gameSelectedIndex = game.BroadcastedDropdownIndices[i]; | ||
|
|
||
| if (gameSelectedIndex != filterValue) | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this could be deduplicated I think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
60b23c5 sorts it.
| Texture2D icon = AssetLoader.LoadTexture(iconName); | ||
| if (icon != null) | ||
| { | ||
| string text = $"{checkbox.Text}: {(isChecked ? "On".L10N("Client:Main:On") : "Off".L10N("Client:Main:Off"))}"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we use Yes/No already in other places? not sure, need to check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think on/off is more appropriate as we're displaying the status of an option not answering a yes/no question. We do use yes/no throughout the ini files though.
Happy to change it if you want, I have no strong preference either way.
SW: On
Crates: Off
SW: Yes
Crates: No
| } | ||
| else | ||
| { | ||
| // For dropdowns: -1 = All (default), actual indices are offset by 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what is the reason for that math here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorted with 60b23c5.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there are many options you could use the ScrollablePanel I implemented. How does it fare with a lot of options? Say, every option added to broadcast.
Wonder if there's a point in making this all INItializable...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, it is using the scroll panel now. There is an annoying issue where scrolling with the mousewheel, if the cursor is over a dropdown, the dropdown scrolls too.
| /// <summary> | ||
| /// Whether the icon should be shown in the game information panel. | ||
| /// </summary> | ||
| public bool IconShownInGameInfo { get; private set; } | ||
|
|
||
| /// <summary> | ||
| /// Whether the icon should be shown in the game filters panel. | ||
| /// </summary> | ||
| public bool IconShownInFilters { get; private set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I know I said previously that it would be good to have it customisable, but I am not sure now. Do you think there would be any case when the modder or game maintainer would want to have an icon in game list but not have the icon anywhere else? ATM I am not sure if this makes sense, because the game info/filters icon work as legend, so probably could omit that.
Also I think we could draw the icons near game options in the lobby themselves the same way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think we keep the settings but make it apply to the whole thing not just the icon. So "ShownInGameInfo" instead of "IconShownInGameInfo", etc. It's programmed a bit like that already - hence the missing Credits in that screenshot.
I was having hassles with icons in the game lobby. I'll take another look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the filter and the game info should be switched on by the same option, otherwise it would be pretty user unfriendly to have something in filter but not in game info, and vice versa. Same for icons in the list - if the icon is shown in the game list - it must have the same icon (working as legend) in filter and in game info, so the users don't get confused on the meaning.
| int rightIconsWidth = rightIcons.Count > 0 ? | ||
| (rightIcons.Sum(icon => icon.Width) + (rightIcons.Count * ICON_MARGIN)) : 0; | ||
|
|
||
| int gameTextureWidth = ClientConfiguration.Instance.ShowGameIconInGameList ? hg.Game.Texture.Width : 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do think that if LocalGame is different from the one the game has, the texture should be always on, as we would have no way of easily telling what game is that game room for.
Add 2 column drawing in GameInformationPanel Add SortOrder property Add icons to dropdowns Rename settings Use scrollpanel
| /// Sort order for displaying icons in the GameInformationPanel and GameListBox. | ||
| /// Lower values appear first. | ||
| /// </summary> | ||
| public int SortOrder { get; private set; } = 100; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why this is 100 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So people can easily specify a lower or higher control? Seems like a common way, like the default metric for a network interface.
Mabybe we can make this 100 a constant but 100 is definitely better than a default 0 (unless we want to allow negative numbers, then 0 will be a perfect default value) or int.Max
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see why would we disallow negative values
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed it to 0 as I'm not blocking negatives so I guess it makes more sense. Setting it to 100 just read better to me.
SadPencil
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Only some minor comments
| // First show icons in a row | ||
| if (optionIconsOnly.Count > 0) | ||
| { | ||
| var sortedIconsOnly = optionIconsOnly.OrderBy(x => x.sortOrder).ToList(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick. The List<T>.Sort method sorts the list in-place and has a higher efficiency than an .OrderBy.ToList().
(It's a pity that PriorityQueue is only provided in .NET 6+ but sorting a list here is totally acceptable)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks SP. Let's leave it as is for now. It's not sorting much and would require a bit more code otherwise.
| currentY += maxIconHeight + legendPadding; | ||
| } | ||
|
|
||
| // Then show icons with text ( two columns) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
( two columns) -> (two columns)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed thanks.
| pnlGameOptions.AddChild(iconPanel); | ||
|
|
||
| if (isLeftColumn) | ||
| leftY += legendIconHeight + 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have defined const int iconSpacing = 4; private const int legendPadding = 5; but here we got another magic number 2. You might want to define another constant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, have added a constant.
| { | ||
| var section = SettingsIni.GetSection(GAME_OPTION_FILTERS); | ||
| if (section != null) | ||
| section.RemoveAllKeys(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
section?.RemoveAllKeys();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, thanks.
| /// Sort order for displaying icons in the GameInformationPanel and GameListBox. | ||
| /// Lower values appear first. | ||
| /// </summary> | ||
| public int SortOrder { get; private set; } = 100; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So people can easily specify a lower or higher control? Seems like a common way, like the default metric for a network interface.
Mabybe we can make this 100 a constant but 100 is definitely better than a default 0 (unless we want to allow negative numbers, then 0 will be a perfect default value) or int.Max
|
|
||
| // Convert bytes to integers | ||
| for (int i = 0; i < byteArray.Length / 4; i++) | ||
| gameOptionValues.Add(BitConverter.ToInt32(byteArray, i * 4)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as the V3 PR: be careful using BitConverter. You can throw if BitConverter.IsLittleEndian does not match our expect, or use BinaryPrimitives instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update: looks like we already have ~20 BitConverter usages. Maybe we should change them to BinaryPrimitives or globally throw on unexpected BitConverter.IsLittleEndian in another PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK - let's do this in another PR.
| int textHeight = (int)textSize.Y; | ||
|
|
||
| int iconY = (textHeight - icon.Height) / 2; | ||
| if (iconY < 0) iconY = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our style requires a line break after if (condition)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorted, thanks.
Closes or helps close
New INI settings for GameSessionCheckBox (located in
GameLobbyBase.ini):New settings for GameSessionDropDown (located in GameLobbyBase.ini):
New setting in
ClientDefinitions.ini:GameListBox and GameInformationPanel changes:

Filters menu:
