[3.x] Add option in VisibilityEnabler2D to hide the parent for better performance (reverted)

[BimDav]

As shown in this video: https://www.youtube.com/watch?v=YYoLqAEpHmo&t=24s, canvas items that are not rendered still use a lot of computing time, which can be reduced greatly by setting their visible property to false.
With this change, VisibilityEnabler2D can set its parent's visibility when entering/exiting the screen, making this optimization more accessible to users.
visibility_optimization.zip
Use this project to test the speedup

[fire-forge]

I think a better name for the property would be hide_parent. visibility_parent doesn't make much sense from a grammar perspective.

[lawnjelly]

This looks interesting, some good observations, and this it likely to be a useful approach. 👍 😁

I have two concerns:

States

One a small implementation detail, this will be automatically hiding / showing objects that the user may have previously set to a certain visibility state, and thus could lead to user confusion. One common way of dealing with this is with separate flags, e.g. user_visible, cull_visible and master_visible, or something like this. You might get some ideas from looking at the 3D visibility notifier as it has to deal with similar problems.

Why is 2D hierarchical culling not working already?

The other is more major - this may be pinpointing something that the engine should be doing already automatically (i.e. hierarchical culling), but has not been implemented / is not working correctly. The best way to proceed imo is to make an issue with the minimum reproduction project showing the problem then we can profile, have a look, and find the best overall way of fixing this in the engine (rather than leaping to the first fix).

I've not looked at the 2D culling, but it could even be there is none to speak of (it may have been previously relying on users using TileMap, which may do some of this internally). In which case we should be doing a review of this first, to decide whether we should be using e.g. hierarchical culling in order to make this sort of thing automatic rather than have to have the users worry about creating VisibilityEnablers.

The danger is that if we add this functionality and then later fix automatic culling, we then have to remove a feature and break backward compatibility / confuse users, so it is usually better to spend a little time investigating it before jumping to a solution.

Just to emphasise I haven't looked at all at the 2D culling yet, we would welcome input from anyone who is familiar (although I'm not sure who else is familiar with this area currently aside from reduz). But I will try and have a look after the weekend (and the OP or anyone else is welcome to take a look and give us a summary).

[lawnjelly]

This needs an entry to describe the constant.

[lawnjelly]

@BimDav I've managed to get stalled on something else so spent a little time today looking into this.

As far as I can see, rather surprisingly, there is no hierarchical culling (or even efficient culling) going on whatsoever for the canvas items, and you are absolutely right this is doing a bunch of unnecessary processing. This PR is therefore a valid way of handling the problem.

It has always seemed somewhat strange to me that Godot doesn't have automatic culling and throttling of processing for objects off the screen, both in 2D and 3D, and instead relies on the user manually creating VisibilityNotifiers. This was vaguely plausible for the 3D, but for 2D I can see the scene tree hierarchy is being sent to the VisualServer, so I'm surprised it is not doing all this already.

When I had a look at the VisibilityEnabler2D code I saw it was toggling a visible flag, so I tried just making the enabler the root node of the subscene. I figured out this wasn't working - and the reason is that rather confusingly there is a visible flag in the enabler, which shadows an identical visible flag in the CanvasItem. When I changed the call in the enabler to use set_visible() instead of changing the flag directly, having the enabler as root worked.

Nevertheless, given that the rest of the enabler works using the parent node, I agree with your approach of working this through the parent node. I am of the opinion that the visible flag in enabler should be renamed to avoid this confusion of it shadowing another variable, but this can be done in another PR.

The two questions I initially posed above remain, if we proceed we should probably do it via a secondary visible flag. However given the hierarchy is stored in the VisualServer it may be possible to do all this automatically to some extent. I'm not clear yet whether that would be a better approach or could be done in addition to offering this approach in the VisibilityEnabler.

I can only imagine this doesn't come up more often because most people use tilemaps to create large numbers of 2D sprites, which have a bunch of sprites as commands in each item, so the inefficiency is less of a problem.

For an automatic approach we could for example maintain bounding boxes for each item (plus children), and therefore cull away branches quickly. This of course depends on whether people construct their 2D scenes in a spatially friendly manner. If you e.g. create 30000 children sprites and place them all over a map with one parent, this won't help a lot (and in fact could be counterproductive). This is perhaps one of the reasons for the VisibilityEnabler approach.

Alternatively we could just use a spatial partitioning scheme like the BVH and do a pass culling the items to the viewport prior to rendering.

Overall though as a quick fix I'm tempted to agree with your approach. Worst case if we do add some automatic culling, the VisibilityEnabler will probably be more efficient in some circumstances.

I think rather than explain the flags approach, it would be fine to merge this as is (providing any review comments above are addressed), and I can do a small follow up PR to sort out the flags situation to make it a little more user friendly.

[lawnjelly]

Approve providing the comments are addressed:

• Constant needs an entry
• hide parent seems to be preferred

[BimDav]

The problem with automatic hierarchical culling is that a child can have a visibility_rect that is larger than its parent's. There can also be a node with set_as_toplevel(true) at the bottom of a deep hierarchy. It's probably possible, but there are probably a lot of niche cases, right?

[BimDav]

It would be great to add this to 4.0 as well, but the last time I checked, the early return
if (!visible) return;
was not there (making the rendering super slow), and visible was not accessible from the function doing the culling, so I was not able to easily add it

