Antag math is confusing as hell

[SpacePrius]

So while I was digging through antag code for my own antag idea, I hit a brick wall in that it seems that a lot of the math is dense, undocumented, and poorly designed. I could not for the life of me understand what the hell it was supposed to be doing, or why it was doing it this way. Here is the example of the antag cap for the traitor gamemode.

Code: Select all

var/num_traitors = 1

	var/tsc = CONFIG_GET(number/traitor_scaling_coeff)
	if(tsc)
		num_traitors = max(1, min(round(num_players() / (tsc * 2)) + 2 + num_modifier, round(num_players() / tsc) + num_modifier))
	else
		num_traitors = max(1, min(num_players(), traitors_possible)) 

I can't even begin to decipher this or what any of it is trying to do. This is a major problem, not only for long term code maintenance, but for any balance changes that involve this kind of complicated math. Since I don't understand how this behaves I don't know if it is working properly or how it is working at all.

[PKPenguin321]

Need to see a full code block but it's not actually that complicated at all, only weird thing is that magic + 2

Basically the tsc var is the traitor scaling coefficient, a config-settable value that allows us to scale the number of antags by server population size without changing hardcoded variables. It checks that the config value has been set, and if it has then it runs a calculation that basically says "the number of traitors this round will either be the number of players divided by 2x the TSC and + 2 (this +2 is the only part that jumps out as strange but I'm sure if you graphed outputs of this calculation it would make more sense) + a modifier (I don't know what this is since you didn't post the full code block), or the number of players divided by the TSC and + the modifier, whichever is smaller. Also, if both of these values are less than 1, then we instead force there to be exactly 1 traitor."

If there's no TSC then it just says "just make as many traitors as we have set in traitors_possible, unless there are like almost no players in which case we make them all antags. And if there are somehow less than 1 players we make exactly 1 antag." That last part probably isn't necessary, but this code should never run anyways since we always have a TSC set server-side.

Again, if you graph this out it will probably make a shitload more sense. The calculations themselves aren't very hard to follow at all. Could use a comment that gives some sample outputs though. If you're utterly lost because of this you should probably practice more or read more slowly or something, this isn't a failiure on the code's part.

[MisterPerson]

Here's the PR that changed it to what it is today complete with graph demostrating things: https://github.com/tgstation/tgstation/pull/4657

Let's break it down a bit. First, min()/max() used like that is Clamp(). Second, round() is floor(). Third, let's remove the num_modifier for now:

Code: Select all

num_traitors = Clamp(1, floor(num_players() / (tsc * 2)) + 2, floor(num_players() / tsc))

So let's take both sides of the clamp separately and call them limit1 and limit2:

Code: Select all

var/limit1 = floor(num_players() / (tsc * 2)) + 2
var/limit2 = floor(num_players() / tsc)
num_traitors = Clamp(1, limit1, limit2)

We're then going to take whichever limit is lower. tsc is just some number in the config. The second limit is simple: everyone gets a 1/coefficient chance of being a traitor. The first limit is more confusing but basically it's a reducer on the number of traitors at high pop so there's not literally 20 traitors running around. The +2 is an extra 2 traitors to make sure this limit isn't getting hit with very low numbers of players.

[SpacePrius]

The math should be documented IN the code, which is the problem in the first place. I shouldn't have to be digging for a 5 year old PR to understand how the math works. I get what its doing, but when it isn't actually explained in the code it makes it way harder to replicate.

Also that latter one is basically what I ended up doing, and what we should probably change it to on all antags. It makes it way more digestible to look at and modify.

[Dr_bee]

The math should be documented IN the code, which is the problem in the first place.

Oranges is going hard on making people actually document their PRs in the code to prevent this kind of confusion. Say what you will about orange man, but that is a good change to make.

[Akrilla]

I agree it should be documented, which is now starting to be mandatory now, but this really isn't that hard to understand if you break it up. A simple coefficient to scale traitors to both low and high pop.

[Ikarrus]

I'm the PR ghost, so this is my fault

Although I swear it used to be more clear before a bunch of variables like tsc were added since I last touched it

I'm sure if I were better at math at the time I could have make a better looking curve instead of taking the min of two lines

like traitors = players ^ (0.5) and use an exponent instead of a coeff for the config

[PKPenguin321]

I'm the PR ghost, so this is my fault

Although I swear it used to be more clear before a bunch of variables like tsc were added since I last touched it

I'm sure if I were better at math at the time I could have make a better looking curve instead of taking the min of two lines

like traitors = players ^ (0.5) and use an exponent instead of a coeff for the config

Considering that you're rounding to whole numbers and that the calculation doesn't need to be that complex, just taking the min of 2 lines is good enough

[Ikarrus]

its so fucking awful to look at, though