axx0 / Civ2-clone

An open-source re-implementation of Civilization 2
GNU General Public License v3.0
65 stars 14 forks source link

Fastrandom bug? #108

Closed axx0 closed 7 months ago

axx0 commented 7 months ago

@reubene should this line https://github.com/axx0/Civ2-clone/blob/4ec54eeee60db703c4a275bc35ad44d008e2fe5b/Engine/src/FastRandom.cs#L52 instead have max+1? It should generate numbers 0...5 (including) for max=5 right?

reubene commented 7 months ago

This code is poorly documented, so I'm not really sure. I think the idea of % max-1 is that there are therefore max possible numbers so Next(5) will give five possible results zero through four.

axx0 commented 7 months ago

For Next(5): max-1 gives 0,1,2,3 max gives 0,1,2,3,4 max+1 gives 0,1,2,3,4,5

Not sure which of the last two is the correct one. I think some other methods in the class have similar issues.

Anyway starting a new game it always chooses Romans from [Roman,Celtic,Russian] civs. For other civs it however seems to chose random, as it should. I'll look into it more.

reubene commented 7 months ago

Have you changed the seed? I had the random number generator hard coded to a seed of 1 at one point

reubene commented 7 months ago

For Next(5): max-1 gives 0,1,2,3 max gives 0,1,2,3,4 max+1 gives 0,1,2,3,4,5

Not sure which of the last two is the correct one. I think some other methods in the class have similar issues.

Anyway starting a new game it always chooses Romans from [Roman,Celtic,Russian] civs. For other civs it however seems to chose random, as it should. I'll look into it more.

Looks like the correct behaviour is just max max-1 is definitely wrong

axx0 commented 7 months ago

I tried with a couple of different seeds and there's not much improvement. Two problems when e.g. generating 100 random numbers with Next(2):

Out of curiosity, why didn't you use the random class that comes with .net?

reubene commented 7 months ago

Interesting. After correcting it to use straight max I get: 1073741823 of the possible 2147483647 seed values generate a 1 the others generate a 0 for Next(2)

That's exactly how you would expect Next(2) to behave, it's almost exactly half.

With Next(2) you will always get clusters of zeros and ones as they are supposed to appear independent (It's a pseudorandom number generator so they are not really independent)

I wrote this one, based on an algorithm I found online, because I wanted something that could be serialized easily into a readable format and could be run across languages. This is from a multiplayer implementation of Colonization I was working on a few years ago that had a interface in Typescript and a server in C#