Skip to content

Role distribution inspection#1714

Merged
TimGoll merged 72 commits intoTTT-2:masterfrom
nike4613:role-scrutability
Jan 26, 2025
Merged

Role distribution inspection#1714
TimGoll merged 72 commits intoTTT-2:masterfrom
nike4613:role-scrutability

Conversation

@nike4613
Copy link
Contributor

This PR adds an admin UI to inspect how and why role distribution selects certain roles, and why it distributes those roles to players.

The goal here is to assist admins in understanding the role distribution process, so they can more effectively tweak the parameters involved, and better see their effects.

This also includes a graphic showing player weights during role distribution (if derandomization is enabled) which can assist debugging that as well as help intuition when tweaking it.

Some of the graphics are a touch jank, and in particular I wish there was a good way to get an icon in the label of a collapsible form, but just role names probably works fine anyway.

This PR is based on (and soft depends-on) #1702. That dependency can be removed if necessary.

Screenshots:
Screenshot_20250111_221601
Screenshot_20250111_222215
Screenshot_20250111_222252
Screenshot_20250111_222312
Screenshot_20250111_222324
Screenshot_20250111_222328
Screenshot_20250111_221616
Screenshot_20250111_221629
Screenshot_20250111_221656
Screenshot_20250111_221731
Screenshot_20250111_221747

@TimGoll
Copy link
Member

TimGoll commented Jan 20, 2025

Regarding my first comment, is there a reason it is disabled by default?

image

@nike4613
Copy link
Contributor Author

Regarding my first comment, is there a reason it is disabled by default?

image

It records quite a lot of information, including lots of cloned tables, and persists them for an arbitrary amount of time. For large player counts, this could hypothetically become an issue, and I dislike silently changing behavior, even if it should be mostly transparent like this.

@TimGoll
Copy link
Member

TimGoll commented Jan 20, 2025

It records quite a lot of information, including lots of cloned tables, and persists them for an arbitrary amount of time. For large player counts, this could hypothetically become an issue, and I dislike silently changing behavior, even if it should be mostly transparent like this.

Ok, makes sense. If you think that it is too heavy, then it is probably the best. Personally I chose to enable things by default and give users the option to disable it. So they see that it is there and can disable it if they so chose

@TimGoll TimGoll self-assigned this Jan 21, 2025
@TimGoll TimGoll added type/enhancement Enhancement or simple change to existing functionality type/feature New functionality labels Jan 21, 2025
Copy link
Member

@TimGoll TimGoll left a comment

Choose a reason for hiding this comment

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

Not an in-depth review. I might take another look at your derma code. But overall this looks pretty good to me. You have a few blobs of dead code in your code, which should be removed. Also either tackle the ToDos, or remove them

@nike4613 nike4613 requested a review from TimGoll January 24, 2025 01:45
Copy link
Member

@TimGoll TimGoll left a comment

Choose a reason for hiding this comment

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

Mostly just minor nitpicks that build on top stuff already pointed out.

Don't get me wrong, because I'm really impressed by the stuff you did here, but most of my comments are about stuff you are able to spot yourself in 10 minutes when scrolling through the diff. This could have been quicker for you, if you removed any dead code :D

Thank you!

Copy link
Member

@TimGoll TimGoll left a comment

Choose a reason for hiding this comment

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

From my POV this PR is good as it is now. Since this is a rather big PR, I'd like a quick ok from @Histalek or @saibotk here as well. You don't have to review this in depth, but please give it a quick scroll through

Copy link
Member

@Histalek Histalek left a comment

Choose a reason for hiding this comment

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

the function typo needs fixing, otherwise this seems good to go :)

Amazing work!

Co-authored-by: Histalek <16392835+Histalek@users.noreply.github.com>
@TimGoll TimGoll merged commit e32af20 into TTT-2:master Jan 26, 2025
4 checks passed
Histalek added a commit to WardenPotato/TTT2 that referenced this pull request Jan 31, 2025
This PR adds an admin UI to inspect how and why role distribution
selects certain roles, and why it distributes those roles to players.

The goal here is to assist admins in understanding the role distribution
process, so they can more effectively tweak the parameters involved, and
better see their effects.

This also includes a graphic showing player weights during role
distribution (if derandomization is enabled) which can assist debugging
that as well as help intuition when tweaking it.

Some of the graphics are a touch jank, and in particular I wish there
was a good way to get an icon in the label of a collapsible form, but
just role names probably works fine anyway.

This PR is based on (and soft depends-on) TTT-2#1702. That dependency can be
removed if necessary.

Screenshots:

![Screenshot_20250111_221601](https://github.com/user-attachments/assets/3238a7ae-68ad-4e32-89f2-3c88ae47faea)

![Screenshot_20250111_222215](https://github.com/user-attachments/assets/623eac41-db13-45d0-a8aa-46815bc72603)

![Screenshot_20250111_222252](https://github.com/user-attachments/assets/cae1e417-d63b-44fe-9bc0-73be97007d0c)

![Screenshot_20250111_222312](https://github.com/user-attachments/assets/1f4ca740-f9c7-4ded-be63-da09b73d98b3)

![Screenshot_20250111_222324](https://github.com/user-attachments/assets/b34f0688-5423-4bf9-8bba-8fd312631056)

![Screenshot_20250111_222328](https://github.com/user-attachments/assets/2e9c9ba4-ddfa-43ea-b492-c5a59f0b8bcc)

![Screenshot_20250111_221616](https://github.com/user-attachments/assets/e63513b4-5852-4a6e-a578-abbbf9b95ddd)

![Screenshot_20250111_221629](https://github.com/user-attachments/assets/980540be-54e9-4268-93cd-687ea3be8611)

![Screenshot_20250111_221656](https://github.com/user-attachments/assets/e1f1798f-4e68-4ad2-b54c-61751d64d714)

![Screenshot_20250111_221731](https://github.com/user-attachments/assets/7884d52a-c323-41ed-8d9b-137627c8574c)

![Screenshot_20250111_221747](https://github.com/user-attachments/assets/90405fac-f055-4d97-93ea-da771c6bbfbc)

---------

Co-authored-by: Tim Goll <github@timgoll.de>
Co-authored-by: Histalek <16392835+Histalek@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type/enhancement Enhancement or simple change to existing functionality type/feature New functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants