-
-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
[3.x] Fix top level CanvasItem
visibility
#58870
Merged
akien-mga
merged 3 commits into
godotengine:3.x
from
timothyqiu:canvas-item-visibility-3.x
Mar 7, 2022
Merged
[3.x] Fix top level CanvasItem
visibility
#58870
akien-mga
merged 3 commits into
godotengine:3.x
from
timothyqiu:canvas-item-visibility-3.x
Mar 7, 2022
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
It would be clearer for the Git history if you could do this as actually |
This reverts commit 642591b.
timothyqiu
force-pushed
the
canvas-item-visibility-3.x
branch
from
March 7, 2022 17:36
888bba4
to
b76147e
Compare
I meant you could revert this commit no? f49ffe4 Ah though I guess you wanted to keep some of the changes of that commit. It's fine as is :) |
akien-mga
approved these changes
Mar 7, 2022
Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR first reverts the backported #57684 (part of #57900) and #58386.
#57684 was backported mainly because it solves a bug where
But after inspection these days, I think that is the expected behavior.
CanvasItem
s that are explicitly marked as top level or have a non-CanvasItem
parent were handled together as "top level", and they are logically parented to the nearest ancestorCanvasLayer
orViewport
world canvas. SeeCanvasItem::_enter_canvas()
.After the revert, the only problem is that
CanvasLayer
visibility propagation was only propagated to direct childCanvasItem
s. Fortunately, all these "top level" nodes are in a group calledroot_canvas{canvas_rid}
, so moving the propagation logic into acall_group()
is all that have to be done I think.The editor gizmo fix from the reverted 642591b is kept.
I also stopped propagation onto explicitly top level
CanvasItem
s because:CanvasLayer
.CanvasLayer
, we propagate to theseCanvasItem
s withcall_group()
.godot/scene/2d/canvas_item.cpp
Lines 385 to 387 in 6b4d7d2
CanvasItem
s are only controlled by their logical parent insideVisualServer
. So propagating onto top level nodes won't actually change anything.Tested recent regressions like #58251, #58315, and #58782. They are not reproducible.
Fixes #58782