Improve lobby restrictions #303
Conversation
|
|
||
| rating_type = | ||
| cond do | ||
| team_count > 2 && team_size == 1 -> "FFA" |
There was a problem hiding this comment.
Needs to change when Lexon does big/small team split
There was a problem hiding this comment.
I would leave that as a comment in the code itself.
You could also leave that in the commit message, but for this, you would need a much smaller commit doing pretty much only this action.
There was a problem hiding this comment.
There are a few issues with formatting, I'd suggest you setup elixir lsp with the code formatter. The codebase already largely conform to that.
I don't know how much effort that would be, but I'd like some integration test on that.
If that proves to be way too much effort, I'm not against merging it with only some manual testing, but it would be good to start new features with correct tests.
I'm thinking either sending spring commands, or "just" creating players and a match as processes and using the API you defined + some assertion.
| @doc """ | ||
| Returns {:ok, nil} or {:error,msg} | ||
| """ | ||
| def check_rank_to_play(user, consul_state) do |
There was a problem hiding this comment.
Prefer using a typespec if you document the return type.
@spec check_rank_to_play(any(), any()) :: {:ok, nil} | {:error, String.t()}
Also, this is janky, {:ok, nil} should just be :ok, the nil is useless here.
| @doc """ | ||
| Returns {:ok, nil} or {:error,msg} | ||
| """ | ||
| def check_rating_to_play(user_id, consul_state) do |
There was a problem hiding this comment.
see comment above regarding signature.
|
|
||
| def get_rating_bounds_for_title(consul_state) do | ||
| default_state = %{maximum_rating_to_play: @rating_upper_bound, minimum_rating_to_play: 0} | ||
| consul_state = Map.merge(default_state, consul_state) |
There was a problem hiding this comment.
You shouldn't adjust the state this way, just so you can use the . accessor later. I think the consul state should have these properties already, but that may be a bit troublesome to check now. So perhaps leave a TODO.
In the meantime, something more idiomatic is to bind these default to variable and use Map.get/3:
max_rating_to_play = Map.get(consul_state, :maximum_rating_to_play, @rating_upper_bound)
# and similar for the minimum bound
| cond do | ||
| play_level_bounds == nil && play_rank_bounds == nil -> [] | ||
| true -> ["This lobby has the following play restrictions:", play_level_bounds, play_rank_bounds] | ||
| |> Enum.filter(fn x-> x != nil end) |
There was a problem hiding this comment.
this looks janky, are you using elixir lsp? It comes with a code formatter.
| @@ -567,6 +567,7 @@ defmodule Teiserver.CacheUser do | |||
| def send_direct_message(from_id, to_id, "!joinas" <> s), | |||
| do: send_direct_message(from_id, to_id, "!cv joinas" <> s) | |||
|
|
|||
| @spec send_direct_message(T.userid(), T.userid(), list) :: :ok | |||
|
|
||
| rating_type = | ||
| cond do | ||
| team_count > 2 && team_size == 1 -> "FFA" |
There was a problem hiding this comment.
I would leave that as a comment in the code itself.
You could also leave that in the commit message, but for this, you would need a much smaller commit doing pretty much only this action.
| Returns {:ok, nil} | ||
| Or {:error, msg} | ||
| """ | ||
| def check_lobby_name(name, consul_state) do |
There was a problem hiding this comment.
Same comment regarding typespec and {:ok, nil}
| cond do | ||
| allwelcome_name?(name) -> {:error, "You cannot set a rating limit if all are welcome to the game"} | ||
| true -> {:ok, nil} | ||
| end |
There was a problem hiding this comment.
:D you do really like the cond structure. if or unless would do the job just fine here :D
| cond do | ||
| noob_matches > 0 && anti_noob_matches == 0 -> true | ||
| true -> false | ||
| end |
There was a problem hiding this comment.
you can directly return the result of the boolean operation: noob_matches > 0 && anti_noob_matches == 0
I find true -> false a little bit confusing.
| @@ -0,0 +1,58 @@ | |||
| defmodule Teiserver.Lobby.Libs.LobbyRestrictionsTest do | |||
| @moduledoc false | |||
| use Teiserver.ServerCase, async: false | |||
There was a problem hiding this comment.
I don't think you need ServerCase, ExUnit should be enough, this way you don't need to setup the entire server.
You can also make that synchronous, since you're not even touching the DB, you're only testing pure functions.
|
@geekingfrog thanks for review. I think I can do the changes you have requested and will work on it. Currently I use some formatter in VSCode but it is not set to auto format on save. So I must manually format the document. I could try and set it to autoformat on save. Also they currently have a staging server for integration tests. It was used for testing small/big team ratings. |
|
@geekingfrog I tried to do the changes. However, now getting failures on |
|
What is the dialyzer command you run, and what is the error you get? |
When I said "integration test" I meant more something that is run with |
However, I solved it and have updated the branch. Well actually I didn't solve it - I asked on Elixir forums. |
I just tested those spring tests and they don't work. I think we should fix those first (and can be done in a separate ticket) so I have some sort of template to follow and understand how they "should" be written. |
|
FFS, this PR is still not merged. Well, fuck it, test or not I don't care anymore, ship it. |
That's on me and my getting pulled apart in multiple directions. I'm trying to stay on top of things, but it can be hard at times. Thanks for your understanding! 😞 I've just merged that one now. |
There was a problem hiding this comment.
Looks reasonable overall from a partial review, but like @geekingfrog says - it's difficult to be confident about being able to merge this without either doing manual tests on the integration server or having automated tests doing that for us. The latter is of course preferable, otherwise we don't know when we break things.
Wanna VC sometime tomorrow or later and figure those out together? I need to finally sit down and figure Hailstorm out, that sounds like a good opportunity for it!
| result = LobbyRestrictions.is_noob_title?("Noobs 1v1") | ||
| assert result == true | ||
|
|
||
| result = LobbyRestrictions.is_noob_title?("No Noobs 1v1") | ||
| assert result == false | ||
|
|
||
| result = LobbyRestrictions.is_noob_title?("All Welcome 1v1") | ||
| assert result == false | ||
|
|
||
| result = LobbyRestrictions.is_noob_title?("Newbies 1v1") | ||
| assert result == true | ||
|
|
||
| result = LobbyRestrictions.is_noob_title?("Nubs 1v1") | ||
| assert result == true |
There was a problem hiding this comment.
For these boolean checks, probably a little bit cleaner and concise to do it like this:
| result = LobbyRestrictions.is_noob_title?("Noobs 1v1") | |
| assert result == true | |
| result = LobbyRestrictions.is_noob_title?("No Noobs 1v1") | |
| assert result == false | |
| result = LobbyRestrictions.is_noob_title?("All Welcome 1v1") | |
| assert result == false | |
| result = LobbyRestrictions.is_noob_title?("Newbies 1v1") | |
| assert result == true | |
| result = LobbyRestrictions.is_noob_title?("Nubs 1v1") | |
| assert result == true | |
| assert LobbyRestrictions.is_noob_title?("Noobs 1v1") | |
| refute LobbyRestrictions.is_noob_title?("No Noobs 1v1") | |
| refute LobbyRestrictions.is_noob_title?("All Welcome 1v1") | |
| assert LobbyRestrictions.is_noob_title?("Newbies 1v1") | |
| assert LobbyRestrictions.is_noob_title?("Nubs 1v1") |
This is, of course, a nitpick :P
|
@StanczakDominik These next few days aren't the best time for VC as I'm fairly busy. I'm in the office during the day and for the next couple of evenings I have plans. (Also I'm in Sydney time). My plan for this PR was to request at some point it to be placed on integration server and then create a thread in the testers channel. There I could ping @GeneralTesters and also individuals who would be interested in these changes. The following individuals should have an interest:
Spreezy and Sun_Tzu are pretty active on Discord and those two alone could go through all the test cases. And there would be random GeneralTesters who could probably assist. The tests are straight forward - they don't require starting a match as I didn't change the match library. As for the automated integration tests, I had a brief look and my skills aren't strong enough to even create them for this PR. I would need to invest more time in levelling up my skills. But if you're planning to look into maintaining Hailstorm then I can look into seeing if I can create some tests for existing work I've done i.e. the balancemode. I did look at Hailstorm in the past and understand a bit but need to revisit. |
|
Actually maybe it's best that I make some changes that would cause a merge conflict with Lexon. That would make us more cautious when merging together since it would generate an error. I want to add this to And then I would add this to This would conflict with Lexon's change here. But having the error on merge might be good reminder to adjust it as needed. |
|
Teiserver.Coordinator.ConsulCommandsTest already has some tests for lobby restrictions, including min/max/setranklimit. |
|
You mention the possible merge conflict with my changes. This is a pretty big PR, I think it would be nice if it could be split into maybe 2 smaller ones. It would make things easier to merge, specifically the fix for #85, that is something I was planning to solve in my PR to avoid issues with new rating categories. |
| def get_rating_bounds_text(state) do | ||
| get_rating_bounds_for_title(state) | ||
| end | ||
|
|
||
| def get_rank_bounds_text(state) do | ||
| get_rank_bounds_for_title(state) | ||
| end |
There was a problem hiding this comment.
What is the purpose of this? Why not call get_rank_bounds_for_title(state) or get_rating_bounds_for_title(state) directly?
| # Contributors auto pass since their ranks are not defined on playtime. To be fixed seperately. | ||
| is_contributor? = CacheUser.is_contributor?(user) |
There was a problem hiding this comment.
Rank is calculated in Teiserver.CacheUser, however there are alternative ways to calculate rank which can be set with profile.Rank method site configuration.
The default one and the one used on server4 is Role
def calculate_rank(userid, "Role") do
ingame_hours = rank_time(userid)
cond do
has_any_role?(userid, ~w(Core Contributor)) -> 6
ingame_hours > 1000 -> 5
ingame_hours > 250 -> 4
ingame_hours > 100 -> 3
ingame_hours > 15 -> 2
ingame_hours > 5 -> 1
true -> 0
end
endOther options are Uncertainity, Leaderboard rating and Playtime
def calculate_rank(userid, "Playtime") do
ingame_hours = rank_time(userid)
[5, 15, 30, 100, 300, 1000, 3000]
|> Enum.count(fn r -> r <= ingame_hours end)
end
# Using leaderboard rating
def calculate_rank(userid, "Leaderboard rating") do
rating = Account.get_player_highest_leaderboard_rating(userid)
[3, 7, 12, 21, 26, 35, 1000]
|> Enum.count(fn r -> r <= rating end)
end
def calculate_rank(userid, "Uncertainty") do
uncertainty =
Account.get_player_lowest_uncertainty(userid)
|> :math.ceil()
(8 - uncertainty)
|> max(0)
|> max(7)
endRole is the only one where contributors get a special rank so it might be a good idea to add a check for current rank method here as well.
| team_count > 2 && team_size > 1 -> "Team FFA" | ||
| team_size == 1 -> "Duel" | ||
| true -> "Team" | ||
| end |
There was a problem hiding this comment.
I noticed the game type logic is spread out in multiple places so I was planning to move it to one place as part of #296 or a separate PR.
|
@L-e-x-o-n If you can make a function in your PR that takes team count and team size and returns a rating type then I'll just wait for you to merge and use that. From what I read of your PR, we actually currently only conflict once in consul_server. It's not hard for me to resolve later. |
jauggy
changed the title
I missed this comment, but that's exactly what I did :D MatchLib.game_type/2 |
…server into lobby-chev-policies-4
jauggy
changed the title
|
Have now merged with master. Main change is here calling Lexon's recently made function Have tested and works locally |
Test results from the integration serverIn short, the only thing I was able to find was, immediately after switching to FFA preset, it was trying to gate on Duel rating (teamsize = 1), but explicitly switching to teamsize = 1 (or possibly changing the number of teams?) seemed to fix it. Overall, I think it's great - I'll just confirm a few things before merging this. |
StanczakDominik
merged commit e6e2881
into
beyond-all-reason:master
Improvements
minchevlevelandmaxchevlevelcommandsChev level commands
While other devs may want to use uncertainty filters, chev restrictions have the benefit that:
However, I acknowledge there are flaws with chevs which include: they can grow with spec time and pve time.
Contributors and Tournament Winners
Contributors chev level is not tied to hours played. Therefore I just allow contributors to ignore chev restrictions. This can be revisited in the future.
Tournament winners have chevlevel 8 and are treated as such. I don't think this is a big issue as they are likely to only play in min chev lobbies.
Other nuances
Teifion added code so that you cannot add restrictions for "All welcome" lobbies. It checks the lobby title (and is trivial to bypass). I did the same for chev restrictions.
I currently don't support having both min and maxchevlevels at the same time. Setting one will reset the other. I assume maxchevlevel will be used on noob lobbies and it wouldn't make sense to have a minchevlevel on a noob lobby. Can be revisited in the future if needed.
Merge conflicts
This PR has been written before Lexon has merged his big/small team rating split. If he merges first I only need to change a couple lines in the
check_rating_to_playfunction inside LobbyRestrictions module.Unit Tests
In VSCode, right click the
lobby_restrictions_test.exfile and copy its path. Then runIntegration Test Steps
If a player doesn't meet lobby play restrictions, the Coordinator will now inform them.
In a lobby use the
minratinglevelormaxratinglevelcommands. Try and move from spec to player when you don't meet requirements. You will be informed by the Coordinator on the right.When checking rating requirements, the lobby team size and team count is looked at to determine what rating type to use. It is the max team size that matters not number of players in the lobby since the lobby may be getting filled. E.g. a lobby with team size 8 and only 2 players will use "Big Team" rating. A lobby with team size between 2 and 5 will use "Small Team" rating.
Try the above but try changing team size/team count drop downs and note the warning given by Coordinator changes.
When the server thinks you are trying to create a noob lobby, some tips will be given by coordinator
Try
$rename nooband see the coordinator message on the right.minchevlevel and maxchevlevel commands
Type
$helpand look for the new minchevlevel and maxchevlevel commands and how to use them. Test them out. Check that when you don't meet requirements to play the Coordinator tells you. Note that$minchevlevel 1will not work as that is everyone. If you are a contributor, you ignore all chevron restrictions.Remove filters in lobby name when everyone leaves
While in lobby run
$statusto see the restrictions you set up earlier. Check that the lobby name has the restrictions. Leave the lobby. Note the change in title. Enter the lobby and run$statusand note the lobby restrictions are gone.