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 warning in scene tree when nested CanvasItems try to clip children #98949

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Meorge
Copy link
Contributor

@Meorge Meorge commented Nov 8, 2024

This PR provides a partial solution to godotengine/godot-proposals#11113 by displaying a configuration warning in the scene tree when a CanvasItem attempting to clip its children has an ancestor CanvasItem that also clips its children:

CleanShot 2024-11-07 at 21 19 58@2x

Warning

The warning notably appears on the child node attempting to clip, not on the parent that is successfully(?) clipping. This does mean that if there is a scene instance that contains within it a clipping node, the error may still be invisible to the user:

CleanShot 2024-11-07 at 21 48 54@2x

Placing a warning on the parent node would fix this, but it would also require each parent to do a recursive walk of its descendants in the best (and hopefully usual) case of no nested clipping. I feel like the performance penalty that this would incur would be a bit too much.

A fair number of files had to be modified so that this PR worked for all of those classes that inherited from CanvasItem; previously, their get_configuration_warnings methods assumed that the relevant superclass for them to call was Node, but since this PR adds get_configuration_warnings to CanvasLayer, which is in between them and Node, they had to be updated.

@AThousandShips
Copy link
Member

A fair number of files had to be modified so that this PR worked for all of those classes that inherited from CanvasItem; previously, their get_configuration_warnings methods assumed that the relevant superclass for them to call was Node, but since this PR adds get_configuration_warnings to CanvasLayer, which is in between them and Node, they had to be updated.

You are very far behind in your branch, that was fixed two months ago in:

@Meorge
Copy link
Contributor Author

Meorge commented Nov 8, 2024

You are very far behind in your branch, that was fixed two months ago

Oops, that made me realize that while I've been making sure to fetch my local repo, it's just been my (now very outdated) fork and not the official Godot repo that I've been branching off of. 😅 Thanks for catching that - I'll sync my fork and then see if I can rebase the necessary changes over to it! If not, I'll close this PR, copy my changes over to a new branch off the modern master branch, and resubmit the PR.

@Meorge Meorge force-pushed the canvasitem-clip-children-warning branch from 0bd6267 to 84df8fb Compare November 8, 2024 16:37
…ve a clipping ancestor

Many subclasses of CanvasItem had to have their `get_configuration_warnings` methods updated so they would display the warning as well

Make clip children warning update whenever clip_children_mode is modified

Update scene/main/canvas_item.h

Co-authored-by: A Thousand Ships <[email protected]>
@Meorge Meorge force-pushed the canvasitem-clip-children-warning branch from f6f67fb to 1e15016 Compare November 8, 2024 16:58
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.

This looks good to me. But this should be adjusted to also check for CanvasGroups which use the same clipping feature internally. Therefore CanvasGroups can't be children of nodes with clip_children and nodes with clip_children can't be children of CanvasGroups

@Meorge
Copy link
Contributor Author

Meorge commented Nov 9, 2024

Thanks, I've added a warning for that as well! It continues moving up the tree until both warnings have been triggered, so that if someone happens to meet both conditions, they are aware of both instead of thinking that there was only one they needed to satisfy.

It did get me wondering, though - do CanvasGroups behave incorrectly if they are children of a clipping node or another CanvasGroup, and if so, should warnings for those cases also be added to this PR?

Edit: Just tested, and yes it appears that Godot does not like nested CanvasGroups. I'll add a warning for CanvasGroup when they detect an ancestor also being a CanvasGroup or having a clip setting other than disabled.

@Meorge Meorge force-pushed the canvasitem-clip-children-warning branch from c49e525 to 9eeb4b9 Compare November 10, 2024 03:56
@Meorge
Copy link
Contributor Author

Meorge commented Nov 10, 2024

Warnings have been added for:

  • CanvasItems, when an ancestor is a CanvasGroup
  • CanvasGroups, when an ancestor is either a CanvasGroup or clips its children

CleanShot 2024-11-09 at 19 55 09@2x

@Meorge Meorge requested a review from clayjohn November 10, 2024 03:59
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.

3 participants