godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
90.9k stars 21.14k forks source link

GDScript `range(0, 1, 0.1)` returns an empty array #58210

Open tavurth opened 2 years ago

tavurth commented 2 years ago

Godot version

v3.4.3.rc1.official [894b6d504]

System information

Mac OSX (Intel 2019)

Issue description

Well actually it throws an error if you try and print that, but if you use it in a loop it just silently fails:

print(range(0.0, 1.0, 0.5))
Screenshot 2022-02-17 at 01 32 24
for i in range(0.0, 2.0, 0.1):
        print("HERE", i)

And Here never get's printed.

Steps to reproduce

Please see above code samples

Minimal reproduction project

No response

Sauermann commented 2 years ago

According to this comment in the source code, range is supposed to work for integers only: https://github.com/godotengine/godot/blob/73ece5ba1962eb3c19ac529b8cb4ac18aefd5cac/modules/gdscript/gdscript_analyzer.cpp#L1243-L1244

Here is the location, where they are converted: https://github.com/godotengine/godot/blob/73ece5ba1962eb3c19ac529b8cb4ac18aefd5cac/modules/gdscript/gdscript_analyzer.cpp#L1289-L1293

The documentation doesn't say anything about range being used for integers only - at least I didn't find anything about it. v4.0 also treats range as integers.

tavurth commented 2 years ago

Oh that's a shame, I never really ran into this problem until now, just always expected it behaved the same way as python.

Is this then a documentation + feature request issue?

I guess we should at least fix the bug with the loop, and incorrect message when printing (0.1 is not 0)

novaplusplus commented 2 years ago

I suppose an "frange" function could be useful, although of course the third parameter would be mandatory to know the increment.

akien-mga commented 2 years ago

Precision errors accumulate on floats, so it's much better to keep integers for your iterator. Just divide it by whatever you need to have the float range you want, e.g.:

for i in range(20):
    print("HERE", i / 20.0)

That being said, using a float step in range() should probably raise a warning instead of just coercing the values silently.

tavurth commented 2 years ago

Precision errors accumulate on floats

Unless we were to floor it each step as in the example above, it would be a bit easier for junior devs

Perhaps it would be possible to do this for a frange util instead though to save performance for range

novaplusplus commented 2 years ago

Yeah. "frange" would be integers internally, only returning floats. It just does a bit of math behind the scenes to set up the array. Honestly I'd consider trying to make it myself but I have only just barely started to dip my toes into the engine code, so...

amoriqbal commented 2 years ago

That being said, using a float step in range() should probably raise a warning instead of just coercing the values silently.

Godot does not show a warning while coercing from float to int.

func abc(x:int):
    print(x)

func _ready():
    abc(1.5)

The above code runs without warning and prints 1.

var x:int = 1.5 does not show any warning either. So, are we to have a special warning for range function?

tavurth commented 2 years ago

I think the problem is that it can just return an empty array without any warning that it's flooring to int.

This caused me a hard to track down bug with the range in the title because it was silently failing for some values.

name-here commented 2 years ago

That being said, using a float step in range() should probably raise a warning instead of just coercing the values silently.

Godot does not show a warning while coercing from float to int.

Honestly, the better fix might be to always give a warning for coercing from float to int. Many languages don't allow implicit conversion from float to int at all, since it can cause accidental truncation like we see here.

Calinou commented 2 years ago

Honestly, the better fix might be to always give a warning for coercing from float to int. Many languages don't allow implicit conversion from float to int at all, since it can cause accidental truncation like we see here.

There is already such a warning, but it's not always triggered: https://github.com/godotengine/godot/blob/6a51999b7f4d9f7b1f950d7a655a464f28542318/modules/gdscript/gdscript_warning.cpp#L87-L89

It mainly works in this kind of situation: https://github.com/godotengine/godot/blob/6a51999b7f4d9f7b1f950d7a655a464f28542318/modules/gdscript/tests/scripts/parser/warnings/narrowing_conversion.gd

vnen commented 2 years ago

We can add a specific warning for this case, or maybe even an error since I don't think there's a valid case for using floats in range()

tavurth commented 2 years ago

I don't think there's a valid case for using floats in range()

I use it a lot when I'm doing procgen stuff. range(0, 1, 0.1) is just shorthand to generate a list of percentages, which I then use to position things on a grid.

Yes you can use int and divide by 10, but having this is a great timesaver and quality of life.

cdemirer commented 2 years ago

Since

for i in Vector3(0.0, 5.0, 1.5):
    print(i)
0
1.5
3
4.5

works, wouldn't it make sense to have a range equivalent?

tavurth commented 2 years ago

@cdemirer

for i in Vector3(0.0, 5.0, 1.5):
  print(i)

Wow I didn't know about this. ~Seems aliasing range to vector3 can be a solution if it covers all cases 😅~

Edit: It's not a good solution as we still need to do something like:

var my_range = range(0, 10)

Having this feature built into the language but only to those who know about it also doesn't seem so kind to the developer. If I start to use Vector3 in my loops then others who come along later probably won't understand at once.

novaplusplus commented 2 years ago

Having this feature built into the language but only to those who know about it also doesn't seem so kind to the developer. If I start to use Vector3 in my loops then others who come along later probably won't understand at once.

I know my first response was "wait, what the hell?"

That's such an odd and obscure syntax. If it weren't for how specifically it works I would almost be inclined to say it's an accident.