Move z_index, z_as_relative and y_sort_enabled from Node2D to CanvasItem

[Geometror]

This PR moves the z_index, z_as_relative and y_sort_enabled properties from Node2D to CanvasItem.
This could be quite handy for advanced UIs since it allows changing the drawing order of Control nodes.

Closes godotengine/godot-proposals#839.

IMO this is a better place for the properties as the setters call RS::canvas_item_set_z_index(...), RS::canvas_item_set_z_as_relative_to_parent(...) and RS::canvas_item_set_sort_children_by_y(...) internally which shows that the RS API is designed to work with CanvasItem, not just Node2D.

Update: To prevent confusion a configuration warning is shown when changing the z_index of a Control node:

[akien-mga]

This would address godotengine/godot-proposals#839.

I think last time we discussed this on Rocket.chat, @reduz raised the objection that would be confusing since it only impacts the render order, while in Controls you would expect the order in which items are laid on top of each other to impact input handling. So changing the Z Index on e.g. a Button to make it appear in front of another Button would be the misleading impression that it's the topmost button, and thus the one receiving the input, when it isn't.

Someone also raises the same objection in that proposal and there's further discussion, to be assessed further.

[KoBeWi]

As I said in the chat, I think Controls should just have a configuration warning if their z_index is non-zero.

[Geometror]

Accidently pushed the wrong branch :/

[Geometror]

Rebased and added the configuration warning suggested by @KoBeWi to Controls with z_index != 0.

[KoBeWi]

Implementation is ok.

[reduz]

I still think this is not a great idea, its very corner case and easy to work around, plus having to look for this in canvas item is a bit weird.

But if you really want it this way, go ahead.

[Geometror]

@reduz To me it just makes a bit more sense there. Having a duplicated property both in Control and Node2D would be somewhat strange (regarding OOP, since the RS provides the functions for CanvasItem) and the current workaround where you have to make a Control node the child of a Node2D is a bit ugly (and needs some research, which leaves many users thinking this isn't possible at all).
After all this seems like a feature many users missed, given the proposal which this PR implements got nearly 50 upvotes.
I think having a full configuration warning is a bit too much, although its fine for now. Maybe a property warning would be enough if that gets implemented (looking at #68420 or something similar) or nothing at all because I have a feeling that the confusion might be less of a problem than anticipated. But I guess only time will tell if this gets merged :)

[YuriSizov]

the current workaround where you have to make a Control node the child of a Node2D is a bit ugly

This is not how you work around this now. You use the server API, for example (a 3.x version):

extends Control

func _ready():
	VisualServer.canvas_item_set_z_index(get_canvas_item(), 100)

I think having a full configuration warning is a bit too much, although its fine for now. Maybe a property warning would be enough if that gets implemented

I think it's not enough, definitely not just a property warning. It breaks several expectations for Control nodes, and hiding this fact so deep is not a good idea.

[Geometror]

This is not how you work around this now. You use the server API, for example (a 3.x version):

You're right, the cleaner workaround would be to use the server API (just for reference, in 4.0 it can be achieved by using the respective RenderingServer methods) However, this has the disadvantage that it can't be done without a Script and as you said you're able to change the behavior of Control nodes, breaking several expectations without any warning.

I think it's not enough, definitely not just a property warning. It breaks several expectations for Control nodes, and hiding this
fact so deep is not a good idea.

Now after thinking about it, that might be true. Even for users which exactly know what they are doing it might be helpful to have a warning directly in the SceneTree to indicate that a Control behaves abnormally. (as mentioned above, this is also the problem with the server API workaround).

[YuriSizov]

Yep, I think we're on the same page. 🙃 Can you update the documentation though, as suggested? A note with basically the same content as the warning would be good to have in the documentation.

[Geometror]

Supplemented the documentation with a note concerning Control nodes (+ added a small example use case for better understanding).

[akien-mga]

Thanks!

[Mickeon]

This is major. Putting them into CanvasItem makes it so much easier for users to imagine and familiarize with the 2D rendering steps at a glance. Now CanvasItem can be really thought to be the 2D rendering node, while Node2D and Control are just different ways to shift the rendering around.

[HarmonyHoney]

I'm stoked on this!! I've often wanted to use z_index on a control node!! I've had a few use cases in the UI for my game ROTA. This will help me organize my project cleanly(:

[naturally-intelligent]

I like this idea! I've been adding Controls to Node2Ds just to get the ZIndex. Always worried it was a hack but did it anyways since it just works

[Zireael07]

Currently editing a project that will MASSIVELY benefit from this <3

[Zireael07]

Is this potentially backportable to 3.x? I have a project that does NOT need Vulkan but needs this...

[YuriSizov]

While this is marked as a breaking change, I don't think it technically is. In a sense that it doesn't replace or reduce functionality, at least. So it's probably possible if anyone wants to do it.

That said, you can already use this feature through scripts in 3.x. This is just a convenience change.

[Zireael07]

While this is marked as a breaking change, I don't think it technically is. In a sense that it doesn't replace or reduce functionality, at least.

That's exactly why I asked

That said, you can already use this feature through scripts in 3.x. This is just a convenience change.

Please tell me (or even better, document somewhere) how to achieve the same thing with Control nodes in 3.x without this change

[akien-mga]

Please tell me (or even better, document somewhere) how to achieve the same thing with Control nodes in 3.x without this change

#68070 (comment)

[Zireael07]

Thanks a lot, goes into my offline notes. This means I can fix my map without waiting for this PR to be backported <3

[Mickeon]

So, this is possible to backport. The limitation was merely artificial by design?

[YuriSizov]

The limitation was merely artificial by design?

I think it's more that the low-level implementation makes it unintentionally possible for any CanvasItem, but the feature itself was only intended to be used directly by Node2D, and thus only accounted for its behavior. If it was only artificial we wouldn't have it, and we wouldn't need a warning because it would just work as expected in every situation.