Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exposed randi_range to global funcs + renamed rand_range to randf_range #40718

Merged
merged 1 commit into from
Nov 6, 2020

Conversation

Chaosus
Copy link
Member

@Chaosus Chaosus commented Jul 26, 2020

I see a lot of user help requests in Discord which has troubles with randi_range usage. Currently, it required to create an instance of RandomNumberGenerator, which may be uncomfortable for simple use cases. So I've added randi_range to GDScript global functions, and renamed rand_range to randf_range (for logic compatibility with other functions + @akien-mga asked me to do that a while ago).

The last change will probably break a lot of user scripts, so it's all up for @reduz and other core contributors decision

@aaronfranke
Copy link
Member

This change makes sense, except that it would also make sense to go in the other direction, removing methods from global scope. Personally I like having these methods available due to how common they can be in projects, but it's worth a discussion. See also godotengine/godot-proposals#1590 for a discussion about removing the seed methods from global scope.

modules/mono/glue/gd_glue.cpp Outdated Show resolved Hide resolved
@Chaosus Chaosus force-pushed the rand_range branch 2 times, most recently from a0f3902 to 2b2c756 Compare October 29, 2020 06:56
@Chaosus Chaosus requested review from neikeq and removed request for bojidar-bg October 29, 2020 07:02
@Chaosus
Copy link
Member Author

Chaosus commented Oct 29, 2020

@aaronfranke I agreed with both directions, so either we choose one or another - I would be happy, I think @reduz should decide :)

@Chaosus Chaosus force-pushed the rand_range branch 4 times, most recently from e7d9c09 to 20d6151 Compare October 30, 2020 14:32
@Chaosus Chaosus requested a review from neikeq October 31, 2020 06:47
core/math/expression.h Outdated Show resolved Hide resolved
core/math/math_funcs.h Outdated Show resolved Hide resolved
@Chaosus Chaosus force-pushed the rand_range branch 5 times, most recently from a1db728 to a368071 Compare November 6, 2020 13:23
@Chaosus Chaosus requested a review from akien-mga November 6, 2020 13:29
Comment on lines +78 to +79
MATH_RANDF_RANGE,
MATH_RANDI_RANGE,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're up for one last refactoring, it would be good to use the opportunity to change the order here:

MATH_RANDI
MATH_RANDF
MATH_RANDI_RANGE
MATH_RANDF_RANGE

(i.e. int first, then float for the ranged version too)

And then adapt the order in all places where these enums are used to keep the same order as the enum.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or the other way around (float, then int) if we want alphabetical order instead:

MATH_RANDF
MATH_RANDI
MATH_RANDF_RANGE
MATH_RANDI_RANGE

@Chaosus
Copy link
Member Author

Chaosus commented Nov 6, 2020

@akien-mga I could make a randi_range exclusive, the advantage of inclusiveness - it's a more intuitive for raw usage, the disadvantage: things like arrays indexes or enums need to be cast down by 1 like randi_range(0, array.size() - 1) in order to generate value within its range.

@akien-mga
Copy link
Member

I could make a randi_range exclusive, the advantage of inclusiveness - it's a more intuitive, the disadvantage: things like max array sizes or enum need to be cast down like randi_range(0, array.size() - 1) in order to correct processing.

Yeah I'm not sure what's best myself, exclusive would be like range(n) which gives n values between 0 and n - 1, but maybe the use case for getting 1 random number in a range is not exactly the same. You have a good point about getting a random index in an array though, but I think programmers are quite used to doing array.size() - 1 for indexing, so keeping it inclusive might be best?

@Chaosus
Copy link
Member Author

Chaosus commented Nov 6, 2020

@akien-mga Also, I think the description of randf is incorrect - it actually exclusive and not inclusive - it generates value from 0.0 to 0.999999 (but never hits 1.0).

@akien-mga
Copy link
Member

Also, I think the description of randf is incorrect - it actually exclusive and not inclusive - it generates value from 0.0 to 0.999999 (but never hits 1.0).

That's probably due to the conversion from uint32_t to float which introduces precision errors (at least in the !defined(CLZ32) implementation (float)(rand() & 0xFFFFFF) / (float)0xFFFFFF;).

But yeah, I don't think a range in floating point can ever be expected to be inclusive anyway.

@akien-mga akien-mga merged commit 391d29f into godotengine:master Nov 6, 2020
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants