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] Consistent render ordering for CanvasLayers #69952

Merged
merged 1 commit into from
Apr 25, 2023

Conversation

lawnjelly
Copy link
Member

@lawnjelly lawnjelly commented Dec 12, 2022

Maintains scene tree ordering for CanvasLayers that share identical layer ID.

Fixes #25384

Notes

  • This is a far more correct fix for CanvasLayers than previously attempted.
  • It adds the overhead of maintaining a tree layer order, which in most circumstances should not be a problem, but is suspectible to the "notification explosion problem", so should ideally be used in conjunction with [3.x] Add deferred notifications #65581 and [3.x] Faster queue free #62444 .
  • Given we are traversing the scene tree to establish render order, we could also consider the possibility of storing a render order for all nodes (if this would be useful for other purposes, killing two birds with one stone). This would involved some more effort to maintain though, as all nodes changes would require reordering (or maybe it could be done in the VisualServer directly during rendering).
  • Same may be applicable to master if it has the same problem.

@lawnjelly lawnjelly requested a review from a team as a code owner December 12, 2022 11:51
@lawnjelly lawnjelly modified the milestones: 3.x, 3.6 Dec 12, 2022
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.

Changes look fine to me. I don't love having to traverse the whole scene when a CanvasLayer moves, but I think we are stuck doing it here so we can limit the check to when a CanvasLayer enters/moves/leaves the scene

Maintains scene tree ordering for CanvasLayers that share identical layer ID.
@lawnjelly lawnjelly force-pushed the canvas_layer_ordering branch from 93d639e to 1730fab Compare March 12, 2023 05:00
@lawnjelly
Copy link
Member Author

lawnjelly commented Mar 12, 2023

Just pushed a small bugfix, I noticed a couple of things:

  • I was only calling set_layer() on the moved canvas_layer. This didn't sound correct, as the ordering of the other canvas_layers could also be shifted as a result, so now I'm calling set_layer() on each layer.
  • The DEV_ENABLED warning was incorrect and likely to trigger, so I removed it.
    This was because historically I had previously been incrementing _layer_order_in_tree for each Node, but that is no longer the case, it is now only incremented for canvas_layers (to make debugging in VisualServer easier).

I don't love having to traverse the whole scene when a CanvasLayer moves, but I think we are stuck doing it here so we can limit the check to when a CanvasLayer enters/moves/leaves the scene

Yes it's a bit of a pain but it is the simplest implementation, and without evidence of this causing a problem it probably isn't worth optimizing at this stage (which would complicate the code and could lead to bugs).

Merging note

I would advise not cherry picking to 3.5 until we have tested in at least a 3.6 beta, due to the potential for the scene tree traversal to cause performance issue in unforeseen situation.

@akien-mga akien-mga merged commit 54e293b into godotengine:3.x Apr 25, 2023
@akien-mga
Copy link
Member

Thanks!

@lawnjelly lawnjelly deleted the canvas_layer_ordering branch July 14, 2023 19:47
@akien-mga akien-mga changed the title Consistent render ordering for CanvasLayers [3.x] Consistent render ordering for CanvasLayers Sep 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants