Allow Visibility to be inherited

[james7132]

Objective

Fixes #4907. Fixes #838.
Supercedes #2087. Supercedes #865.

Visibility is currently entirely local. Set a parent entity to be invisible, and the children are still visible. This makes it hard for users to hide entire hierarchies of entities.

Solution

Expand Visibility to hold both a "self" visibility, and a "inherited" visibility. The entity is visible if and only if both are true. A system similar to transform_propagate_system is then implemented to propagate visibility down through the hierarchy. Unlike Transforms, the action of setting visible or not is relatively light computationally so zero change detection is used for the system.

The visibility_propagate_system can run in parallel with transform_propagate_system, and is set as a dependency of check_visibiility, which will copy the same state to ComputedVisibilty once propagated. This structure is to avoid making a serialized chain of transform_propagate -> check_visibiliity -> visibility_propagate. Due to the lighter nature of the visibility propagation, visibility_propagate_system will almost always finish faster than transform_propagate_system for perfectly overlapped hierarchies, so there is zero overhead on platforms that support multithreaded execution (basically everywhere except wasm32 platforms right now).

---

Changelog

TODO

Migration Guide

TODO

[LoopyAshy]

I was thinking about this very change recently when working on my project and I naively presumed adding Visibility & ComputedVisibility to the parent and setting it to false would make all the children invisible but this did not happen.

As such I am in support of this change.

[ManevilleF]

Since Visibility::inherited is computed by the propagation system, shouldn't it be in ComputedVisibility ? Otherwise Visibility has a one field for the user to change and the other that shouldn't be touched.

[james7132]

Since Visibility::inherited is computed by the propagation system, shouldn't it be in ComputedVisibility ? Otherwise Visibility has a one field for the user to change and the other that shouldn't be touched.

It's either this current implementation or require ComputedVisibility for propagation, which isn't true for for some extraction pipelines like lights and sprites. Also only fetching Visibility does make the queries faster. It'd also preclude check_visibility from doing a full scan clear at the start, instead requiring trusting the prior state.

[mockersf]

The propagation system will always change the components, so it becomes impossible from the user to rely on Changed<Visibility>

[james7132]

The propagation system will always change the components, so it becomes impossible from the user to rely on Changed<Visibility>

This is indeed a downside, though I guess we could put change detection back in to remedy this. My question is if we actually need change detection here. Even with this change, Visibiilty is still 4x smaller than ComponentTicks. It might actually be faster to iterate over them without change detection than rely on the filter. Also the final "output" type ComputedVisibility also is constantly updated as well, so it's not clear how much of a loss this would be.

[cart]

Change detection isn't just about optimization. It can be used for implementing logic. Ex: "send an event every time visibility changes for an entity"

[james7132]

Change detection isn't just about optimization. It can be used for implementing logic. Ex: "send an event every time visibility changes for an entity"

OK, added a few small changes to then avoid triggering it when nothing has actually changed. This still keeps the same structure as before, but Changed<Visibilty> will trigger when either the local visibility or an ancestor's visibility has changed. Should make it easier to detect when an entire tree changes visibility.

[james7132]

@alice-i-cecile removing the Controversial tag since I think the only point that was of major concern was the change detection issue. Unless the overall design is of a concern, which I think might be quelled by the fact that cart suggested basically the same API about a year ago.

[superdump]

This looks good to me and I imagine you've done benchmarking to see that it doesn't significantly impact performance. There are some 'stress tests' for transform hierarchy that could probably be leveraged for testing if not.

[superdump]

This is consistent with TransformPropagate and so I think it should remain like this for this PR but I'd prefer VerbNoun like PropagateTransform and PropagateVisibility. It just reads better. :)

[superdump]

I have never understood why one might want to use the .spawn().insert() pattern and not just basically always .spawn_bundle((Component,)). Is there a reason?

[ManevilleF]

I find this cleaner than creating a tuple with a single element

[james7132]

It also makes no perf difference: spawn_bundle on a single element still does a spawn + bundle insert with a single component. I think the only thing you spare is the command encoding.

[james7132]

This looks good to me and I imagine you've done benchmarking to see that it doesn't significantly impact performance. There are some 'stress tests' for transform hierarchy that could probably be leveraged for testing if not.

Whoops, forgot to put those results up in the original one as perf justification for this approach.

Just to verify, I looked to ensure that the system parallelized with transform propagation, and blocks check_visibility, and it does. A slice from many_cubes sphere:

many_cubes sphere has a flat hierarchy with both transform and visibility on every entity. On it we see transform_propagate take 506.6us (mean), and visibility_propagate take 288.86us (mean).

For a stress test with deeper hierarchies, I tried many_foxes which has a deeper hierarchy. transform_propagate takes 1.44ms (mean) and visibility_propagate takes 770ns (mean).

