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

[3.x] Backport CanvasLayer visibility #57900

Merged
merged 1 commit into from
Feb 10, 2022

Conversation

timothyqiu
Copy link
Member

}

return true;
return visible && visible_in_tree;
}
Copy link
Member

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?

Copy link
Member

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.

Copy link
Member

@lawnjelly lawnjelly Feb 10, 2022

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

Copy link
Member Author

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?

Copy link
Member

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. 👍

Copy link
Member Author

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

Copy link
Member

@lawnjelly lawnjelly Feb 10, 2022

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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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.

@akien-mga akien-mga merged commit c7f09c1 into godotengine:3.x Feb 10, 2022
@akien-mga
Copy link
Member

Thanks!

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.

4 participants