Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#4326 closed enhancement (fixed)

[PATCH] Random function cleanup

Reported by: bb Owned by: bb
Priority: Should Have Milestone: Alpha 22
Component: UI & Simulation Keywords: patch
Cc: Patch:

Description (last modified by elexis)

  • 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.

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

Attachments (1)

t4323_random_merge+pickRandom.diff (18.3 KB) - added by bb 3 years ago.
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.

Download all attachments as: .zip

Change History (41)

Changed 3 years ago by 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.

comment:1 Changed 3 years ago by bb

Description: modified (diff)

comment:2 Changed 3 years ago by 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).

comment:3 Changed 3 years ago by 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.

comment:4 Changed 3 years ago by fatherbushido

so all fit well! Add review keyword ?

comment:5 Changed 3 years ago by bb

Keywords: patch review added

Thx that someone is paying attention...

comment:6 Changed 3 years ago by 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.

comment:7 Changed 3 years ago by 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.

comment:8 Changed 3 years ago by 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");

comment:9 in reply to:  7 ; Changed 3 years ago by 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.

comment:10 Changed 3 years ago by bb

Description: modified (diff)

comment:11 in reply to:  9 ; Changed 3 years ago by mimo

Replying to bb:

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.

I don't get you answer: you only would have to use randFloat(23) and not randFloat(23,0) because of the default value of the arguments. And yes, you would have to modify the current calls with two arguments by exchanging their order. Random numbers is a function which should be as fast as possible, and slowing it with a lot of useless ifs and useless recursive calls looks really weird to me. Better do the needed changes once with a good editor.

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.

For the randInt, having the max being the argument or the argument -1 depending on the number of arguments is really error prone. It's better to decide one convention once (i don't mind which one) and fix all calls by adding the +1 when needed, or if you don't like this +1 in the call function, define a second function randIntExclusive or whatever its name with the second convention. But certainly not change the meaning of one argument depending on the number of arguments.

comment:12 in reply to:  11 Changed 3 years ago by bb

Replying to mimo:

Replying to bb:

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.

I don't get you answer: you only would have to use randFloat(23) and not randFloat(23,0) because of the default value of the arguments. And yes, you would have to modify the current calls with two arguments by exchanging their order.

Maybe wrong example but argument still holds for randFloat(23, 1) which is rather confusing, while randFloat(1, 23) isn't.

Random numbers is a function which should be as fast as possible, and slowing it with a lot of useless ifs and useless recursive calls looks really weird to me. Better do the needed changes once with a good editor.

All functions should be as fast as possible IMO, but every function should be readable too. In this way the if's are very usefull and the recursive calls could indeed be changed by Math.random calls but this implementation is just more elegant IMO.

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.

For the randInt, having the max being the argument or the argument -1 depending on the number of arguments is really error prone. It's better to decide one convention once (i don't mind which one) and fix all calls by adding the +1 when needed, or if you don't like this +1 in the call function, define a second function randIntExclusive or whatever its name with the second convention. But certainly not change the meaning of one argument depending on the number of arguments.

Anyone wanting to use this functions should anyway look up its specifics (whatever the convention is) so we could put in any convention with changes for amount of arguments, as long as it is well documented. Adding another useless function for ex/in-clusive max values is completely redundant, because the one argument variant is only called in relation to array's and there we absolutely need to exclude the max. In other cases we want to specify its max directly so we include max. In the way you propose we will end up with some unused code, with is ugly to nuke and ugly to leave in, so we better avoid that.

comment:13 Changed 3 years ago by elexis

Indeed, it should be (min, max), but it would probably be more straight forward to use min=0, max=1 as default arguments in the function and only using randFloat() or randFloat(0, x). It might be confusing that in one case the first argument is the max, in other cases the first argument being min. Furthermore always nuke else after return.

comment:14 Changed 3 years ago by bb

Replying to elexis:

Indeed, it should be (min, max), but it would probably be more straight forward to use min=0, max=1 as default arguments in the function and only using randFloat() or randFloat(0, x). It might be confusing that in one case the first argument is the max, in other cases the first argument being min.

This would leave in a ton of messy "0" arguments, which is less ugly then -1's IMO, though this only works for the randFloat() function, randInt() would hold its specifics (i.o to avoid -1's and a new function is making thing only more confusing). So we would end up with having randFloat() and randInt() with significantly different specifics, which I wouldn't favour.

