Skip to content
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

Expose player ratings and divisions to the session #6569

Merged
merged 7 commits into from
Dec 10, 2024

Conversation

Garanas
Copy link
Member

@Garanas Garanas commented Nov 30, 2024

Description of the proposed changes

Extends #6479 by implementing the details of #6160. I'm not sure how I forgot about this - the rating and divisions are also not part of the checklist.

Testing done on the proposed changes

Extended the launch script made by @lL1l1 to include (sub)divisions. Launch the game via the script. Observe the divisions in the scoreboard.

image

And similar for clan tags with 8e2b159:

image

Additional context

Discussion on Discord whether we should show this data by default to players.

Checklist

  • Changes are annotated, including comments where useful
  • Changes are documented in the changelog for the next game version

@Garanas Garanas added type: bug area: matchmaker/autolobby related to the matchmaker lobby labels Nov 30, 2024
@Garanas Garanas marked this pull request as ready for review November 30, 2024 16:23
Copy link
Contributor

@lL1l1 lL1l1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Technically divisions aren't needed in the Sim, GetFrontEndData and SetFrontEndData would be the correct choice, if we ignore being consistent with ratings and clan tags.
  • The /clan arg also needs to be passed to the UI of other players for clan tags to work. It goes into the gameInfo.GameOptions.ClanTags field
    gameInfo.GameOptions['ClanTags'] = clanTags

    Unfortunately we can't use front end data due to backwards compatibility unless you add a meta table to SessionGetScenarioInfo().Options.ClanTags with
__index = function(i) 
  return GetFrontEndData("ClanTags")[i] 
end

or simply duplicate the code, if it's not being used often anyway (we will only ever have 2 lobbies).

  • The lobby does some safeguards for args in case the arg is an empty string and causes the next arg to be grabbed instead of the value (since an empty string is not a valid token for the arg getter).

    fa/lua/ui/lobby/lobby.lua

    Lines 159 to 181 in 6208f25

    -- Set of all possible command line option keys.
    -- The client sometimes gives us empty-string as some args, which gets interpreted as that key
    -- having as value the name of the next key. This set lets us interpret that case using the
    -- default option.
    local CMDLINE_ARGUMENT_KEYS = {
    ["/init"] = true,
    ["/country"] = true,
    ["/numgames"] = true,
    ["/mean"] = true,
    ["/clan"] = true,
    ["/deviation"] = true,
    ["/joincustom"] = true,
    ["/gpgnet"] = true,
    }
    local function GetCommandLineArgOrDefault(argname, default)
    local arg = GetCommandLineArg(argname, 1)
    if arg and not CMDLINE_ARGUMENT_KEYS[arg[1]] then
    return arg[1]
    end
    return default
    end

    This safeguarding is similar to what I did for GetCommandLineArgTable more recently:

    fa/lua/system/utils.lua

    Lines 744 to 769 in b872a71

    -- Return a table parsed from key:value pairs passed on the command line
    -- Example:
    -- command line args: /arg key1:value1 key2:value2
    -- GetCommandLineArgTable("/arg") -> {key1="value1", key2="value2"}
    function GetCommandLineArgTable(option)
    -- Find total number of args
    local nextMax = 1
    local args, nextArgs = nil, nil
    repeat
    nextArgs, args = GetCommandLineArg(option, nextMax), nextArgs
    nextMax = nextMax + 1
    -- GetCommandLineArg tokenizes without being limited by `/`, so we need to stop manually
    until not nextArgs or nextArgs[nextMax - 1]:sub(0,1) == "/"
    -- Construct result table
    local result = {}
    if args then
    for _, arg in args do
    local pair = StringSplit(arg, ":")
    local name, value = pair[1], pair[2]
    result[name] = value
    end
    end
    return result
    end

@Garanas
Copy link
Member Author

Garanas commented Dec 2, 2024

  • Technically divisions aren't needed in the Sim, GetFrontEndData and SetFrontEndData would be the correct choice, if we ignore being consistent with ratings and clan tags.

I wonder if these show up in the replay somewhere.

@lL1l1
Copy link
Contributor

lL1l1 commented Dec 5, 2024

Mentioning replays, it's best to put it in the sim so that replays have a copy of clan tags/divisions/rating through the scenario info table.

@BlackYps
Copy link
Contributor

BlackYps commented Dec 5, 2024

They should be handled in the same way as ratings and clans. They should all not be lost when watching the replay.

@Garanas
Copy link
Member Author

Garanas commented Dec 5, 2024

@lL1l1 is 1c6384b roughly the right direction for proper command line parsing?

@Garanas
Copy link
Member Author

Garanas commented Dec 6, 2024

@BlackYps it now also supports clan tags. I can't test this via the test server with just myself, maybe we can test it together before you do a release?

Copy link
Contributor

@lL1l1 lL1l1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think all that is necessary is some "get arg or default" function:

GetCommandLineArgOrDefault = function(option, default)
  local arg = GetCommandLineArg(option, 1)[1]
  if not arg or string.sub(arg, 0, 1) == "/" then
    return default
  else
    return arg
  end
end

This guards against the empty string (fromt client) arg case with the least complexity.

Comment on lines 83 to 84
local arguments = GetCommandLineArg(option, 1)
if arguments and (not option[ arg[1] ]) then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not check string.sub(arg[1], 0, 1) == "/"?

  • It's much simpler than keeping a list of all valid command line arguments (which get preceded and effectively overwritten by the client's own cmdline args anyway).

---@param option string
---@param default number
---@return number
GetCommandLineArgumentNumber = function(self, option, default)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if verifying as thoroughly as checking types is necessary, it just adds to the load of thinking which function is best to use without improving the solution to the original issue: The guards in lobby.lua were implemented to prevent the case of the client giving a command line argument /arg X /arg2 ... where X is an empty string, and GetCommandLineArg(option, 1) would end up returning {"/arg2"} because an empty string is not a token.

Comment on lines +120 to +123
GetCommandLineArgumentArray = function(self, option)
if not self:ValidCommandLineKey(option) then
return {}
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the utils function will return an empty table for a non-existent key, so if we don't want to verify if a requested command line key is valid, then this function is not necessary as the default empty table return value is already implemented.

Comment on lines +531 to +532
-- tuck them into the game options. By all means a hack, but
-- this way they are available in both the sim and the UI
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
-- tuck them into the game options. By all means a hack, but
-- this way they are available in both the sim and the UI
-- tuck them into the game options. By all means a hack, but
-- this way they are available in both the sim (for replays) and the UI

lL1l1 added 2 commits December 8, 2024 18:07
`arg` instad of `arguments`
previous commit was clan tags for the launch script
Copy link
Contributor

@lL1l1 lL1l1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on test server with accounts [TEC] clan KAZAKHNYA and Straiken and the fix works: clans, ratings, and divisions appear in the game options for both the live game and in the replay file. Although the style could be better, this PR is due for a hotfix and it is functional, so I'm approving it.

UI_Lua print(repr(SessionGetScenarioInfo().Options))

@BlackYps BlackYps merged commit 74a5b7d into develop Dec 10, 2024
5 checks passed
@BlackYps BlackYps deleted the Fix/autolobby-division-rating branch December 10, 2024 00:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: matchmaker/autolobby related to the matchmaker lobby type: bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants