Remove YSort node, move its functionality into Node2D

[andriyDev]

Currently, all a YSort node does is control calls to RenderingServer.canvas_item_set_sort_children_by_y. By deleting YSort and moving these calls to Node2D, we can now have any node be treated as a YSort (if it wants). The main advantage of this is that we can now have "deep" y-sorting if desired. In order to y-sort children at the moment, all intermediate nodes must also be YSort nodes, which is very limiting. By making all Node2D's effectively YSort nodes, we can have deep y-sorting anywhere.

In addition, we replace the cell_y_sort property in TileMap with y_sort_enabled.

Discussion of this proposal is offered at godotengine/godot-proposals/issues/1237, similar to Proposition A mentioned in the original proposal.

Edit by godotengine/documentation:
Closes godotengine/godot-docs#2286
Closes godotengine/godot-docs#1915

[NathanLovato]

This feature sounds great. The docs could be clearer and follow our writing guidelines better. I've left a suggestion.

Note for later: If this gets merged, we should write a manual talking about Z-order that we can link from Node2D's class reference.

[NathanLovato]

Approving the documentation

[KoBeWi]

We discussed this PR briefly on IRC some time ago and the change is good, but the YSort node should be deprecated instead of removed (to not break project migrating to 4.0). For now it would be the same as Node2D, but with y_sort_enabled being true by default, and it should have a deprecation warning saying that it will be removed (like AnimationTreePlayer had). Then the YSort will be removed in 4.1.

[andriyDev]

We discussed this PR briefly on IRC some time ago and the change is good, but the YSort node should be deprecated instead of removed (to not break project migrating to 4.0). For now it would be the same as Node2D, but with y_sort_enabled being true by default, and it should have a deprecation warning saying that it will be removed (like AnimationTreePlayer had). Then the YSort will be removed in 4.1.

This is a good point, I will make the necessary changes when I can.

[andriyDev]

I have updated the PR to have YSort deprecated!

[andriyDev]

I pushed a change which actually used doctool to generate the docs, I didn't realize that that was a tool! The docs are now updated correctly, including the YSort doc which contains the new defaulted value.

This may need to be changed based on whether I should actually be keeping YSort for deprecation reasons though!

[andriyDev]

@bojidar-bg @avencherus @akien-mga I am unsure how I should make the behaviour... The currently pushed revision works overall, however it has a large caveat - the root of the Y-sorting is not itself Y-sorted. Ex: if you have 3 sprites in a single chain, the bottom 2 sprites will be Y-sorted, but the root will always render behind (except if Show Behind Parent is true). This behaviour is very easy based on the current implementation.

This is not very intuitive behaviour to me. Ideally, a parent would also be Y-sorted amongst its children, so the root would be able to render ahead or behind its children based on the children's vertical height. I believe I can make this modification, although it would be more code. In addition, it does make the "Show Behind Parent" property on children a little unclear, and would almost certainly need to be ignored during Y-sort amongst the parent, which I think is fine. Please let me know of any opinions you all have!

[andriyDev]

Marking this commit just above as the "simple" implementation - not Y-sorting parent amongst its children.

[andriyDev]

This is the commit for the "complex" implementation - Y-sorting parent amongst its children. This required a reorganization of how canvas items are culled and organized. Specifically, splitting off a function to "attach" canvas items to the list for drawing, and using that to reorder how these "attach" calls are issued.

If the opinion is to use the other method, I have a branch to easily push it onto the PR.

[ghost]

You're right, very good point.

It's a bit vague how behind_parent would or should work. Looking at the current behavior with YSort nodes, it doesn't look like it even does anything sensible there. I don't think it's expected to be used in combination with YSort. The use-cases are quite different.

Below the YSort node, behind_parent nodes otherwise work fine for the sub-children when making a character out of several Sprites. So only that behavior would need to be preserved.

When comparing against the node to be ysorted, I imagine the best that could be done is to track the final render order/index of the ysort parent, and at some point during the algorithm inserting these behind_parent child nodes just above the ysort parent. IE: insert(parent_index - 1, child).

The order of the child traversal would be something to watch for as well.

[andriyDev]

In either implementation, behind_parent works for non-Y-sorted nodes (including ones that are part of some Y-sort tree above its parent). I think trying to force behind_parent to do something for Y-sorted nodes isn't sensible. It'll just make the behaviour unclear and I for the life of me cannot think of a situation where you'd need to Y-sort but also remain beneath the parent? The workaround would be to make a root node above the parent and add a new Ysorted tree adjacent to the parent. Trying to jam behind_parent behaviour in would almost certainly make the code more complex for something probably no one is going to use. That's my opinion at least, I'd like to see what the others think on this. :)

[reduz]

I am starting to see more than one case for 4.0 where not only we need to replace one node type by another, but also do some custom configuration or logic to keep compatibility, so this may require changes to the format loaders. Let me discuss this a bit with other contribs, so maybe we can add a compat API.

[golddotasksquestions]

Hi, I would like to leave a comment here from my perspective as someone who cares about usability first and features only second to that.

2D sorting is already not easy to comprehend for newcomers. It took me ages to make sense of it and it's still difficult for me to explain all the different options and things that may or may not cross influence each other and eventually have a final say in what is drawn on top of what other thing. People come to the community forums on a daily basis looking for help with sorting issues.

I recently posted a comment on Godot's subreddit, trying to explain 2D sorting in Godot, and it took a whole a page long wall of text. Thanks to the changes in 3.1, the current y-sorting solution is one of the easiest things to explain and understand out of all the sorting options.