Furthermore always nuke else after return.

Fixed here: https://github.com/bb-bb/0ad/tree/t4326_random (First and 3rd commit make up this patch)

comment:15 Changed 3 years ago by mimo

I still repeat myself by saying that this implementation is a no-go for me, so better to agree first on it before discussing early return. The fact that it was already used in the random maps is not a justification to continue to use it and extend it to simulation and ai. A cleaning patch should clean and improve, not extend the mess. And I would think than using directly Math.random in the calling functions is stil cleaner than what is proposed here. IMO some basic requirements of a good function are:

  • the meaning of arguments should not change depending on the number of arguments (you have arg1=max when 1 argument while arg1=min when 2)
  • the intrinsic meaning of arguments should also not change (you have max=arg1 when 1 argument but max=arg2-1 when 2)
  • there should not be useless ifs and recursions: calling a simple random int with 2 args, you need 3 RandXXX calls and 7 if conditions! why not also add a delay :-)

All that is really ugly and error prone (and I cannot understand your answer 12 when you argue that this makes them more readable than a simple one line function). And what will you do when you need 2 args with the exclusive requirement? the fact that it is not used presently does not imply it will never be (either by the main game or by a mod).

As I already said, this should stay simple, fast, readable and easy to remember as for example the 3 one-line functions I already proposed:

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

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

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

If people are bothered by the order of the arguments, we can use f(min,max) but that would require calls like f(0,max). Not a big deal, but we could also replace that by f(delta,start) with delta=max-min and start=min which would remove the need for the extra 0 and would be nearer to what is needed in most calling functions. This last one would be my preferred solution.

Finally if having two functions for integer is felt as a problem (which i don't think so as that clearly indicate the intend of the call), then randInt(x,y) and randIntInclusive(x,y) could be merged by adding another argument (rather than changing the arguments meaning) as in

function randInt(delta=1, start=0, inclusive=false)
{
if (inclusive) ++delta;
return Math.floor(start + delta*Math.random());
}

comment:16 in reply to:  15 Changed 3 years ago by bb

Replying to mimo:

I still repeat myself by saying that this implementation is a no-go for me, so better to agree first on it before discussing early return. The fact that it was already used in the random maps is not a justification to continue to use it and extend it to simulation and ai. A cleaning patch should clean and improve, not extend the mess. And I would think than using directly Math.random in the calling functions is stil cleaner than what is proposed here. IMO some basic requirements of a good function are:

  • the meaning of arguments should not change depending on the number of arguments (you have arg1=max when 1 argument while arg1=min when 2)
  • the intrinsic meaning of arguments should also not change (you have max=arg1 when 1 argument but max=arg2-1 when 2)

I really want to avoid repeating myself too...

  • there should not be useless ifs and recursions: calling a simple random int with 2 args, you need 3 RandXXX calls and 7 if conditions! why not also add a delay :-)

I can agree on changing those randFloat() calls to math.Random calls, leaving out the recursion.

All that is really ugly and error prone (and I cannot understand your answer 12 when you argue that this makes them more readable than a simple one line function).

I am not arguing that the randFoo function is more readable but I argue that the about 1k calls to this function will be more readable and I rather have a little more difficult randFoo and easy to understand calls from everywhere in the code.

And what will you do when you need 2 args with the exclusive requirement? the fact that it is not used presently does not imply it will never be (either by the main game or by a mod).

Ofcourse they can be used sometime, but we should limit the number of -1's as they are messy, I am not saying we should entirely ban them, just avoid.

As I already said, this should stay simple, fast, readable and easy to remember as for example the 3 one-line functions I already proposed:

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

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

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

This way we would need to remember 3 functions instead of an easy 2. Falls not under the category "easy to remember" for me.

If people are bothered by the order of the arguments, we can use f(min,max) but that would require calls like f(0,max). Not a big deal, but we could also replace that by f(delta,start) with delta=max-min and start=min which would remove the need for the extra 0 and would be nearer to what is needed in most calling functions. This last one would be my preferred solution.