Unfortunately I don't think we have many stress tests that target both in deep hierarchies.

[superdump]

There's the transform_hierarchy stress test that has a variety of configurations. And I was thinking more of a before/after comparison, though the breakdown of how much time visibility_propagate takes is at least promising for that part.

[james7132]

There's the transform_hierarchy stress test that has a variety of configurations. And I was thinking more of a before/after comparison, though the breakdown of how much time visibility_propagate takes is at least promising for that part.

The visibility propagate currently requires visibility to be at every level of the hierarchy (like transforms): the transform hierarchy stress test wouldn't be a good test. I could try reworking it to support hierarchies with gaps, but this would be a huge hit to perf as it would likely match all entities, not just those bound to the hierarchy.

[superdump]

Fair enough. Sounds like we need a new test then.

[superdump]

Oops. Finger slipped.

[cart]

Broadly in favor of this, but I still think ComputedVisibility should own this info leaving Visibility as the "user-defined configuration component".

[cart]

This should have a constructor to make disabling visibility "inline" during entity spawning possible. As it stands, this requires defining calling visibility.set_visible(false) on an intermediate variable.

[cart]

I do still think this belongs on ComputedVisibility, and that we should be encouraging people to write systems that read from ComputedVisibility when they care about "actual visibility of a thing".

The stated counterarguments to this (lights and sprites), are both examples of things that should probably be using ComputedVisibility (sprites will probably ultimately support some sort of visibility culling and they will definitely need to support RenderLayers. same goes for lights). Earlier in the new renderers' lifecycle I did make the call not to include ComputedVisibility for sprites for performance reasons, but we have enough usability reasons to include it at this point (multiple cameras / controllable render layers / visibility inheritance / etc) that i'm convinced its worth the cost.

[cart]

I'm currently putting together the changes I outlined above (store computed hierarchy visibility on ComputedVisibility) so we can compare performance and make a call. This will also move sprites and ui to ComputedVisibility and will enable RenderLayers functionality. So it would also be an alternative to #5114 and #4007. I suspect it to perform worse than #5114 because it will be populating camera "visible entity" lists. But imo unifying these systems makes a lot of sense. Keeps the code smaller and the behaviors in sync / consistent. And if the current impl isn't good / fast enough for sprites, is it really good enough for 3d meshes? Unifying them would create pressure to optimize the system as a whole, which I like.

[cart]

Still working out implementation details, but heres some bevymark performance numbers (100,000 sprites) with various approaches:

• main: 0.017612s
• PR: 0.017564s
• Move hierarchy propagation to ComputedVisibility and port sprites, default them to "visible in view" and filter them out of check_visibility (skip view assignment, render layers, frustum cull option branching, etc): 0.017614s
• Move hierarchy propagation to ComputedVisibility and port sprites (no other changes ... sprites are processed in check_visibility): 0.018317s
  • note that this doesn't include porting sprite rendering to use the computed visibility lists. ill do this next

What this tells me is that the ComputedVisibility switch doesn't inherently change the cost (which makes a lot of sense to me), but adding render layer filter checks, checking for (non existent) frustum culling, and populating each view's "visible entity list" does have a cost. And that feels like a cost we should pay (but also try to minimize as much as possible).

[cart]

