Role Gen Fix

[AlchlcDvl]

Addresses #177 and #180

Basically the issue was that the order or the list would mostly be fixed because of the settings staying the same across games. In this pr what it does is basically, it adds the role with 100% first, then randomises the last bit. After which the list is shuffled. For extra insurance, the list is also shuffled when performing TakeFirst so it would be less likely to raise issues

[JeanTheFungle]

Good Changes @AlchlcDvl

[fariparedes]

FYI, these changes seem to result in the roles with 100% being placed after other roles, meaning they are never selected if there are too few players to take every role. This seems like the opposite of the intended functionality.

[AlchlcDvl]

FYI, these changes seem to result in the roles with 100% being placed after other roles, meaning they are never selected if there are too few players to take every role. This seems like the opposite of the intended functionality.

it doesn't, at first it simply adds them to the front of the list and then that list is shuffled every time an element is pulled from it. it never can have all 100% roles at the bottom of the list

[fariparedes]

Probably just a small sample size then. Still, it seems odd for 100% roles to not be guaranteed over say, 20% roles. Maybe they should be put into two separate lists, shuffled, and then the sub-100% roles merged onto the end of the 100% list?

[AlchlcDvl]

ok so basically here's how the new sorting works

it adds all the roles with 100 chance first into the list, then the roles with a lower chance is then added

at each phase it checks if the size of the list is equal to the amount that was set, if it is that phase aborts.

every time an element is pulled, it is randomised before and after pulling so it's completely rng

the only issue i see is that roles that are lower in sequence might have a higher chance of not appearing which i'll look to fixing soon

[AlchlcDvl]

now this thing shuffles the spawning list at every moment, so it's almost impossible for the rng to keep giving you the same role

[AlchlcDvl]

this is just a band aid pr tho, the role gen would need to be improved a lot more for it to actually have a definite rng property

[fariparedes]

OK, I think I've identified a problem.

On line 305, you can see that crewRoles and impRoles are shuffled, and then GenRole is called to start handing out roles. However, Classic mode uses the ImpostorRoles and crewAndNeutralRoles lists, NOT the crewRoles and impRoles lists (these are used for the "All/Any" gamemode).

Therefore, after the Neutral roles are added to crewAndNeutralRoles on lines 261-264, that list is NOT shuffled before the roles start to be handed out (this may mean that the first crew assigned will NEVER be a neutral?). Obviously this is not ideal, the list should be fully random going into role assignment. Shuffling it after each role is taken out will put a bandaid on this issue, but the whole function should probably be rewritten so that it's unnecessary. Adding more randomness will not fix things, it may even make it worse.

The shuffle algorithm, I note, is a Fisher-Yates shuffle which is not going to feel very random because elements frequently remain in the same place, but it will generate each permutation around the same number of times so it will be appropriately random in the long run.

Another element to the feeling of "streaks" is the impostor generation, which I believe is still done by base Among Us, and untouched by this mod, and that is partially what #180 is complaining about, so I believe that this PR does not do anything to address that.

[fariparedes]

BTW, the current code does not correctly implement Fisher-Yates either. Here is a correct implementation:

public static void Shuffle<T>(this List<T> list)
{
    var count = list.Count;
    for (var i = count-1; i > 0; --i)
    {
        var j = Random.Range(0, i+1);
        (list[i], list[j]) = (list[j], list[i]);
    }
}

As you can see on the wiki page, 0 <= j <= i, and when counting up you must count from 0 to n-2. This is not the case for the current implementation, which counts from 0 to n-1 (I changed it above to be n-1 to 0) and does not account for the fact that Random.Range is exclusive on its maximum.

[AlchlcDvl]

well i wasn't thinking that deep into it. i also faced the same issue with role gen being repetitive in my mod which was why i had recoded it completely

i brought the sorting from there into this one in hopes to fix the issue because it did in my mod. now i realise that might have been a mistake since over there i sorted and handed out each faction separately and then did the assignment at once. and not only the role list was shuffled, so were the players so you had a double randomisation going there

looks like i might have to do a full recode of the role gen like i did previously

[AlchlcDvl]

as for the shuffling, i didn't touch that but i could very well implement the algorithm you provided

[fariparedes]

I'm presently working on a full recode of GenEachRole. I'll show you when it's done and we can compare notes? :)

[AlchlcDvl]

sure! i'm open to looking for ways to improve role gen (given its current janky-ness)

[AlchlcDvl]

shall we continue this on discord? i don't visit github very often + ease of use

[fariparedes]

Additional issue: SortRoles() appears to do nothing at all, presumably because the roles variable is not an out parameter, lol. You can fix this by calling roles.Clear() and then roles.AddRange(newList) instead of calling roles = newList.

[fariparedes]

I've submitted a PR to your repo with my changes. I tested them pretty thoroughly today, and found them most satisfactory. It did not feel like there was any "streaking" of roles at all even with only 5 players in the lobby.

https://github.com/AlchlcDvl/Town-Of-Us-R/pull/1

[AlchlcDvl]

Will add them!

[AlchlcDvl]

Ill fully through this when i get home

[AlchlcDvl]

alright im home and looked through all of the changes. this method is actually better than what i had planned so there's no need for what i had to do anymore lol. as for the shuffles, honestly i'd let them stay there so it's randomised every time as like a deterrent to more possible repetitions. but it's up to donners and mdb if they wanna keep them in ¯_(ツ)_/¯

[JeanTheFungle]

this pr need to be merged in TOU

[JeanTheFungle]

@AlchlcDvl i wante test ur mod i didn't find people to play with him where i can found ?

[AlchlcDvl]

first of all, i'd like to read english. second, you can use mci to test out the mod yourself

[JeanTheFungle]

first of all, i'd like to read english. second, you can use mci to test out the mod yourself

sometime my english is broken

[Gunx30]

mci is alr built into the mod btw just press f2

[AlchlcDvl]

no it isn't

[Gunx30]

ad i was talking about reworked....

[AlchlcDvl]

oh-

[MyDragonBreath]

Give me a week and I'll code review- I've just had a lot on

[JeanTheFungle]

Give me a week and I'll code review- I've just had a lot on

hey i would like see my pull request too if possible #175

[AkitaAttribute]

What happened to this? Asking cause I watch people who play this weekly.

[AlchlcDvl]

the mod is getting a rewrite so the pr will be merged into it in the next release

[JeanTheFungle]

the mod is getting a rewrite so the pr will be merged into it in the next release

how did you know ? so if i made any something new it can be added ?

[AlchlcDvl]

if it's good enough quality, but right now the mod's prioritising bug fixes and updating to the latest version

[MyDragonBreath]

Closing - but has been merged