Finally if having two functions for integer is felt as a problem (which i don't think so as that clearly indicate the intend of the call), then randInt(x,y) and randIntInclusive(x,y) could be merged by adding another argument (rather than changing the arguments meaning) as in

function randInt(delta=1, start=0, inclusive=false)
{
if (inclusive) ++delta;
return Math.floor(start + delta*Math.random());
}

Adding more arguments is an absolute hack and we simply want to call the functions with arguments (min, max) not some strange delta. Calling with a set (min, max) is much easier to read. Putting a boolean to the end will break the possibilty of using (...array) in function call, so that is rather impractical as this can be used for interval array's.

comment:17 Changed 3 years ago by Vladislav Belov

function randInt(max=1, min=0) {

return Math.floor(min + (max-min)*Math.random());

}

BTW, why the max param on left of the min param? Usually random functions use func_name(lower_bound, upper_bound) style.

Version 0, edited 3 years ago by Vladislav Belov (next)

comment:18 Changed 3 years ago by mimo

I'm always impressed by how people jump to argue on minor points just to hide the original problems. All your answers were on code fragments that I gave only as hints how to solve these problems, to which you've not answered. So let's start again on the beginning (although i'm starting to think i loose my time here): 1) do you believe that having a JS function which reacts differently according to the number of arguments (in our case exclusive range if 0 or 1 args, and inclusive range if 2 arguments) is good programming practise, and that this is cleaner than having two different functions with explicit names? 2) do you believe that having a JS function which has a given parameter (here the max range) in different position depending on the number of arguments is good programming practise?

comment:19 Changed 3 years ago by bb

Also these points are actually minor issues. I could ask you the same questions like "do you like having -1's all over the code?" or a function call like "(max, min)?" etc. Answers to all these questions is "no" whoever you ask them (even the stuborn people like myself :-S). So the thing we disagree about is "how ugly" every problem is and what would be the overall best compromise. In my opinion having the calls to these functions cleaner is way more important than an easy one-liner or fixed arguments. You might have another opinion, which I absolutely respect, only disagree with.

The thing this ticket is about IMO is the fact that the random functions are a bloody mess and we want to compress that into two as simple as possible functions but still easy in use too. For this both functions should IMO have at least (min, max) in function call and the implementation of boundary's should avoid -1's. When we work this out it seems that our "basic requirements" cannot be compressed into one implementation. For randFloat() we could by "allowing" (0, value) as parameters and then skip the whole one argument part (I don't like this implementation entirely but it isn't too worse either). But for randInt() I really don't see a great solution, taking into account all issues stated by both of us.

The fact that we disagree on some points does not mean I do not read or value your opinion and I really hope we can find a solution we both can agree on :)

comment:20 Changed 3 years ago by elexis

No recursion, no if-statements in the randomfunctions, min max order, no changing of argument meaning depending on argument count, make it happen.

comment:21 in reply to:  19 ; Changed 3 years ago by mimo

Replying to bb: These are not minor issues, because that is the basic requirements to have a clean set of functions that you can use without always having to refer to the code (as you would need to do when parameters order or meaning of parameters vary). Then for the RandInt?, what's the problem with having two functions, one inclusive and one exclusive? If both cases are needed by the calling functions, we should provide them. And nothing forces us to do that in a hacky way with only one function depending on the number of parameters. Using two different functions, you remove all need of adding any +1 in the calling functions (just call the inclusive one in that case). And sorry if i overreacted in my previous answer, but i had the impression we were going nowhere.

comment:22 in reply to:  21 Changed 3 years ago by bb

Replying to mimo:

Then for the RandInt?, what's the problem with having two functions, one inclusive and one exclusive? If both cases are needed by the calling functions, we should provide them. And nothing forces us to do that in a hacky way with only one function depending on the number of parameters. Using two different functions, you remove all need of adding any +1 in the calling functions (just call the inclusive one in that case).

I rather have -1's all over code then having 2 functions almost doing the same, since then one would need to remember the implementation of both function as 1 will be included and one excluded and what is randInt be then? So I would still end up grepping code and being confused by which function would be correct for my use case. Also a fast grep through code yields no use cases for a randInt(min, max) with min!=0 and max excluded and we rather set the max directly then setting a max which is excluded except when we use so array length but then we want a randInt(0, max-1), so we only need a one argument function randInt(max) for the excluded version. Adding a second argument is redundant as stated above and would confuse due to the 2 very similar functions.

So if we need 2 randInt function I rather have randInt(min=0, max=1) (with included min, max) and a randIntExcludedMax(max) (with included 0, excluded max). This function can then be a oneliner again.

