Feature/role preference options #1142

EntranceJew · 2023-11-21T19:23:23Z

made dead code not dead

TimGoll

Nice contribution thank you. I also like the way you named your variables. I'm hesitating a bit because of the convars though

gamemodes/terrortown/gamemode/server/sv_roleselection.lua

TimGoll · 2023-11-22T09:12:09Z

gamemodes/terrortown/gamemode/server/sv_roleselection.lua

+
+	---
+	-- @realm server
+	ttt2_roles_trickle_down_odds = CreateConVar("ttt2_roles_trickle_down_odds", "3", {FCVAR_NOTIFY, FCVAR_ARCHIVE}, "The 1/N odds of a player receiving a role and not being passed up."),


I'm also not 100% sure we need this. If you add this, you have to explain in detail in the UI how changing this convar affects the selection. Because I think it is hard to understand.

Personally, I don't even like having the math.random call in there -- I think the shuffle is doing all the work the RNG really needs to do to consider things.

Should we just only focus on conditions that prevent the role from being selected and pass true by default?

That's hard to make a call on as I never worked on the roleselection. Maybe @saibotk has more insight into this. Or @ZenBre4ker?

The system is broken in a way anyway. As @saibotk mentioned in another PR, I wouldnt give so many options. I would either always allow or forbid avoiding roles alltogether. I dont like that system anyway, so I would suggest to remove role avoiding alltogether.

About the random call, I believe they tried to make it more random as in the past often the same players in a row got the same role over and over again.

And the trickle system I need to take a look again.

I feel the same way, all this role avoidance stuff is rarely used by players and should rather be something a server owner decides upon. This just adds unnecessary complexity and confusion.

Yeah checked again, I would just set the tricklerate as before to 3 and check if it is 2, so 1/3 chance, that role gets assigned without the other conditions.

gamemodes/terrortown/gamemode/server/sv_roleselection.lua

TimGoll · 2023-11-22T09:15:03Z

I also have to run the TTT2LanguageParser on this one

gamemodes/terrortown/gamemode/server/sv_roleselection.lua

ZenBre4ker

You definitely revived the dead code 😄

But I believe, that you implemented SelectBaseRolePlayers wrongly. Removing the random aspect and the while loop means, that this pass could be false for many players.
I am unsure about the consequences, but it does not have the same behaviour as before, if I am not wrong.

Also in my opinion please dont insert two more convars. The trickle rate is just there to randomize the other states.

ZenBre4ker · 2023-11-23T16:22:55Z

gamemodes/terrortown/gamemode/server/sv_roleselection.lua

+		for j = 1, #plysSecondPass do
+			local ply = plysSecondPass[j]


This should stay as "i". I dont see where this would be a problem.

gamemodes/terrortown/gamemode/server/sv_roleselection.lua

ZenBre4ker · 2023-11-23T16:35:10Z

gamemodes/terrortown/gamemode/server/sv_roleselection.lua

+
+	---
+	-- @realm server
+	ttt2_roles_trickle_down_odds = CreateConVar("ttt2_roles_trickle_down_odds", "3", {FCVAR_NOTIFY, FCVAR_ARCHIVE}, "The 1/N odds of a player receiving a role and not being passed up."),


Yeah checked again, I would just set the tricklerate as before to 3 and check if it is 2, so 1/3 chance, that role gets assigned without the other conditions.

ZenBre4ker · 2023-11-23T16:44:47Z

gamemodes/terrortown/gamemode/server/sv_roleselection.lua

-	local minKarmaCVar = GetConVar("ttt_" .. roles.GetByIndex(subrole).name .. "_karma_min")
-	local min_karmas = minKarmaCVar and minKarmaCVar:GetInt() or 0
-
-	while curRoles < roleAmount and #plys > 0 do


Doesnt removing this while loop here means, that players might not get a role in this pass?

As far as I can see, the "CanSelectRole" method can most often just return false.

I'm also unsure about what this change implies for the selection process further down.

We need to think of the assumptions, that were made for what this function does codewise.
It always tries to assign players to the given subrole until roleAmount of players received the role.
This is now not the case anymore and we need to check if that is assumed to happen here and if other functionality relies on that to happen etc.

(After requested Review)
Isnt this still having the same problem here?
Exchanging the while loop with a for loop seems like different behaviour?

TimGoll · 2023-12-06T18:29:48Z

I see you worked again on this PR. We discussed this one as well las sunday and we had the uniform opinion, that the trickle down convar should be removed. It might be a good idea to move it to a library variagble though. Something like roleselection.trickleDownRate. That way addons can easily interact with it, if they so desire.

@ZenBre4ker Wasn't sure if you broke any of the roleselection logic, so he has to confirm your changes.

