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

Rename pos to index on Node #67403

Merged
merged 1 commit into from
Oct 15, 2022
Merged

Conversation

bitbrain
Copy link
Contributor

@bitbrain bitbrain commented Oct 14, 2022

Why this change?

When calling Node::move_child() it currently expects to_position which can get very confusing when using GDScript. I was wondering why I was unable to pass an actual position (Vector2) into it. Then I realized that by "position" we actually mean the "index" in the node hierarchy. To avoid mixing concepts together I decided to rename pos to index. This should not break compatibility as it is just an internal rename + documentation update.

@bitbrain bitbrain requested review from a team as code owners October 14, 2022 18:26
@KoBeWi KoBeWi added this to the 4.0 milestone Oct 14, 2022
@bitbrain bitbrain changed the title Rename pos to index on Node Rename pos to index on Node Oct 14, 2022
@kleonc
Copy link
Member

kleonc commented Oct 15, 2022

Kinda related to: #61418. But in here in case of Node::move_child its parameter seems to already be treated as an index, not as a position in a sense explained in #61418 (comment). Hence the renaming done seems correct.

The strange part to me is that child_count is treated as a valid index, it's treated equivalently to child_count - 1. Personally I'd make passing child_count be invalid since it's not a valid index. But oh well, it's already made like that (for whatever reason) so changing it would be breaking and thus it's for a potential separate PR (if changing it is desired at all).

@Calinou I don't think this PR breaks compatibility, it's only renaming the parameter.

@Mickeon
Copy link
Contributor

Mickeon commented Oct 15, 2022

The strange part to me is that child_count is treated as a valid index, it's treated equivalently to child_count - 1. Personally I'd make passing child_count be invalid since it's not a valid index. But oh well, it's already made like that (for whatever reason) so changing it would be breaking and thus it's for a potential separate PR (if changing it is desired at all).

I don't know. Compatibility can be kept by changing it to clamp instead of subtracting 1, which is less awkward, may I add.

@akien-mga akien-mga merged commit 2942951 into godotengine:master Oct 15, 2022
@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