Another approach would be to rename the randInt function to randIntInclusive{Max}(min=0,max=1) and randIntExclusive{Max}(min=0, max=1), which would also avoid all confusing (So there wouldn't be a randInt function anymore). We could decide upon "Max" is necessary in function name. On this randIntExclusive{Max}(min=0, max=1) not sure if that (min=0, max=1) is really needed since randIntExclusive{Max}() would then return 0 always, very random...

comment:23 Changed 3 years ago by elexis

Milestone: Alpha 22Work In Progress

Moving to the new WIP milestone.

comment:24 Changed 3 years ago by bb

Keywords: rfc added; review removed

Implementation of two functions for randInt: https://github.com/bb-bb/0ad/tree/t4326_random

comment:25 Changed 3 years ago by elexis

  • I agree with the approach of using randIntInclusive/randIntExclusive and that the optional arguments should become mandatory.
  • Avoid the recursion in random.js as the functions will be called often enough (using the correct formula in the first place does barely need more characters in this case and reading the final formula is easier than looking up the one function)
  • pickRandom could be moved to random.js and receive a JSdoc comment hinting that it wants an array. Could that become shorter with an x[y] || undefined?
  • chatHelper.js could look like g_DiplomacyMessages of messages.js (a pattern which allows modders to change these arrays externally without touching the code)
Last edited 3 years ago by elexis (previous) (diff)

comment:26 Changed 3 years ago by mimo

Thanks for the new patch which looks much cleaner. I've just a few additional comments on top of elexis' ones:

globalscripts/random.js

line 4 "and" should be removed

session.placement.js

I would have used randIntExclusive(0, 65535) instead of randIntInclusive(0, 65534)

but that's just a personnal preference

chatLaunchAttack: the additionnal condition on proba when HugeAttack? was mostly because i was short of idea for a good second sentence, so I reused the ones from normal attacks. It would be better to add another sentence and remove this extra condition on proba. And if you also have nice alternate sentences for all these AI dialogs, feel free to add them to make more diversity.

chatAnswerRequestAttack: I'm not a fan of interlinked ternaries which i find difficult to read. I prefer an if+ternary

headquarters: i don't think a "" class would work. A working way would be to reuse one of the already present class as "classes.push(pickRandom(["Ranged", "Melee", "Infantry"]));" or modify gamestate.findTrainableUnits to use MatchesClassList? instead of hasClass and thus use an OR as in "classes.push(pickRandom(["Ranged", "Melee", "Ranged Melee"]));"

comment:27 in reply to:  26 ; Changed 3 years ago by bb

Description: modified (diff)

Replying to mimo:

session.placement.js

I would have used randIntExclusive(0, 65535) instead of randIntInclusive(0, 65534)

but that's just a personnal preference

I did change the number because now we set the maximum directly and maximum + 1, but when its better/cleaner to have this another way, I wouln't mind. (Changed the value in GIT for now to 65535)

chatLaunchAttack: the additionnal condition on proba when HugeAttack? was mostly because i was short of idea for a good second sentence, so I reused the ones from normal attacks. It would be better to add another sentence and remove this extra condition on proba. And if you also have nice alternate sentences for all these AI dialogs, feel free to add them to make more diversity.

I think its better to split this of into another patch/ticket (patch is already big enough), that hugeAttack thingy can then be changed aswell. (But fully agree, that it should be done.)

chatAnswerRequestAttack: I'm not a fan of interlinked ternaries which i find difficult to read. I prefer an if+ternary

I like the ternary's better here, since we don't need some variable declaration, maybe with inlined strings it would have been a bit unreadable but since they have moved to the top of the file, I don't see why this is unreadable.

comment:28 Changed 3 years ago by elexis

That the previous term Math.floor(65535 * Math.random()) and the new terms equal to that (randIntExclusive(0, 65535) and randIntInclusive(0, 65534)) might be wrong (off by 1) in the first place didn't bother anyone? bb did some testing after I mentioned it to him and it didn't crash. Looking it up in the code, we see the value being used at cmpVisual.SetActorSeed(cmd.actorSeed) which then again uses u32, so it's off by one and by a factor of 216 even. The factor might an issue with Atlas using unsigned int in L293 of ObjectHandlers.cpp though.

comment:29 in reply to:  27 Changed 3 years ago by mimo

Replying to bb:

Replying to mimo:

session.placement.js

chatAnswerRequestAttack: I'm not a fan of interlinked ternaries which i find difficult to read. I prefer an if+ternary

I like the ternary's better here, since we don't need some variable declaration, maybe with inlined strings it would have been a bit unreadable but since they have moved to the top of the file, I don't see why this is unreadable.

Better readability is something subjective, so i may understand that some people like them. But just browse the web and see what are the majority of comments about them: the response is most often "avoid such interlinked ternaries". For me, the important think for a readable code is to be able to have an idea of what it does at a glance, and that is certainly not true with interlinked ternaries where you have to concentrate your attention to them to understand what they do. And keeping a few intermediate variables when they help the readability is usually good practise imo.

comment:30 Changed 3 years ago by elexis

with interlinked ternaries where you have to concentrate your attention to them to understand what they do

We're talking about an a ? b : c ? d : e here, it's as complicated as an if-elseif-else. Take a look for example at the ternary in init of summary.js. Might look frightening when never being confronted with ternaries before, but it's nothing else but an if-elseif-elseif-...-else chain.

Since ternaries are a basic logical operator present in many languages, the reader should be familiar with them. I agree that if it's not a linear concatenation but something nested that this should be dealt with (but in the form of moving code to new functions or removing logic, since nested if's are just as terrible to grasp).

