-
-
Notifications
You must be signed in to change notification settings - Fork 21.3k
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] Backport CanvasLayer
visibility
#57900
Conversation
} | ||
|
||
return true; | ||
return visible && visible_in_tree; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the new variable called visible_in_tree
, if it is not whether the item is visible in the tree? Should this be parent_visible_in_tree
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In 4.0 it can depend on Window, which might not be a direct parent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could there be a better name for this? It strikes me as confusing.
In a sense with a window too, it is still the parent. Because the window is similar to a parent, and if the window isn't a direct parent, then the effects of the window are mediated by the direct parent?
At least that seems better than calling it something which it isn't, which is the case at the moment.
Essentially if I'm understanding correctly the logic is:
IF MY PARENT IS VISIBLE IN THE TREE and I'M VISIBLE then I'M VISIBLE IN THE TREE
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does ancestors_visible_in_tree
make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's much better than is_visible_in_tree
yes. Personally I think to stretch to using parent is fine here too, but we can see what others think. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, the classref uses antecedents.
if the node is present in the SceneTree, its visible property is true and all its antecedents are also visible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about antecedent - it is not really a commonly used English word, and most people would have to look it up to find the definition (including me as a native speaker 😀 ).
You could also use parent_and_window_visible_in_tree
potentially if worried about the window thing (not so relevant in 3.x but no problem having this carryover). Ancestors is fine too, to me it's a bit overkill because if your parent is visible in the tree, then by definition the ancestors are also visible.
But yeah ancestors is fine if you are struggling and don't want to use parent.
They do say naming things is one of the most difficult problems in computer science lol.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either way, changing is_visible_in_tree()
would break compatibility in 3.x
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The discussion is about variable name, which is the same as the method name, but does different thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then parent_visible_in_tree
is what it tracks I think? There's no Window
visibility involved in 3.x
.
e35482b
to
f49ffe4
Compare
Thanks! |
show()
andhide()
logic intoset_visible()
, so later changes are easier.visible
property andvisibility_changed
signal toCanvasLayer
.CanvasLayer
visiblity propagation of the original PR.hidden
signal so that it won't be emitted again when theCanvasItem
was already invisible.is_visible_in_tree()
is now as cheap asis_visible()