Last but not least we'd like to let you know that we tend to remove the role avoidance completely as it doesn't work that well anyway. Most players seem really confused by it.

EntranceJew · 2023-12-07T04:38:55Z

is the intention to have this commit remove the role avoidance settings instead? or will that be later?

TimGoll · 2023-12-07T07:31:07Z

is the intention to have this commit remove the role avoidance settings instead? or will that be later?

Later. If you want to add to the role avoidance that is fine. Just be aware that it will probably be removed at some point

saibotk · 2023-12-07T10:25:57Z

gamemodes/terrortown/gamemode/server/sv_roleselection.lua

@@ -16,6 +16,7 @@ roleselection.finalRoles = {}
 roleselection.selectableRoles = nil
 roleselection.baseroleLayers = {}
 roleselection.subroleLayers = {}
+roleselection.trickleDownRate = 0


This still can have a sane default, as it was there before too with the random(3) == 2

This is still 0, should be at least 3.

saibotk · 2023-12-07T10:50:01Z

gamemodes/terrortown/gamemode/server/sv_roleselection.lua

-	local minKarmaCVar = GetConVar("ttt_" .. roles.GetByIndex(subrole).name .. "_karma_min")
-	local min_karmas = minKarmaCVar and minKarmaCVar:GetInt() or 0
-
-	while curRoles < roleAmount and #plys > 0 do


I'm also unsure about what this change implies for the selection process further down.

We need to think of the assumptions, that were made for what this function does codewise.
It always tries to assign players to the given subrole until roleAmount of players received the role.
This is now not the case anymore and we need to check if that is assumed to happen here and if other functionality relies on that to happen etc.

ZenBre4ker · 2023-12-19T09:07:18Z

gamemodes/terrortown/gamemode/server/sv_roleselection.lua

-	local minKarmaCVar = GetConVar("ttt_" .. roles.GetByIndex(subrole).name .. "_karma_min")
-	local min_karmas = minKarmaCVar and minKarmaCVar:GetInt() or 0
-
-	while curRoles < roleAmount and #plys > 0 do


(After requested Review)
Isnt this still having the same problem here?
Exchanging the while loop with a for loop seems like different behaviour?

ZenBre4ker · 2023-12-19T09:08:49Z

gamemodes/terrortown/gamemode/server/sv_roleselection.lua

@@ -16,6 +16,7 @@ roleselection.finalRoles = {}
 roleselection.selectableRoles = nil
 roleselection.baseroleLayers = {}
 roleselection.subroleLayers = {}
+roleselection.trickleDownRate = 0


This is still 0, should be at least 3.

ZenBre4ker · 2023-12-19T09:12:02Z

gamemodes/terrortown/gamemode/server/sv_roleselection.lua

-	while plysAmount > 0 and availableRolesAmount > 0 do
-		local pick = math.random(plysAmount)
-		local ply = plys[pick]
+	for i = #newPlys, 1, -1 do


The for loop here also has the wrong behaviour. It only goes through every player once. But it could be that some players are not assigned.

ZenBre4ker · 2023-12-19T09:14:17Z

CHANGELOG.md

