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] Fix top level CanvasItem visibility #58870

Merged
merged 3 commits into from
Mar 7, 2022

Conversation

timothyqiu
Copy link
Member

@timothyqiu timothyqiu commented Mar 7, 2022

This PR first reverts the backported #57684 (part of #57900) and #58386.

#57684 was backported mainly because it solves a bug where

CanvasLayer could propagate its visibility to CanvasItems that were separated by plain Nodes.

But after inspection these days, I think that is the expected behavior. CanvasItems 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 ancestor CanvasLayer or Viewport world canvas. See CanvasItem::_enter_canvas().

After the revert, the only problem is that CanvasLayer visibility propagation was only propagated to direct child CanvasItems. Fortunately, all these "top level" nodes are in a group called root_canvas{canvas_rid}, so moving the propagation logic into a call_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 CanvasItems because:

  • Their logical parent is a CanvasLayer.
  • For CanvasLayer, we propagate to these CanvasItems with call_group().
  • There was previously a comment:
    if (c && c->visible) { //should the toplevels stop propagation? i think so but..
    c->_propagate_visibility_changed(p_visible);
    }
  • Previously, the visibility of CanvasItems are only controlled by their logical parent inside VisualServer. 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

@akien-mga
Copy link
Member

It would be clearer for the Git history if you could do this as actually git revert commits + another commit on top with your further changes.

@timothyqiu
Copy link
Member Author

timothyqiu commented Mar 7, 2022

It would be clearer for the Git history if you could do this as actually git revert commits + another commit on top with your further changes.

The backport of #57684 is part of #57900. It can only be reverted manually. Should I do that as a separate commit too?
OK, I'll do a fake revert commit ;)

The editor gizmo fix from previously reverted
642591b is kept here.
@akien-mga
Copy link
Member

akien-mga commented Mar 7, 2022

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 akien-mga merged commit 927dd95 into godotengine:3.x Mar 7, 2022
@akien-mga
Copy link
Member

Thanks!

@timothyqiu timothyqiu deleted the canvas-item-visibility-3.x branch March 8, 2022 01:48
@timothyqiu
Copy link
Member Author

I meant you could revert this commit no? f49ffe4

#57900 backports 3 PRs in that one commit f49ffe4. It's the last one of the three that needs revert. CanvasLayer visibility is kept.

I think I should have made that backport PR 3 separate commits in the first place :(

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.

None yet

2 participants