[lawnjelly]

The problem with automatic hierarchical culling is that a child can have a visibility_rect that is larger than its parent's. There can also be a node with set_as_toplevel(true) at the bottom of a deep hierarchy. It's probably possible, but there are probably a lot of niche cases, right?

Yes, you have to traverse the hierarchy to maintain the bounds, and there are niche cases to take care of. I actually had a bash at the automatic hierarchical culling yesterday afternoon and have it mostly working, your test project rendering in debug goes from 23fps to 17000fps (though obviously this is best case). It seems to render the editor okay, but as you say I have to check for regressions / special cases.

With hierarchical culling, as well as culling off an entire branch, you can also determine that an entire branch is within the viewport, and thus remove all the intersects checks later in the traversal. The cost is in storing and maintaining the bounds.

Regardless, I think the addition to the VisibilityEnabler makes sense.

It would be great to add this to 4.0 as well

I did ask in rendering about this as I'm not so familiar with 4.x - @KoBeWi pointed out that things were quite different in 4.x, so it might require a different approach there.

[KoBeWi]

Yeah in 4.0 VisibilityEnabler takes effect depending on whether it's actually rendered, so making it invisible would just disable it permanently.

[akien-mga]

Thanks!

[lawnjelly]

Just looking at #77613 , I noticed that @BimDav never made the recommended changes before this got merged. This was my fault I shouldn't have prematurely approved.

I'll do a PR to fix these two points (and look at the flags issue I mentioned earlier). I'm also suspecting (with lack of MRP so far) that the VisibilityEnabler part of #77613 is caused by this PR defaulting to true. While it does make some sense to default to true (as most people won't change these inspector settings), it is slightly compat breaking, especially I suppose for situations where people have set up the Enabler incorrectly.

Given that hopefully we will merge #68738 soon, which solves this problem in the general case, then the VisibilityEnabler version is useful only in a subset of cases and might make more sense to default to false (which is more backward compatible).

[BimDav]

@lawnjelly sorry about that, I seem to have completely missed some GitHub notifications. I can do the requested changes, as well as making it defaulting to false

[lawnjelly]

It's okay, I can do it as I probably will combine with some flag stuff at the same time. 👍

[lawnjelly]

Quick recap: It is imperative that the flags need to be solved for this PR to behave sensibly, because if a user changes visible flag to false, this PR can set it to true even when the user wants the item hidden permanently.

I had assumed this wouldn't be too bad, but it turns out the flags situation is a barrel of worms. I've separated the visible flags in CanvasItem into a requested flag, a enabler flag (for the VisibilityEnabler) and a master flag. This is usually the pattern for this kind of thing.

The problem is we have a boatload of code already written in the engine that already relies on the set_visible() and is_visible() logic. set_visible() is obvious to set the requested flag, which may or may not flip the master flag. The problem comes when we decide what to return from is_visible(). Should it be the requested flag, or the master flag?

If we return the master flag, we learn whether the item is visible after the enabler is applied. But we have the illogical situation that set_visible(true) followed by is_visible() can return false. So it seems more likely that is_visible() should return the requested flag. But I'm slightly concerned that there might be code that might rely on this being that actual visibility.

Alternatively we could have a look at solving the whole multiflag logic within the VisibilityEnabler. Although I'm not at all sure that is technically possible. Consider that a visible item is hidden by the VisibilityEnabler, the user then calls set_visible(false). This state is lost in the set_visible() no-op check, then the VisibilityEnabler erroneously sets visible to true again when the item comes on screen.

I'm left wondering whether it might be wise to revert this PR (at least temporarily), apply #68738 and see if that solves the bottlenecks in this area, in which case there is no pressing need for the VisibilityEnabler approach. I had previously thought #63193 might be more efficient in some situations, but I think we probably should test this empirically before moving ahead (or at least significant enough to outweigh the risk of flag logic bugs).

Anyway @BimDav let me know your thoughts, I'll see what @akien-mga thinks either here or rocket chat.

@BimDav also, it would be great if you could try the artifacts from #68738 and see whether it solves the performance problems in your game.

[BimDav]

I think I personally prefer is_visible() returning the actual visibility, but I may not have thought about it enough.

If #68738 is just around the corner, it makes sense to revert this PR. It seems that most people did not notice / were not bothered by the inefficiency, so in the future the same people won't know about this feature. The ones that really care about it either can implement the feature themselves using VisibilityNotifier, or use the core feature of #68738. I just thought that since VisibilityEnabler existed, it made sense to add this, and I am also very glad that it made the underlying issue more visible so that it will be actually fixed by your work.

[lawnjelly]

It's tricky, because if we push the flag logic to CanvasItem, it is pushing complexity upstream, and it might end up forcing everyone else in the engine to be conscious of the distinction between requested and actual visibility, which is a bit of a cognitive load and bug surface.

I can't immediately see an easy way we could keep this flag logic within VisibilityEnabler2D (that would be ideal). But maybe there's a cunning way that we've not come up with yet, needs a eureka moment.

This is all assuming we go ahead with #68738 . If we didn't for some reason, then this approach would again seem one of the best compromises to solve the problem.

[akien-mga]

Reverting with #78182 as discussed above.

At this stage it seems very likely we'll proceed with #68738, it's just pending a final check from Clay.