@@ -108,6 +108,10 @@ All notable changes to TTT2 will be documented here. Inspired by [keep a changel
 - Moved reset buttons onto the left (by @a7f3)
 - Added ammo icons to the weapon switch HUD and player status HUD elements (by @EntranceJew)
 - Changed the disguiser icon to be more fitting (by @TimGoll)
+- Role selection RNG improvements (by @EntranceJew):


Also in my opinion there are no RNG improvements, as you said in this PR, it generally only "made dead code not dead"

… into feature/role-preference-options

# Conflicts: # CHANGELOG.md

TimGoll requested changes Nov 22, 2023

View reviewed changes

saibotk requested changes Nov 23, 2023

View reviewed changes

gamemodes/terrortown/gamemode/server/sv_roleselection.lua Outdated Show resolved Hide resolved

ZenBre4ker requested changes Nov 23, 2023

View reviewed changes

EntranceJew force-pushed the feature/role-preference-options branch from 965dd50 to 5ca661c Compare December 6, 2023 14:10

EntranceJew force-pushed the feature/role-preference-options branch 2 times, most recently from eefe340 to f5d7a83 Compare December 7, 2023 10:11

saibotk requested changes Dec 7, 2023

View reviewed changes

EntranceJew force-pushed the feature/role-preference-options branch from f5d7a83 to a9ac53f Compare December 7, 2023 21:51

Histalek added type/enhancement Enhancement or simple change to existing functionality accepted labels Dec 8, 2023

EntranceJew force-pushed the feature/role-preference-options branch 5 times, most recently from 5c71b7b to aa6e29a Compare December 15, 2023 00:02

EntranceJew force-pushed the feature/role-preference-options branch from aa6e29a to c0a4b86 Compare December 18, 2023 10:32

allow role avoiding, role RNG optimized

5e549e3

EntranceJew force-pushed the feature/role-preference-options branch from c0a4b86 to 5e549e3 Compare December 18, 2023 11:17

Merge branch 'master' into feature/role-preference-options

9ff6bab

EntranceJew requested review from ZenBre4ker, saibotk and TimGoll December 19, 2023 03:35

ZenBre4ker requested changes Dec 19, 2023

View reviewed changes

ZenBre4ker reviewed Dec 19, 2023

View reviewed changes

EntranceJew added 2 commits December 19, 2023 05:08

Merge branch 'master' into feature/role-preference-options

fdd18d7

Merge branch 'master' into feature/role-preference-options

9121aa3

EntranceJew added 6 commits December 19, 2023 15:11

Merge branch 'master' into feature/role-preference-options

0cad328

Merge branch 'master' into feature/role-preference-options

bc466e5

Merge branch 'master' into feature/role-preference-options

5de648e

Merge branch 'master' into feature/role-preference-options

845348c

Merge branch 'master' into feature/role-preference-options

2c72f38

Merge branch 'master' into feature/role-preference-options

393f6f3

Histalek added this to the v0.13.0b milestone Dec 20, 2023

EntranceJew added 9 commits December 20, 2023 10:37

Merge branch 'master' into feature/role-preference-options

24cc875

Merge branch 'master' into feature/role-preference-options

988ea27

Merge branch 'master' into feature/role-preference-options

3b3b866

Merge branch 'master' into feature/role-preference-options

f718dc8

Merge branch 'master' into feature/role-preference-options

c3d9fde

allow role avoiding, role RNG optimized

af3e270

Merge remote-tracking branch 'origin/feature/role-preference-options'…

ea96711

… into feature/role-preference-options

Merge branch 'master' into feature/role-preference-options

6292d93

Merge branch 'master' into feature/role-preference-options

15b40ce

NovaDiablox added a commit to NovaDiablox/TTT2 that referenced this pull request Dec 29, 2023

Added Feature/role preference options TTT-2#1142 Text

75afb01

EntranceJew added 2 commits December 30, 2023 01:16

Merge branch 'master' into feature/role-preference-options

43d8d3d

Merge branch 'master' into feature/role-preference-options

9a1b12b

NovaDiablox mentioned this pull request Dec 31, 2023

Updated Turkish Localization #1257

Closed

EntranceJew added 8 commits January 4, 2024 16:37

Merge branch 'master' into feature/role-preference-options

3d2274a

Merge branch 'master' into feature/role-preference-options

8e9f04f

Merge branch 'master' into feature/role-preference-options

e97f203

Merge branch 'master' into feature/role-preference-options

dfd5605

changelog migrate

a98d02d

Merge branch 'master' into feature/role-preference-options

5af37d2

Merge branch 'master' into feature/role-preference-options

dae87eb

# Conflicts: # CHANGELOG.md

Merge branch 'master' into feature/role-preference-options

aaa1f19

EntranceJew closed this Jan 11, 2024

EntranceJew deleted the feature/role-preference-options branch January 11, 2024 16:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/role preference options #1142

Feature/role preference options #1142

EntranceJew commented Nov 21, 2023

TimGoll left a comment

TimGoll Nov 22, 2023

EntranceJew Nov 22, 2023

TimGoll Nov 22, 2023

ZenBre4ker Nov 23, 2023

saibotk Nov 23, 2023

ZenBre4ker Nov 23, 2023

TimGoll commented Nov 22, 2023

ZenBre4ker left a comment

ZenBre4ker Nov 23, 2023

ZenBre4ker Nov 23, 2023

ZenBre4ker Nov 23, 2023

saibotk Dec 7, 2023

ZenBre4ker Dec 19, 2023

TimGoll commented Dec 6, 2023

EntranceJew commented Dec 7, 2023

TimGoll commented Dec 7, 2023

saibotk Dec 7, 2023

ZenBre4ker Dec 19, 2023

saibotk Dec 7, 2023

ZenBre4ker Dec 19, 2023

ZenBre4ker Dec 19, 2023

ZenBre4ker Dec 19, 2023

ZenBre4ker Dec 19, 2023

Feature/role preference options #1142

Feature/role preference options #1142

Conversation

EntranceJew commented Nov 21, 2023

TimGoll left a 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

TimGoll commented Nov 22, 2023

ZenBre4ker left a 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

TimGoll commented Dec 6, 2023

EntranceJew commented Dec 7, 2023

TimGoll commented Dec 7, 2023

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