I think I have everything working at this point. I added fixedbitset-based view-visibility-checks to the queue_sprites system, which result in a bevymark time of 0.019092s for 100,000 sprites, which is a ~1.53 millisecond increase over this pr. This is still much better than using a hashset (like #4007), which runs in 0.021622s.

So as expected this definitely isn't free, but it buys us:

1. Consistency: ComputedVisibility behaves as expected for all entities and is "the one way" to check if an entity is visible. VisibleEntities is available in both 2d and 3d. Frustum culling could be implemented using the same pattern for both 2d and 3d. And because it is done on the app side it means (a) users can write custom app logic that feeds on the visibility data and (b) extract logic can feed on the visibility data.
2. Visibility is "user config only" and ComputedVisiblity is "computed results only", which I like
3. RenderLayers + multiple views now work for sprites. But note that this pr's current impl could be paired with something like Make sprites respect RenderLayers #5114 for much cheaper functionally-equivalent behavior. The only difference would be points (1) and (2).

That being said, merging this puts us into "performance debt". Leaving 1.53 milliseconds on the table on bevymark is a big deal and isn't something we should be doing for what at this point are "aesthetic reasons". I suspect the real world performance gap will close slightly if we were to add sprite frustum-like culling, given what I said in (1b), but thats a theoretical gain for a not-yet-implemented feature. I'm gonna sleep on this, but I'm starting to lean toward merging this pr as-is, in combination with #5114. Then next release cycle we can try to make a consistent better-performing ComputedVisibility app-side approach happen.

Heres a "split screen" example that uses render layers to hide the blue sprite on the left camera.

And here is my current branch: https://github.com/cart/bevy/tree/computed-visibility

[superdump]

2D frustum culling could maybe help for something like many_sprites with lots of off-screen sprites, but it’s possible that clipping them in rasterisation is faster than CPU frustum culling, as the geometry is so simple. For bevymark it would only be a cost as everything is on screen and in view.

The gains for the bevymark case seem to be about reducing overdraw and not recreating the vertex buffer, rather doing instancing/vertex pulling of some kind to deduplicate all the vertex attribute data into per-instance data, including the position and orientation of the sprite. The combination of using an instanced draw of 6 vertices where the instance contains translation and half extents, and using a depth buffer and greater than depth comparison function made bevymark hit 120fps up from 60fps for 160k sprites in batches of 10k on an M1 Max. Using a depth buffer for transparent sprites is wrong but it demonstrates the value. With just instancing there was no performance gain. With just the depth buffer there was no performance gain. I guess as GPU-side rendering is in parallel with main app simulation and render app CPU-side rendering, the bottlenecks must be on both CPU and GPU.

I think sprite performance could benefit from opaque and alpha mask and a depth buffer but transparent stuff would still cause performance problems due to back to front rendering and overdraw. I discussed on Discord that we can make tighter-fitting meshes and use UV mapping like Unity and TexturePacker do and that would reduce overdraw though it makes batching more complicated and moves sprites more toward Mesh2d. Batching would still line up with the ‘one big vertex buffer + meshlet’ approach I’ve been discussing in the GPU instancing issue. I think that is a good direction from a rendering perspective, but it’s a more complicated workflow so I would only want to do it when helpers/other infrastructure is in-place. Such as asset pipeline and asset metadata stuff to allow a sprite/spritesheet import workflow that automatically generates the meshes and uvs internally.

Coming back to the topic of this PR - even if frustum culling is only a cost for bevymark, one can opt out of doing it using the NoFrustumCulling component. Also, culling for the many_sprites problem could probably be better served by some coarse space division like a tiled world and just load all the content from the tiles that intersect the screen, maybe simulating some entities anyway but marking them as not visible to skip the more costly frustum culling if you know they’re in a tile that is off screen or something.

I feel like the bevymark case is unrealistic as it stacks lots of sprites on top of each other to the point that you can’t really see what is going on. It’s still a good stress test. many_sprites is perhaps more reasonable but there’s just a lot of sprites off screen for which there are strategies to reduce the load. So maybe we don’t need to worry too much about artificial extreme cases for sprites?

[cart]

Thanks for the update on what you've been exploring. I love the extra context on culling and im glad we've got more perf wins ahead of us.

For clarity: I'm testing with bevymark largely because it highlights per-drawn-entity CPU costs in a roughly-linear and reasonably consistent way, not because its a great real world benchmark. Its a good way to "isolate the variables" that are affected by the changes being made. The GPU-side costs of bevymark (in the context of this pr) are equivalent across implementations, so we can easily compare frame times. And we'd be paying these per-entity costs with or without frustum (or some other) culling, so it feels orthogonal.

[superdump]

Yup, I agree that they're useful stress tests to focus on performance of various relevant systems and hopefully by now it's clear that I care about performance. :)

That said, I'm asking the question - do we need to prioritise improving sprite performance from roughly where we're at compared to other things? My guess is that having some 10s or maybe 100s of sprites on-screen, or at a stretch some 1000s, is going to be the vast majority of use cases for real applications. Maybe some Touhou-style bullet-hell shooter would have a lot of sprites but I would still doubt anywhere near 100k. If we think the answer is that we don't need to prioritise it so much from where we're at, and my argument was also that there are other ways to improve performance if really needed, then taking the performance hit for consistency of ComputedVisibility and company sounds like the right call as that should improve usability and maintainability.

[cart]

That said, I'm asking the question - do we need to prioritise improving sprite performance from roughly where we're at compared to other things?

That is a welcome dose of pragmatism. Thanks for the (friendly) bonk on the head. Lets review the changes and a make a call as a group. I think the changes are substantial enough (and the context has changed enough) to merit a separate PR with a new description. That will also make it easier to merge this PR if we decide my changes are the wrong direction. I maintained the original authorship of all commits in this pr and @james7132 still deserves all of the credit for the inheritance logic (and the "first position" author slot on the blog post section for this feature).

[alice-i-cecile]

As someone heavily interested in making 2D games, including performance intensive one, I'm extremely okay sacrificing rendering performance here for more features and a better experience.

[cart]

New pr is up! Still a few changes to make / things to test (hence the draft), but please do start looking at it + forming opinions.
#5310

[alice-i-cecile]

Closing this out to collect efforts there.