adding Random.choices()

[rusty-tendrils]

One of the features approved in #7

[Xrayez]

Missing bind argument:

Suggested change

	ClassDB::bind_method(D_METHOD("choices", "from", "count", "weights"), &Random::choices, DEFVAL(1), DEFVAL(Variant()), DEFVAL(false));
	ClassDB::bind_method(D_METHOD("choices", "from", "count", "weights", "cumulative"), &Random::choices, DEFVAL(1), DEFVAL(Variant()), DEFVAL(false));

[Xrayez]

Prepend p_ for parameters (Godot naming convention).

Also, given that the code could be used in C++ as well, it's better to provide default arguments in the method signature as well.

The following suggestion applies both ideas:

Suggested change

	Variant choices(const Variant &p_sequence, int count, const Array &p_weights, bool is_cumulative);
	Variant choices(const Variant &p_sequence, int p_count = 1, const Array &p_weights = Variant(), bool p_cumulative = false);

Don't forget to do the same to cpp file.

[Xrayez]

If weights/indices can only be integers, I think you could use PoolIntArray instead (both here and as input). If you pass Array, Godot will do the conversion to PoolIntArray automatically. I think this will actually simplify validation down the road.

[rusty-tendrils]

How do I convert an Array to PoolIntArray ? I would need it for getting dict.values() .
It says no suitable user-defined conversion from 'Array' to 'PoolIntArray' exists

[Xrayez]

Hmm yeah, looks like Godot doesn't provide such conversion. I'd just initialize in a loop.

Variant Random::choices(const Variant &p_from, int count, const PoolIntArray &p_weights, bool is_cumulative) {
	int sum = 0;
	PoolIntArray cumulative_weights;
	PoolIntArray weights;
	PoolIntArray indices;
	PoolIntArray weighted_choices;

	if((p_from.get_type() == Variant::DICTIONARY) && p_weights.empty()){
		Dictionary dict = p_from;
		Array w = dict.values();
		for (int i = 0; i < w.size(); ++i) {
			weights.push_back(w[i]);
		}
	} else {
		weights = p_weights;
	}

There will be other missing functions like back(), you'd have to access the last element manually.

[Xrayez]

Note that size() and length() are different in String. size() includes null character, while length() doesn't. Not sure whether this logic matters here though. But if it does, you'd likely need to use str.length() instead.

[Xrayez]

Suggested change

	Variant Random::choices(const Variant &p_from, int p_count, const PoolIntArray &p_weights, bool p_is_cumulative) {
	Array Random::choices(const Variant &p_from, int p_count, const PoolIntArray &p_weights, bool p_is_cumulative) {

[Xrayez]

I think casting is no longer necessary.

Suggested change

	if(int(weights[i]) < 0) {
	ERR_FAIL_V_MSG(Array(), "Weights must be positive integers.");
	} else {
	sum += (int)weights[i];
	cumulative_weights.push_back(sum);
	}
	if(weights[i] < 0) {
	ERR_FAIL_V_MSG(Array(), "Weights must be positive integers.");
	} else {
	sum += weights[i];
	cumulative_weights.push_back(sum);
	}

[Xrayez]

Same as above.

Suggested change

	if(int(cumulative_weights[mid]) < random_number) {
	if(cumulative_weights[mid] < random_number) {

[rusty-tendrils]

Is returning an Array (instead of a String) a good idea when the input is a String ?
I cannot imagine what it could be used for in the context of games.

[Xrayez]

Well, I think String may not be used as often and it's probably specific, but it's a sequence type so better to have support for it. See "Completeness" development philosophy in Goost.

Therefore, having the method return an Array is better in terms of auto-completion and documentation for common cases. Besides, this is what Python does as well it seems:

[Xrayez]

Thanks and congrats for your first contribution!

[rusty-tendrils]

Thanks, I learnt a lot.

[filipworksdev]

Nice! I like the cummulative bool approach. I'm still here just migrated all my data to a brand new account (with the same name) :)