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

Update Y-sort position of the first item in the sorted subtree #79452

Merged

Conversation

kleonc
Copy link
Member

@kleonc kleonc commented Jul 14, 2023

Fixes #79451.

Also updated the docs as Y-sorting affects not only the child nodes, the CanvasItem with y_sort_enabled = true is being Y-sorted together with its children (always was, in a potentially buggy manner but still).

@kleonc kleonc added bug topic:rendering topic:2d cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release labels Jul 14, 2023
@kleonc kleonc added this to the 4.2 milestone Jul 14, 2023
@kleonc kleonc requested review from a team as code owners July 14, 2023 00:24
@kleonc kleonc requested a review from clayjohn September 16, 2023 15:36
@YuriSizov YuriSizov modified the milestones: 4.2, 4.3 Nov 9, 2023
@YuriSizov YuriSizov added cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release and removed cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release labels Nov 15, 2023
@akien-mga akien-mga requested a review from lawnjelly February 13, 2024 14:10
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tentatively approving, as the changes seem simple and I trust @kleonc's assessment. But I'm not competent on this rendering code so a review from Clay or lawnjelly would still be very welcome (but if they're busy, I'm happy to yolo merge this in a few days and we'll see in production if any edge case was forgotten).

Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not really sure about this code as I'm not familiar enough with the surroundings to know why the parent node was excluded from the y-sort in the first place, but I trust kleonc, so let's go ahead with a yolomerge

@clayjohn clayjohn removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Feb 20, 2024
@kleonc
Copy link
Member Author

kleonc commented Feb 20, 2024

I'm not really sure about this code as I'm not familiar enough with the surroundings to know why the parent node was excluded from the y-sort in the first place

AFAICT the docs being incorrect about that is a result of them being simply a reworded version of the old YSort node's description when it was changed in #42282. But in that PR the canvas items with y-sort enabled were made to be included within the y-sorting themselves so it would work properly.
In 3.x it seems to me like the VisualServer part was written taking in mind specifically a separate YSort node which were assumed to not draw anything on its own, and hence it was skipped within y-sorting. Not sure whether it was intentional or was just overlooked but there's e.g. #49165 which seems to be caused by this.

Also by always was I guess I meant in 4.x 🤔:

Also updated the docs as Y-sorting affects not only the child nodes, the CanvasItem with y_sort_enabled = true is being Y-sorted together with its children (always was, in a potentially buggy manner but still).

@akien-mga akien-mga merged commit dc2d1e3 into godotengine:master Feb 20, 2024
@akien-mga
Copy link
Member

Thanks!

@kleonc kleonc deleted the ysort-update-first-item-position branch February 22, 2024 16:52
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.

First Y-sorted CanvasItem in the subtree can be sorted incorrectly
4 participants