[PATCH] Random function cleanup

[bb]

• In current code there exist some duplicate function generating random numbers, it would be good to merge those into one globalscripts file.

• Also it would be good to have a randomPick function to randomly pick an element from an array, so we don't need to do shuffle array and then pick array[0], and thus save some performance.

• This new pickRandom function can the be used in instances like if (randNumber >0.5);else;

• Occurrences of Math.round(Math.Random()) and similar expressions should be nuked into some RandInt() calls.

• Occurrences of randFloat(0, value) should be nuked to randFloat(value) as that returns is the exact same interval and saves some performance.

The attached patch is a rebase from #3102 and this ticket is necessary for the final implementation of that ticket.

[bb]

Rabse form #3102, I would be glad to split the nuked function from the pickRandom(), but as the calls to pickRandom are mainly old calls to GetRandom and that last function got nuked, it seems better to merge it. So we don't change lines twice and the patch isn't *that* big. Also didn't add a messy "-1" in UnitAI because adding 1 ms to the random feed/walk time doesn't seem to matter.

[fatherbushido]

That patch is a very nice cleanup.

In common randInt functions, there are mainly two conventions for randInt(c) and randInt(a,b): having the same interval length or use the one used in the patch. I often thought the second one (ie the one in the patch) is less misleading but I don't know what is better for our use.

For the unitAI stuff it's a bit disturbing that the number written/expected in the template will not be the one in the code (Adding -1 everywhere would be bad too).

[bb]

When looking strictly into this, the number in current code is confusing. Because now the actual max time is the (templateMax-1). With this patch it will get the templateMax so we skip the minus 1!! So then we use the number written in template, more like I would expect.

[fatherbushido]

so all fit well! Add review keyword ?

[bb]

Thx that someone is paying attention...

[elexis]

There are many occurances which should also use pickRandom. It would be cleaner to have it in two diffs, one introducing the randomFloat randomInt part and the other fixing all occurances of pickRandom. But if the patch isn't broken and all occurances will be fixed afterwards, that would work too.

[mimo]

I don't like all these if conditions in the function definitions. Wouldn't it be better written as

function randFloat(max=1, min=0)
{
return min + (max-min)*Math.random();
}

function randInt(max=1, min=0)
{
return Math.floor(min + (1+max-min)*Math.random());
}

Also, i'm not at all convinced by the use of pickRandom when the array does not preexist like is the case for your changes in the ai chat code. That adds a lot of useless steps (creating all strings, create an array with them and pick one randomly) compared to just choosing a random number and create one string according to its value. Futhermore, imo the additional use of ternary operator does not help the readability of all these expressions. The worst being these two nested ternary lines 24 to 31.

[elexis]

Moving these arrays to the top of the file like g_DiplomacyMessages in messages.js would prevent the redundant array construction and allow adding more variations without touching the code and doing any of the random computations by hand.

Those setSkySet calls in maps/random/ would greatly benefit from pickRandom. It's called only once, so array creation shouldn't be an issue. We could have that array at the top of the file near the other consts, but that's likely not needed at all if we can have one simple pickRandom(["cirrus", "cumulus", "sunny"]) instead of

random_var = randInt(1,3);

if (random_var==1)
	setSkySet("cirrus");
else if (random_var ==2)
	setSkySet("cumulus");
else if (random_var ==3)
	setSkySet("sunny");

[bb]

Replying to mimo:

I don't like all these if conditions in the function definitions. Wouldn't it be better written as

function randFloat(max=1, min=0)
{
return min + (max-min)*Math.random();
}

function randInt(max=1, min=0)
{
return Math.floor(min + (1+max-min)*Math.random());
}

We really don't want IMO function calls like randFloat(23, 0) as in (max, min) instead of a straightforward (min, max). That is way more confusing especially since these calls are all over the code and I rather have one little more difficult function and about 1k easy to understand function calls.

Besides that the in/ex-clusion of the max value (especially in randInt()) seems to reduce most of all +/-1's when calling. This is because we can have [0, value] aswell as [0, value-1] as output without putting -1 in call (this is done with calling randInt(0,value) or randInt(value)). This last thing is obviously not possible in one-liners as above. IMO adding those ugly -1 is again more confusing then a few if's.