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

For loop with true iterators instead of arrays #3765

Closed
ret80 opened this issue Feb 18, 2016 · 19 comments
Closed

For loop with true iterators instead of arrays #3765

ret80 opened this issue Feb 18, 2016 · 19 comments

Comments

@ret80
Copy link

ret80 commented Feb 18, 2016

Often need a simple classic "for" both "с".
Create an array and a listing of the iterator is not the best option. It's slow and consumes a lot of memory. A realization of the cycle through "while" is not always convenient.

Discuss?

@nunodonato
Copy link
Contributor

for i in range(1,10) ?

@sheepandshepherd
Copy link
Contributor

@nunodonato range currently just generates an Array with all the values, which is inefficient. The GDScript documentation says range doesn't allocate, which means it's probably supposed to create an iterator, not an array.

Custom iterators are actually implemented in Godot already in the Variant class, but they're not documented. You can turn any class into an iterator by overriding bool _iter_init(arg), bool _iter_next(arg), and Variant _iter_get(arg). I don't know what the arguments are for, but they seem to always contain [self]. Example of one in GDScript.

Should we change range into an actual iterator?

@ret80
Copy link
Author

ret80 commented Feb 19, 2016

for i in range(1000000): # create a huge array that is not effective

for i in range(10, 0): # instead of the [10, 9, 8, ...] becomes [0, 1, 2, ...] which also creates some difficulties

@sheepandshepherd
Thanks for the interesting examples

@akien-mga
Copy link
Member

for i in range(10, 0): # instead of the [10, 9, 8, ...] becomes [0, 1, 2, ...] which also creates some difficulties

I believe you should use this instead:

for i in range(10, 0, -1):

@nunodonato
Copy link
Contributor

the point is, it seems GDScript is trying to stay close to the python syntax (similar discussions have been happening in other issues), so in this case, too, I think it would be better to do it as python does and not reinvent the wheel. for large numbers you can use a while as it seems in python a 'for' is used to iterate over "iteratable" things :)

@akien-mga
Copy link
Member

for large numbers you can use a while as it seems in python a 'for' is used to iterate over "iteratable" things :)

+1. Fixing the crash of #3756 is anyway a good thing and @est31 has done a nice job in his PR. A further step would be to prevent using such big sizes for range as it's very inefficient anyway. As you say, if one needs to iterate over 1M elements, better use a while loop.

@akien-mga akien-mga changed the title feature: clasic for (gd script) For loop with true iterators instead of arrays Feb 19, 2016
@reduz
Copy link
Member

reduz commented Feb 19, 2016

I thought it was working with a true iterator, the code was added to Variant to handle this, but seems I forgot to add it..

@sheepandshepherd
Copy link
Contributor

the point is, it seems GDScript is trying to stay close to the python syntax

Interesting. Python's range() had this same situation: in Python 2 it created a list, but in Python 3 it was changed to an iterator. In Python 2 the iterator was called xrange.

The Python 3 way would break code that's intentionally using range to create an Array. Of course, a "RangeIterator" object could have a "toArray" function to replace the current range as well. Just depends on which API we'd rather use in Godot :)

Edit: made an example range iterator in C++. It could still use improvement (error-checking/constructors?), but it can do everything range does:

func example():
    for i in RangeIterator.new().set_range(10):
        print(i)
    for i in RangeIterator.new().set_range(5, 10):
        print(i)
    for i in RangeIterator.new().set_range(10, -10, -5):
        print(i)

@bojidar-bg
Copy link
Contributor

Maybe we can generate an iterator only in some cases? Or make the iterator switch to Array mode whenever you change it...

@est31
Copy link
Contributor

est31 commented Feb 23, 2016

Or make the iterator switch to Array mode whenever you change it...

That'd be an useful modification. The array allocation should only happen if you add, change, or remove elements.

I'm sure we can do better than python here.

@sheepandshepherd
Copy link
Contributor

Hmm. I don't think a variable can change its own type, so changing to an Array automatically would be difficult. range knows nothing about the context it's called in; it's just a function with a return value (either Array or iterator).

The RangeIterator class I made now has a to_array method like the current range function. Isn't an explicit conversion better? But I can hold off on making a PR until we've discussed what we actually want GDScript's range to do.

@est31
Copy link
Contributor

est31 commented Feb 23, 2016

range could be made return a PseudoArray object which only allocates an actual array internally when somebody writes to it. The type to the outside always stays Array. So for gdscript code, there is no difference, but its far more efficient.

@sheepandshepherd
Copy link
Contributor

Are the index and index-assignment operators overridable for GDScript? That would allow me to turn the iterator into a pseudo-array.

@est31
Copy link
Contributor

est31 commented Feb 24, 2016

@sheepandshepherd theoretically they can be overridden, yes (functions get and set). But modifying this in a way where nothing is broken is really hard. Think of legacy code that passes the return value of range to methods which have params typed Array. Simulating an array is much more than just these two things, a truly backwards compatible solution is too complicated to be realized, I think.

I've added your commit to #3814. The PR is I think the best way to solve this issue. It takes a conservative approach, mimicking Python 2, but being as fast as Python 3 in the common use case of the for loop.

@bojidar-bg
Copy link
Contributor

BTW, there are some other things that might (or... might not) work better with iterators, like Node::get_children().

@akien-mga
Copy link
Member

Reopening as the previous fix has been reverted.

@bojidar-bg
Copy link
Contributor

I think that's closable after @reduz's recent commits...

@Giacom
Copy link
Contributor

Giacom commented Oct 26, 2017

What was changed? I think this issue is still worth looking into.

@reduz
Copy link
Member

reduz commented Oct 26, 2017 via email

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

No branches or pull requests

8 participants