I think what I am asking for here is to consider if adding this new y-sorting really makes sorting in Godot easier to understand and more intuitive to use.
If it does not fulfill this criteria, but instead adds functionality for the cost of usability, I do think it would be fare to discuss if it is really worth it, and what could be done to make it at least as easy to use and explain as our current feature.

In the vast majority of my time using Godot, I use Ysort extensively, because I love top down 2D games. In 2.5 years of using Ysort in Godot, even in bigger projects, I have never once had the need to Ysort the descendants of Ysort children. Nesting Ysort nodes covered all my needs (in cases like to tidy up the Scene Tree, to group nodes together)

I would also welcome a feature that adds more flexibility and functionality as this one here. Maybe this opens up a possibility for usecases I can't even imagine right now. However I'm also sensing it might result in the same confusion as the current pause implementation periodically brings up, or the Control Container nodes influencing all it's children, or mouse filtering of Control nodes.

If we start to give nodes like Sprites and KinematicBody2Ds a ysort property that if enabled will ysort all their decendance, this just opens a can of worms. What if I only want some of the children ysorted? Would I have to manually tick every one of them? What if I want all but a few of the children ysorted? Y-sorting the children of a character is almost never wanted for example. Would I need to uncheck the Character Kinematic body then? If I did would that mean I would need a parent node that has this property checked (this is the behavior we currently have btw)? What about the Sprite chain as @andriyDev described? In this case you would want the root to y-sort itself.

The way I see it this will quickly descend into into a similar system as the pause propagation and the mouse filtering. Do we really want that? Who really has a uscase/need for this new y-sorting feature anyway? I have not come across any proposal or community post asking for it or expressing a need. Also people here are cheering, but noone actually explained why this is needed in the first place. It certainly does not close godotengine/godot-docs#2286, instead it raises more questions. It seems like an awesome feature addition on first glance, but I'm afraid it might make things a lot more complicated for very little benefit and without much (if any) need. I would really love to hear about a practical usecase (Isn't this what the GIP system is for?)

[Pennycook]

I'm inclined to agree with @golddotasksquestions. It took me a while (and some experiments) to figure out how the current implementation of YSort works, and this new change seems like it would make things even more complicated for new users.

Reading back over the discussion, I'm also beginning to wonder whether there's a good reason to make this a breaking change. If many people are happy with the existing behavior but other people are interested in some new use-cases, why not make the existing YSort node more configurable instead of changing the default behavior to the most complex and expensive use-case?

For example, the "Enabled" checkbox in the YSort node itself could be augmented by a drop-down with options like "Children", "Descendants" and "Group". That would allow the existing behavior to remain the default while also providing users a way to opt-out of deep recursive sorts or opt-in to sorting nodes at arbitrary depth (i.e. all nodes in a user-specified group).

[akien-mga]

We discussed this in a PR review meeting yesterday and confirmed that this is a good change. Arguably, making Y sorting a property of Node2D instead of a dedicated node might make things more user-friendly, as the property will likely be easier to find than a dedicated node (unless you're experienced and know to search for YSort to do Y sorting).

Could you rebase the PR? Note that the TileMap code was fully rewritten so you might need to adjust the code and make sure that it still works as intended.

@golddotasksquestions Regarding the first part of your comment, there is no change in functionality in this PR. The only thing it changes is that instead of using a YSort node to do Y sorting, you'll use a Node2D node (or any node that inherits Node2D) and enable the y_sort_enabled property.

For further features as discussed by @golddotasksquestions and @Pennycook, you should write a proposal on https://github.com/godotengine/godot-proposals. More options for Y sorting can be considered but that's orthogonal to this PR which is just a refactoring and simplification of the API.

[Pennycook]

For further features as discussed by @golddotasksquestions and @Pennycook, you should write a proposal on https://github.com/godotengine/godot-proposals. More options for Y sorting can be considered but that's orthogonal to this PR which is just a refactoring and simplification of the API.

Sorry about that -- I didn't mean to make a proposal, just to point out that it seemed there were alternative implementations that wouldn't require the Y-Sort node to be removed.

I think I might be confused about what this change does, because reading back over the discussion makes it seem like much more than a "refactoring and simplification of the API". There are two things I don't understand:

1. Whether or not a node with Y-Sort enabled is itself included in the Y-Sort.

2. When the "deep Y-Sort" stops.
   Does it stop collecting children the first time that it encounters a Node2D that doesn't have Y-Sort enabled, or does it keep recursing to find any descendant with Y-Sort enabled? I thought the latter was what Remove YSort node, move its functionality into Node2D #42282 was proposing, and I think that would change the behavior of existing Y-Sort nodes.

Apologies if I've misunderstood the change; I'm still not very familiar with the codebase.

[andriyDev]

@Pennycook No worries!

1. A node with Y-Sort enabled is indeed included in the Y-Sort. Practically I don't expect this to come up much, the root of a y-sorting group is likely not to be a renderable element anyway. Regardless, it will work in that situation.
2. Yes, the new y-sort will stop collecting children the first time it encounters a Node2D without y-sorting - just like the existing behaviour!

The codebase can be tricky haha, I came back to this PR and I was in shambles trying to rebase this.

[andriyDev]

@akien-mga Done. Rebased, and verified everything is working as intended!

[akien-mga]

Thanks!

[Pennycook]

@Pennycook No worries!

1. A node with Y-Sort enabled is indeed included in the Y-Sort. Practically I don't expect this to come up much, the root of a y-sorting group is likely not to be a renderable element anyway. Regardless, it will work in that situation.
2. Yes, the new y-sort will stop collecting children the first time it encounters a Node2D without y-sorting - just like the existing behaviour!

The codebase can be tricky haha, I came back to this PR and I was in shambles trying to rebase this.

Thanks for the explanation, and sorry again for the noise!