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

Node.raise() is misleading #28948

Closed
KoBeWi opened this issue May 16, 2019 · 9 comments · Fixed by #60108
Closed

Node.raise() is misleading #28948

KoBeWi opened this issue May 16, 2019 · 9 comments · Fixed by #60108

Comments

@KoBeWi
Copy link
Member

KoBeWi commented May 16, 2019

Godot version:
3.1.1

Issue description:
The description of this method says:

Moves this node to the top of the array of nodes of the parent node.

What does the "top of the array" mean? I interpreted it as it moves it as first child, but actually it's the opposite. The documentation should clearly state where the node will move instead of using vague terms.

@Zylann
Copy link
Contributor

Zylann commented May 17, 2019

It is called raise() because it was made for bringing the node visually on top of its siblings in 2D. For that, the node has to be moved to the bottom of the list, so the doc is wrong indeed.
But considering it's a Node method in an engine where also 3D and non-graphic things exist, raise is quite a generic term which could be interpreted in many ways...
Another candidate for #16863 ?

@Zireael07
Copy link
Contributor

Definitely a candidate for renaming, neither the docs nor the name say what it does.

@regakakobigman
Copy link

The name is also confusing to anyone expecting raise() to be a counterpart to Python's raise statement.

@aaronfranke
Copy link
Member

If this method is desired, I think it would make sense to move it to CanvasItem.

@bojidar-bg
Copy link
Contributor

bojidar-bg commented Feb 12, 2020

@aaronfranke the problem with doing this is that while raise() is mostly useful only for CanvasItem, its functionality does not depend on CanvasItem in any way, and thus it makes little sense to be there.

@Zylann
Copy link
Contributor

Zylann commented Feb 12, 2020

@bojidar-bg does it have a use case in 3D though? It seems to be way less common.

@KoBeWi
Copy link
Member Author

KoBeWi commented Feb 12, 2020

Tree order affects e.g. processing order, so technically it has use in any node.

@Zylann
Copy link
Contributor

Zylann commented Feb 12, 2020

@KoBeWi This has an effect for sure, but then the use case is different.
Do we want a general move_to_end function that moves a node to the end in an attempt to fulfill different use cases (for which it would have needed different names anyways) ;
or, do we want bring_to_front, in which case it makes sense to move it to CanvasItem, matching the use name it was originally intented for.
The function is a shortcut to a one-liner, so common use case matters more than what it actually does.

@KoBeWi
Copy link
Member Author

KoBeWi commented Dec 24, 2020

The doc was updated, but what should we do with the method? Rename? (could be called move_to_top()) Remove?

@KoBeWi KoBeWi added this to the 4.0 milestone Jul 15, 2021
@KoBeWi KoBeWi moved this to To Assess in 4.x Priority Issues Apr 9, 2022
@KoBeWi KoBeWi moved this from To Assess to Todo in 4.x Priority Issues Apr 9, 2022
@KoBeWi KoBeWi moved this from Todo to In Progress in 4.x Priority Issues Jul 2, 2022
Repository owner moved this from In Progress to Done in 4.x Priority Issues Sep 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

7 participants