godot icon indicating copy to clipboard operation
godot copied to clipboard

Godot 4.0 beta 7 - randi() in Vector2i() produces negative numbers

Open Pizza-Script opened this issue 1 year ago • 6 comments

Godot version

v4.0.beta7.official [0bb1e89fb]

System information

Windows 10

Issue description

When using randi() in Vector2i(), it produces negative numbers. Discovered this, when trying to generate a random position in the viewport.

Steps to reproduce

Kindly see code below.

Minimal reproduction project

extends Node2D

func _ready() -> void:

	print("--- TEST A ---")
	for i in range(10):
		var test_a: Vector2i = Vector2i(randi(), randi())
		print(test_a)

	print("--- TEST B ---")
	for i in range(10):
		var test_b: Vector2i = Vector2i(randi(), randi()) % Vector2i(640, 360)
		print(test_b)

	print("--- TEST C ---")
	for i in range(10):
		var test_c: Vector2i = Vector2i(randi() % 640, randi() % 360)
		print(test_c)

Results :

--- TEST A --- (1806296788, -1897042084) (983833438, 1443881349) (1894031998, -1861650848) (-141651958, -1576647642) (-1127344993, -1328438374) (-1643842200, -959493189) (906330563, 1402981330) (-736853035, 1510943120) (-1572733590, 1720123564) (-138079385, 711575314)

--- TEST B --- (208, -353) (151, -131) (-120, -52) (-267, 177) (-277, 240) (-510, -287) (-513, -121) (-596, -158) (-294, -60) (-139, -195)

--- TEST C --- (39, 86) (441, 351) (462, 314) (546, 339) (610, 110) (170, 43) (490, 117) (457, 100) (630, 232) (23, 298)

Pizza-Script avatar Dec 06 '22 16:12 Pizza-Script

This is probably intentional, perhaps desired.

Vector2i and similar uses 32-bit integers for each component. Godot's int type is a 64-bit integer. If you only limit it to positive numbers, you may only get 2 ^ 16 possible random numbers, from 0 to 65536. For random, general use numbers, that may be too low, especially in comparison to the 4,294,967,296 possibilities for the signed 64-bit int.

Mickeon avatar Dec 06 '22 16:12 Mickeon

The 32-bit integer overflow in Vector*i types should probably generate a warning message as it's undesired in most situations. (If the check is too expensive, it can be skipped in release builds.)

Calinou avatar Dec 06 '22 16:12 Calinou

That's what I thought first, but according to the docs, randi() produces 32-bit integers? https://docs.godotengine.org/en/latest/classes/class_%40globalscope.html#class-globalscope-method-randi

Pizza-Script avatar Dec 06 '22 16:12 Pizza-Script

The docs seem wrong/outdated:

core/math/random_number_generator.h
54:     _FORCE_INLINE_ uint32_t randi() { return randbase.rand(); }
58:     _FORCE_INLINE_ int randi_range(int p_from, int p_to) { return randbase.random(p_from, p_to); }

core/variant/variant_utility.cpp
653:    static inline int64_t randi() {
665:    static inline int64_t randi_range(int64_t from, int64_t to) {

It's a 64-bit signed integer in the global scope, and the RandomNumberGenerator function does return a 32-bit integer, but unsigned, so it could still overflow in Vector2i's signed 32-bit integer.

Edit: Mickeon is correct below. The signature is int64_t simply to be able to fit the uint32_t from Math::rand().

Vector2(randi() / 2, randi() / 2) might be a suitable workaround to avoid overflowing. Or alternatively Vector2(randi(), randi()).abs().

akien-mga avatar Dec 06 '22 16:12 akien-mga

Yeah, it's technically correct. Just poorly explained. It says 32-bit unsigned integer. That means the leading bit of the integer (the one on the far left) is not used to denote whether or not the number is negative.

What randi does is return a 32-bit, unsigned integer, which Godot's int has no problem putting in the positive range with plenty of space to spare, because it has got 64-bits to work with. However, Vector2i uses 32-bit signed integers, so there's a random chance for the leading bit to to be flipped and result in a negative number.

The first number you got above, 1806296788, is actually the following in binary:

0110 1011 1010 1001 1110 0110 1101 0100

The second number, -1897042084, is this:

1111 0001 0001 0010 1001 0000 1010 0100

Mickeon avatar Dec 06 '22 16:12 Mickeon

If it helps.. if you have a uint32_t rand in the full range, and you want positive only int32_t values, you can bitwise AND the uint32_t with ~(0b1 << 31) (to unset the most significant bit) then cast to int32_t.

But usually a randi() for int32_t I would personally expect to be positive or negative, rather than just positive. That's kind of the point of signed integers.

lawnjelly avatar Dec 06 '22 18:12 lawnjelly

I see, I understand the issue a bit more now (did not know what signed and unsigned means).

@lawnjelly on the randi() docs, it specifically mentions - randi() # Returns random integer between 0 and 2^32 - 1

So I assumed all results should be positive, which technically is correct, since randi() is unsigned.

But I guess when used in Vector2i 32-bit signed integers, the negative result does make sense.

Thank you.

Pizza-Script avatar Dec 06 '22 18:12 Pizza-Script

Aside from the discussion of what the resulting Vector2i should be, note that if you desire the result to be positive you may wish to use the posmod operation instead. Also note that this is currently missing from Vector2i/3i/4i but it's present in Vector2/3/4 - we should add it to Vector2i/3i/4i at some point.

aaronfranke avatar Dec 06 '22 19:12 aaronfranke

If I may add, this seems like an issue of poor documentation to note down for a next tweaking pass.

Mickeon avatar Dec 07 '22 18:12 Mickeon