Skip to content

Add support for range based loops for Array#1717

Merged
dsnopek merged 1 commit into
godotengine:masterfrom
tomfull123:typed-array-range-based-loop-support
Apr 2, 2025
Merged

Add support for range based loops for Array#1717
dsnopek merged 1 commit into
godotengine:masterfrom
tomfull123:typed-array-range-based-loop-support

Conversation

@tomfull123

@tomfull123 tomfull123 commented Feb 22, 2025

Copy link
Copy Markdown
Contributor

Fixes #1710
This PR adds support for range based loops for the TypedArray class.
Allowing syntax such as:

TypedArray<int> elements;
for (const auto& e : elements) {}

I wanted to get proper type inference for the element, but I had issues getting the Variant type to cast properly.

@tomfull123
tomfull123 requested a review from a team as a code owner February 22, 2025 15:05
@dsnopek

dsnopek commented Mar 13, 2025

Copy link
Copy Markdown
Collaborator

Thanks!

However, one of godot-cpp's core design goals is to be compatible with Godot's internal APIs, and this differs a little bit from them:

  • I don't think we should publicly expose ptr() or ptrw() because Array/TypedArray don't have them in Godot
  • These iterators should be on Array rather than TypedArray, because that's where they are in Godot, and that would also allow folks to use range-based loops an Arrays, which would be nice

@dsnopek dsnopek added the enhancement This is an enhancement on the current functionality label Mar 13, 2025
@dsnopek dsnopek added this to the 4.x milestone Mar 13, 2025
@tomfull123 tomfull123 changed the title Add support for range based loops for TypedArray Add support for range based loops for Array Mar 18, 2025
@tomfull123

tomfull123 commented Mar 18, 2025

Copy link
Copy Markdown
Contributor Author

Changed the PR to now add the iterator to Array instead of TypedArray, I can make the ptr() and ptrw() methods private, but they are public in the PackedArray classes, so it would be consistent with those, but maybe its better for them to have those methods private too, what do you think? @dsnopek

I think ptr() and ptrw() should be made private in all the PackedArray classes too, to match Godot, I can put in a PR for that too.

@dsnopek

dsnopek commented Mar 19, 2025

Copy link
Copy Markdown
Collaborator

Thanks!

Changed the PR to now add the iterator to Array instead of TypedArray, I can make the ptr() and ptrw() methods private, but they are public in the PackedArray classes, so it would be consistent with those, but maybe its better for them to have those methods private too, what do you think?

The goal isn't to be internally consistent among the classes in godot-cpp, but to be consistent with the API of Godot.

In Godot, the PackedArray classes have public ptr() and ptrw() methods, but Array/TypedArray do not, so we should do the same in godot-cpp. If we wanted to have those methods on Array/TypedArray, we'd need to make a PR to add them to Godot first.

Also, lots of extensions are already using ptr() and ptrw() on the PackedArray classes (including some I maintain :-)) so we really couldn't remove them now anyway

@tomfull123

tomfull123 commented Mar 19, 2025

Copy link
Copy Markdown
Contributor Author

@dsnopek cool, the Array's ptr() and ptrw() methods are now private

Comment thread binding_generator.py
@tomfull123
tomfull123 requested a review from dsnopek March 26, 2025 12:36
@tomfull123

Copy link
Copy Markdown
Contributor Author

Is this PR waiting on anything? @dsnopek do you have any other concerns?

@dsnopek

dsnopek commented Apr 1, 2025

Copy link
Copy Markdown
Collaborator

Thanks, this is looking good to me now!

Only one last thing - this needs to be squashed to a single commit per Godot's git workflow: https://docs.godotengine.org/en/latest/contributing/workflow/pr_workflow.html#the-interactive-rebase

(This applies to all your PRs, actually - sorry that I didn't notice earlier that you were making multiple commits)

@tomfull123
tomfull123 force-pushed the typed-array-range-based-loop-support branch from 4eca894 to a2c37f8 Compare April 1, 2025 19:48
@tomfull123

Copy link
Copy Markdown
Contributor Author

Ahh no worries, I should have read the PR guidelines more thoroughly, should be good to go now

@dsnopek
dsnopek merged commit da064d8 into godotengine:master Apr 2, 2025
@tomfull123
tomfull123 deleted the typed-array-range-based-loop-support branch April 2, 2025 18:10
@dsnopek

dsnopek commented Jun 24, 2025

Copy link
Copy Markdown
Collaborator

Cherry-picked for 4.4 in PR #1805

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

Labels

enhancement This is an enhancement on the current functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Godot-cpp Array API does not feature begin/end functions

2 participants