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

Remove Node.get_position_in_parent() #37556

Merged
merged 1 commit into from
Apr 6, 2020

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Apr 3, 2020

See #21802

@KoBeWi KoBeWi added this to the 4.0 milestone Apr 3, 2020
@KoBeWi KoBeWi requested a review from a team as a code owner April 3, 2020 18:29
@YuriSizov
Copy link
Contributor

Why remove this and not the other? It's shorter and perfectly aligns with the fact that the result will be used as an index argument in a function like get_child.

@Zireael07
Copy link
Contributor

I agree, get_position_in_parent() is waaay too wordy...

@KoBeWi
Copy link
Member Author

KoBeWi commented Apr 3, 2020

I agree, get_position_in_parent() is waaay too wordy...

It's more descriptive though. Maybe we could come up with a completely new name?

@YuriSizov
Copy link
Contributor

YuriSizov commented Apr 3, 2020

I don't think that such function needs a verbose name. It's a function that returns node's index in its parent. Should we call it get_index_in_parent() then? Well, where else would that index be? And this is how we arrive to the get_index() name.

Calling it get_position_in_parent() is too descriptive and this is what documentation is for.

Edit: Also, a "position in parent" may mean something different for 2d and 3d nodes, so that name may even be confusing for some.

@aaronfranke
Copy link
Member

get_index_in_parent() is what immediately occurred to me. get_position_in_parent() is a really poor choice of words because "position" means something else for Node2D and Node3D.

@KoBeWi
Copy link
Member Author

KoBeWi commented Apr 3, 2020

How about get_sibling_index?

@aaronfranke
Copy link
Member

I think get_sibling_index is not immediately clear that we're getting the index of this node among siblings, since it could also be read as "get the index of another sibling". get_index_among_siblings? I'm still leaning towards get_index_in_parent.

@YuriSizov
Copy link
Contributor

Well, if we must include a reference to the parent, then get_index_in_parent is our only option. Definitely not sibling. That implies that this index has something to do with current node's siblings.

In fact, I can't think of any adjective to put before the word index here, that wouldn't sound ambiguous or even more confusing than without any.

@hubbyist
Copy link

hubbyist commented Apr 4, 2020

get_node_index() maybe? But get_index also implies that node is inside a collection which will have a parent anyway.

@Xrayez
Copy link
Contributor

Xrayez commented Apr 5, 2020

I agree, get_position_in_parent() is waaay too wordy...

It's more descriptive though. Maybe we could come up with a completely new name?

That's what documentation is for, thanks goodness Godot has good accessibility for docs.

EDIT: oops, literally quoted #37556 (comment).

@aaronfranke
Copy link
Member

Since get_index() seems good to many people, we should change this PR to be the opposite of what it currently is, keeping get_index() and removing get_position_in_parent().

This way we can avoid having a duplicate methods for now, and we can maybe rename get_index later if it's desired.

@KoBeWi KoBeWi changed the title Remove Node.get_index() Remove Node.get_position_in_parent() Apr 5, 2020
@KoBeWi
Copy link
Member Author

KoBeWi commented Apr 5, 2020

Heh, looks like get_index() was more commonly used in the code. The number of changed lines significantly dropped.

doc/classes/Node.xml Outdated Show resolved Hide resolved
scene/main/node.cpp Outdated Show resolved Hide resolved
scene/main/node.h Outdated Show resolved Hide resolved
scene/main/node.cpp Outdated Show resolved Hide resolved
@akien-mga akien-mga merged commit 6b07c72 into godotengine:master Apr 6, 2020
@akien-mga
Copy link
Member

Thanks!

@KoBeWi KoBeWi deleted the kill_get_index branch April 6, 2020 08:19
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.

None yet

7 participants