That being said, I still don't mind the use of if + helper variables if that makes things easier to read.

comment:31 Changed 3 years ago by mimo

In 19109:

petra: cleanup of chat messages based on patch by bb, refs #4326

comment:32 Changed 3 years ago by bb

comment:33 Changed 3 years ago by elexis

Keywords: rfc removed

comment:34 Changed 3 years ago by elexis

In 19270:

Unify random integer and float helper functions of GUI, Simulation and AI.

Patch By: bb
Differential Revision: D121
Refs: #4326

Removes the Random.js simulation helper and randomFloat function of the random map scripts library.
Adds randomIntInclusive and randomIntExclusive to make the calls more readable and fix and prevent off-by-one mistakes.
Adds randBool and use it in an AI occurance. It will be used in many places by the random map scripts.
Use the pickRandom function introduced in r19109 in more applicable occurances.
Replace remaining occurances of Math.random() with the new functions to easily test completeness.

Cleanup of the random map script functions will come in a separate commit.

comment:35 Changed 3 years ago by elexis

In 19305:

Use pickRandom in random map scripts.
Avoids helper index variables, replacing deprecated randInt calls, making the code shorter and nicer to read.

Patch By: bb
Differential Revision: https://code.wildfiregames.com/D222
Refs #4326 https://code.wildfiregames.com/D121

comment:36 Changed 3 years ago by elexis

In 19355:

Replace many deprecated randInt calls with randBool.
Add optional probability to randBool to receive true and use where applicable.

Patch By: bb
Differential Revision: https://code.wildfiregames.com/D235
Refs #4326 D121

comment:37 Changed 2 years ago by elexis

In 19443:

Replace deprecated randInt calls (besides those in the Unknown maps).

Differential Revision: https://code.wildfiregames.com/D278
Patch By: bb
Refs #4326

comment:38 Changed 2 years ago by elexis

In 19464:

Remove optional arguments from randFloat, since the function can be called from the simulation and that should explicitly specify arguments if the defaults aren't entirely self-evident.
Make a lot of calls easier to read.

Differential Revision: https://code.wildfiregames.com/D372
Patch By: bb
Refs #4326 D121

comment:39 Changed 2 years ago by elexis

Resolution: fixed
Status: newclosed

In 19721:

Complete the move of random number helper functions from the random map library (rP9096, refs #6) to globalscripts.

Differential Revision: https://code.wildfiregames.com/D596
Patch By: bb

rP19109 introduced pickRandom and its petra chat application,
rP19270 used the randFloat function more often, introduced randBool and randIntInclusive/Exclusive

to replace randInt (whose behavior and argument meaning depended on the number of arguments) calls in the GUI and simulation,

rP19305 changed randInt to pickRandom for random maps,
rP19355 introduced randBool with a probability and replaced deprecated randInt calls for random maps,
rP19443 replaced randInt calls with randIntInclusive/Exclusive for random maps except the Unknown,
rP19464 removed optional arguments of randFloat,
this commit replaces the remaining occurances in the Unknown maps,
thus fixes #4326 (old patch from refs #3102).

comment:40 Changed 2 years ago by elexis

Description: modified (diff)
Milestone: Work In ProgressAlpha 22
Priority: Nice to HaveShould Have

Thanks for the many cleanups bb!

Note: See TracTickets for help on using tickets.