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

Add editor check for compatible visibility layers for CanvasItem #93351

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AThousandShips
Copy link
Member

@AThousandShips AThousandShips commented Jun 19, 2024

The wording might need some improvement, and need confirmation that this catches the full behavior of this feature. It doesn't check the cull layers of any viewport, can add if desired but keeping it simple for just the hierarchy (and an error for the viewport might be annoying if you want them to be invisible at the start just by the layers, but the hierarchy itself would usually be an accident I'd say)

In short: This checks that the condition for visibility is met, i.e. that a CanvasItem has at least one layer shared with all parents until either the root or one parent that is top-level. To avoid annoying potential false positives it also doesn't check if the layers for the node itself is empty, that feels like something intentional and shouldn't throw a warning

Correctly updates when layers of parents update, as well as when top-level changes for parents and self

See (I'd say regardless of that one being merged this one is useful, but not vice versa):

@AThousandShips
Copy link
Member Author

There, changed to using get_parent_item and moved the configuration update to _top_level_changed which is propagated to child nodes to future proof it with that part, and left a note in the layer setter

@@ -500,6 +523,7 @@ void CanvasItem::_top_level_changed() {
child->_top_level_changed_on_parent();
}
}
update_configuration_warnings();
Copy link
Member Author

Choose a reason for hiding this comment

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

I can move this back to set_as_top_level if there would be performance concerns calling it down a deep tree and add a note to this as well, works either way

@JonqsGames

This comment was marked as resolved.

@AThousandShips

This comment was marked as resolved.

@JonqsGames

This comment was marked as resolved.

@AThousandShips

This comment was marked as resolved.

@AThousandShips

This comment was marked as resolved.

@AThousandShips
Copy link
Member Author

So if someone on the rendering team could confirm some details of the functioning here if there's something missing, to clarify:

  • This seeks to catch cases where the hierarchy is obviously unworkable, where there's little to no reason to assume a setup is valid, where a node can't possibly be visible
  • It doesn't care about the flags on the viewport, partially because this isn't easily accessible in the editor, and partially because that could be something the user wants to change to make nodes visible or not later

Here's the assumptions made that I'd like verified (based on the wording in the docs and other conversations as well as reading the code):

  • Top level nodes are considered the root for these calculations
  • The check stops with the top most canvas item, either having no canvas item parent, or being top level (based on the server parent assignment code elsewhere), and doesn't jump in the hierarchy or otherwise check for nodes higher

In other words, is there potentially something further needed than the direct traversal of canvas item parents here

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.

Viewport cull mask and canvas item visibility layer not working
3 participants