MakeRandom bug crashing Linux

[John Adams]

I ran into a problem with our MakeRandom() function today, which seems to work great for whole numbers but blew up soon as I tried to get a random value from a decimal. I sent in MakeRandom(0.94, 1) and Linux would explode. Windows, of course, loves it.

Upon investigating the function, it appears the srand() call is trying to turn my "diff" of 0.05 into an (int) (whole number) which resulted in zero, thus the arithmetic exception.

Code: Select all

srand((uint32_t)time(0) * (time(0) % (int)diff));

How I resolve it was this, though I not confident it was the best approach.

Code: Select all

float MakeRandom(float min, float max) {    static bool seeded=0;    float diff = max - min;    int seed_diff = 0; // need to handle < 1.0 min/max    if(!diff) return min;    if(diff < 0)        diff = 0 - diff;    if((int)diff == 0)        seed_diff = 1;    else        seed_diff = diff;    if(!seeded)    {        srand((uint32_t)time(0) * (time(0) % (int)seed_diff));        seeded = true;    }    return (rand() / (float)RAND_MAX * diff + (min > max ? max : min));}

I added a "seed_diff" that is an int, the check if the original diff is 0, and if so use the seed_diff hardcoded to 1 instead. Otherwise, the function should behave as before.

If anyone has a better suggestion, please let me know.

[Ratief]

Why do "srand((uint32_t)time(0) * (time(0) % (int)seed_diff));" at all? Isn't "srand((uint32_t)time(NULL));" good enough? The time value will be unique for each server start, adding an additional multiply still keeps it unique (it's a multiple of time(NULL) (*2 or greater), but unique is unique. So is there some reason that I'm missing to add the multiplication?

[John Adams]

You'll have to ask Xinux.

Code: Select all

/**
* @brief function to Generates a random float
*
* @author Xinux
* @date 9 Jan 2015
*/

I'm good with cleaning it up and removing the excess junk as that will solve my problem (maybe?)

[Ratief]

For when Xin gets around to reading this. I'm not 100% sure what is being returned by this function yet as I haven't decoded the last line, but I'm guessing that it is a random float number between min and max. The ?: actually makes it a bit hard to read and I would suggest (assuming that my assumption above is correct) would be to make a check earlier for min > max and set min=max. That way the return statement would be simplified to only have +min in it.

[Arutam]

A suggestion (untested):

Code: Select all

void InitRandom() {
    static bool initialized = false;
    if(!initialized)
    {
        srand((uint32_t)time(0));
        initialized = true;
    }
}

/**
* Returns a random float within the range of the lower (inclusive) and higher (exclusive) boundary.
*/
float MakeRandom(float boundary1, float boundary2) {
    float high = boundary1 > boundary2 ? boundary1 : boundary2;
    float low = boundary1 > boundary2 ? boundary2 : boundary1;

    if(low == high)
        return low; // exit early, no need to call rand()

    InitRandom();

    return low + ((float)rand() / RAND_MAX) * (high - low);
}

As Ratief already pointed out the calculation for seeding is most likely not necessary. By separating initialization and randomization you can decide to call InitRandom() at server start as well.

[Faux]

Here is another way to get a random in a specific range (yes, I found it on stack overflow):

Code: Select all

// Add on to the base damage with a random number generated from the bonus range.
	std::random_device rd; // obtain a random number from hardware
    std::mt19937 eng(rd()); // seed the generator
    std::uniform_int_distribution<> distr(min_bonus, max_bonus); // define the range

	base_damage += distr(eng);
	LogDebug(LOG_COMBAT, 5, "Base damage is %i.", base_damage);

So far it has seemed to work really well and in limited test cases it has not overly repeated any one value. Food for thought.