The probability is a bit off

Todd_Vance · October 7, 2022, 8:11pm

Even though I’m a mathematician, I have to look at some things carefully to convince myself it’s right. Turns out it isn’t, exactly, though without taking statistics of game plays, it would probably not be noticed.

gist.github.com

https://gist.github.com/DeplorableMountaineer/6a28fc47320057dbcfc89f695336e9ba

DropLibrary.cs

private DropConfig SelectRandomItem(int level){
            float totalChance = GetTotalChance(level);
            float randomRoll = Random.Range(0, totalChance);
            float chanceTotal = 0;
            foreach(DropConfig drop in potentialDrops){
                chanceTotal += GetByLevel(drop.relativeChance, level);
                if(chanceTotal > randomRoll) return drop;
            }

            return null;

This file has been truncated. show original

So, with some pencil and paper work (I used a pen since it was handy, but still…) I see that if the random roll is between 0 and the very first relative chance, nothing will be dropped and null will be returned. Only if the character had a 100% drop chance would this be obvious, though, and then after many trials.

The solution is to put the if(chanceTotal… line BEFORE the chanceTotal +=… line.

A second less serious issue is the random roll has a minute probability of equaling the totalChance and thus the > sign should be >=. This bug would almost surely never be noticed in practice. (Unity random returns min <= value < max if min and max are both integers, but if either is a float, it returns min <= value <= max.)

Another issue which affects the probabilities and thus might not be noticed immediately, is:

gist.github.com

https://gist.github.com/DeplorableMountaineer/b266f2757494829c3485f03ed80deb96

DropLibrary.cs

public IEnumerable<Dropped> GetRandomDrops(int level){
            if(!ShouldRandomDrop(level)) yield break;
            for(int i = 0; i < GetRandomNumberOfDrops(level); i++){
                yield return GetRandomDrop(level);
            }
        }

C# (I had to look it up to be sure) is like C or C++ in that if you call a function in the second part of the three-part for loop argument, it gets called every iteration. Therefore, the first time the index is greater than or equal to the function result, the loop will terminate. Thus, it will tend to terminate early, on average, and larger numbers of drops will occur less often than smaller numbers of drops. Of course, you might want that, but my opinion is it would be better to make that explicitly happen rather than by accident!

Brian_Trotter · October 8, 2022, 3:50am

To the second point, you’re right, we should be caching GetRandomNumberOfDrops(level)

int numberOfDrops = GetRandomNumberOfDrops(level);
for(int i=0;i<GetRandomNumberOfDrops(level); i++)

I’m going to disagree on the SelectRandomItem, however. This is a classic ticket scheme, just using floats instead of ints…
So the first chanceTotal represents the chance as of the current DropConfig…
With no dropconfig, the chanceTotal is 0
If the total chances is 100 (counting all the drop.relativechances), and let’s suppose the roll is 4
If the first DropConfig has a relativechance of 5, then ChanceTotal will now be 5, and the randomRoll will be four and the first item will be dropped.

Were you to reverse this, checking first and adding the relative chance after, then the first item would have an effective chanceTotal of 0, and would fail the test, and the second item would be selected (as the relativechance from the 1st item would be effectively used as the chancetotal for the second item.

The >= observation, though, is spot on. There is about a 1 in 1e07 (about 1 in 10 million) chance of getting a float result equal to the chance total (and before you ask, it doesn’t matter if the total chance is 5 or 10,000,000, the result is a float between 0 and 1 (inclusive) multiplied by the total number of chances, lower total chances = higher granularity in the result, but the odds of getting 0 or 1 are the same).

Todd_Vance · October 8, 2022, 9:08am

You’re right about SelectRandom: for some reason I was reading it as “randomRoll > chanceTotal”.

Satinel · November 4, 2022, 7:29pm

Just to clarify, rather than:

int numberOfDrops = GetRandomNumberOfDrops(level);
for(int i = 0; i < GetRandomNumberOfDrops(level); i++)

Which caches the value but doesn’t appear to use it, we’d want:

int numberOfDrops = GetRandomNumberOfDrops(level);
for (int i = 0; i < numberOfDrops; i++)

Correct?

Brian_Trotter · November 4, 2022, 9:35pm

That is correct.