Add RandomNumberGenerator::rand_weighted method

[joaoh82]

This is the implementation for Add a method to get a random element with weighted probability (similar to random.choices() in Python) as in this Proposal.

Usage

It was added to the core classes as part of the RandomNumberGenerator class, where you pass a PackedFloat32Array to it as the weights.

var rnd = RandomNumberGenerator.new()

var my_array = ["one", "two", "three, "four"]
var weights = PackedFloat32Array([0.5, 1, 1, 2])

# May prints one the four elements in `my_array`.
# It is more likely to print "four", and less likely to print "two".
print(my_array[rng.rand_weighted(weights)])

I also added a test to check if size of both arrays match.

• Bugsquad edit, closes: Add a method to get a random element with weighted probability (similar to random.choices() in Python) godot-proposals#3948

[AThousandShips]

You used a merge commit to update your branch, please use rebase in the future instead, see here

You also need to use force pushing to update your branch, with git push -f, all this is in the link above for the workflow :)

[Mickeon]

Worth considering whether or not the weights parameter could be a PackedFloat64Array or even PackedFloat32Array. If it needs to be a number I don't see why not to, at a glance. GDScript would also do the conversion from an Array implicitly if it can.

Second, perhaps the method should be called pick_weighted_randomto match pick_random.

And finally, the fact that there's no equivalent in the RandomNumberGenerator class means more "experienced" users can't use it, for predictable RNG with known seeds.

[joaoh82]

@Mickeon thanks for the review!

Worth considering whether or not the weights parameter could be a PackedFloat64Array or even PackedFloat32Array. If it needs to be a number I don't see why not to, at a glance. GDScript would also do the conversion from an Array implicitly if it can.

I can definitely make this change. Do you think it would perform better or not really?

Second, perhaps the method should be called pick_weighted_randomto match pick_random.

Makes sense. It would make it more straight forward to the developer I guess.

And finally, the fact that there's no equivalent in the RandomNumberGenerator class means more "experienced" users can't use it, for predictable RNG with known seeds.

Not really sure what you mean here. Can you clarify what would be the course of action here?

[Mickeon]

I can definitely make this change. Do you think it would perform better or not really?

The performance benefit would come from not having to check for the type of every element being a float, which is quite inefficient and rather silly because PackedFloat32Array would basically guarantee it.

Can you clarify what would be the course of action here?

Okay, so. RandomNumberGenerator is a class that can be used to hold self-sustaining random number generators that do not influence one another.
There is no pick_random in RandomNumberGenerator, but that is okay because it is very simple to recreate. For weighted randomness however, having it present in that class may actually be desirable. It is a more complex algorithm that may also require fine-grained control at times (think of roguelikes and random loot generation for example).

[joaoh82]

@Mickeon I took the advice of @smix8 and moved the logic to the RandomNumberGenerator taking a single parameter with a PackedFloat32Array for the weights that returns the picked index. I think it more generic and can be used by anything really.

[AThousandShips]

Might be good to add some tests, but matches the code referenced in godotengine/godot-proposals#3948 (comment)

Looks good and makes sense, some further feedback from the core team would be good though

[Mickeon]

The opportunity to have this in core excites me because there may actually be a faster algorithm out there but its obscurity would mean a "common" developer using the engine may not implement it this way and benefit from it.

I've known about it for a while. If/when this PR is merged I'd love to take a stab at trying to implement it.

[AThousandShips]

The specific implementation is just that, an implementation detail, so we're free to improve it in the future :)

[joaoh82]

Absolutely. We can always iterate on it and improve it at any time.

[akien-mga]

Could you squash the commits? See PR workflow for instructions.

And while doing it, please edit the commit message to match the title of this PR / the new implementation.

[joaoh82]

all done! @akien-mga

[joaoh82]

I was wondering if I should also add it to the C# API and added it to the Math namespace with the other random methods. @akien-mga @Mickeon @AThousandShips

[Mickeon]

It's no longer a requirement since it belongs to RandomNumberGenerator now, and it's how this method can be accessed in C#. I think the PR is completely fine as is.

[akien-mga]

Thanks! And congrats for your first merged Godot contribution 🎉

[joaoh82]

Thanks for the help all!! Really happy to have contributed!

[luevano]

Hey, I'm looking through the code as I wanted to propose a change/addition related to this PR and the Array::pick_random and noticed the following:

RandomPCG::rand_weighted returns int but the value itself that it's returned (i) is defined as an int64_t.

Shouldn't it be int64_t RandomPCG::rand_weighted?

[guladam]

Hey everyone,
This was a welcome addition to the core RNG functions in Godot.
I'm just wondering, is there any specific reason why this method is not available in the Global Scope?

[Mickeon]

Because (if I recall correctly) we agreed that when working with weighted randomness it would be much more beneficial to have more granular control over it, and RandomNumberGenerator allows this. Although, I don't believe there's much stopping this from being in the Global Scope, as well.

[joaoh82]

yeah. what @Mickeon said.