Visibility as a marker component

[ManevilleF]

Objective

The Visibility component could be a marker component allowing simpler and probably faster querying

Solution

I changed the Visibility component to a marker struct Visible and adapted the queries.

This is a breaking change that could be expanded to ComputedVisibility

Associated discussion: #3833

[superdump]

I think we should consider computed visibility at the same time, and benchmark this. As noted on Discord, for 2D/UI graphs where the computed visibility is only (for now?) dependent on the user-configured visibility and the render layers, and as such it is the usual case that there would not be large numbers of visibility changes happening very often, I think this should be a win for query speed. It would be interesting to see if it has a noticeable impact on bevymark for example.

For 3D we have frustum culling that impacts the computed visibility. As the view is moved around, which will happen all the time in basically all types of 3D game, entities will change visibility as they come into and go out of view. This means that adding/removing marker components will happen frequently and there is a cost to this as it changes the archetype of the entity which I understand involves some allocations and copies of data. Notably this happens at the edges of the frustum and not throughout its volume which could mean it's a less bad problem than it may initially feel. Regardless, this could still be a win compared to iterating over all entities and having to check their computed visibility component's boolean member.

Particularly for 3D I feel like to understand which approach is better, we should design some 'heavy' tests that push at the boundaries of people could reasonably want to do with the engine that stress these different aspects and measure the two solutions to identify the best one. So I propose an example 3D test scene that:

• has a lot of entities distributed in space
  • lots of entities stresses the case with an inner boolean and having to iterate over all entities
  • distributed in space is intended to make it somewhat realistic with some entities coming into/going out of view as the camera is moved around. Perhaps they should be clustered a bit as props and things are commonly around walls/pillars when indoors, around trees, clusters of bushes, on the ground, etc when outdoors. That is, there is commonly a lot of air either in rooms or outside. :)
• flies the camera around to see sometimes all and sometimes subsets of the entities
  • moves things in and out of view

What do you think @cart ?

[mcobzarenco]

Just some thoughts, take them with a grain of salt, I'm a newcomer to bevy! I also may be missing context.

Semantics

Not having the Visible component does not seem semantically the same as having Visibility with is_visible = false. An entity that has the Visibility component means that it makes sense to talk about its visibility. E.g. how would a system that enables visibility on all hidden entities look like? Something like this

fn enable_visible(query: Query<&mut Visibility>) {
    for visiblity in query.iter_mut() {
        *visibility.is_visible = true;
    }
}

After this PR, querying for all entities and adding Visible component is not the correct thing to do. Adding a marker component CanBeVisible essentially reproduces a boolean.

Performance

Is the claim that

fn system(query: Query<&mut Transform, (With<Visible>,)>) {
  // do stuff
}

should always be faster than

fn system(query: Query<&mut Transform, Visibility>) {
  // check if `visibility.is_visible` is true and only then do stuff
}

Would the relative performance depend quite a bit on the specific scene? A question for those knowledgeable in the internals of the ECS, i.e. for entities that change visibility often, could the fact that visible / not-visible entities have different archetypes impact performance? Which doesn't happen when using a booleans.

What @superdump suggets re: benchmarking and considering ComputedVisibility at the same time makes a lot of sense.

Ergonomics

On the subject of "simpler querying", you may be right, but it is not immediately obvious to me this is a simpler. I.e. representing the 2 states as a piece of data (visible or not visible) vs. the absence or presence of a component make it hard to write code that manipulates visibility. E.g. how would a function that toggles the visibility of all components that can be visible look like:

fn blink(time: Res<Time>, query: Query<mut Visibility>) {
    // Toggle visiblity every second
    if time.seconds_since_startup().as_secs() % 2 == 1 {
        for visibility in query.iter_mut() {
            visibility.is_visible = !visibility.is_visible;
        }
    }
}

Inspector

Lastly, maybe not the best argument, but today with something like inspector-egui, it's nice to have a checkbox to enable / disable visibility. To reproduce this, it may need to be extended to allow adding components dynamically.

[ManevilleF]

I removed ComputedVisibility on this MR but I'm not yet sure of the gains.

My objective is to simplify, if Visible is a marker component the querying is faster because visibility can be used as a filter.
Mabe the solution lays on enum variant filtering but that's more of an ECS question.

If user visibility is toggled a lot (object pooling for example) the query gain might be counter balanced by the regular use of EntityCommands for component insertion/removal.
But if the user has scenes with hidden elements, like dunjeon rooms or assets that are not required to be visible then the gain is very clear because the rendering systems will never query this elements.

About visibility change, since you can then filter Without<Visible> the query to enable visibility on all hidden elements is also faster but you do require component insertion which is a drawback.

Edit: A small gain by the marker component concept is that systems that toggle visibility don't use mutable queries

[alice-i-cecile]

Once we have benchmarks for this, we should be testing how changing this component's storage type impacts performance.

[alice-i-cecile]

I'm in favor of this change. It is, in theory at least, the faster approach. And we have a lot of head-room to accelerate component insertion and removal, especially of unit structs.

More importantly though, this is the more idiomatic approach. Being able to filter at the query level makes this section of the code base easier to read and extend for users.

[cart]

There are some issues with marker components that we should discuss before embracing them for central components like Visible:

Marker components have a "visual editor" problem. With a standard "display current entity components" approach, marker components aren't discoverable. You need to know that a given entity supports them. With the current bool approach, Visibility would always show up on supported entities in the editor with a toggle-able is_visible widget.

Theres also the general "discoverability" problem: marker components that aren't part of the default bundle will have no direct correlation to the entities that support them. Ex: the NotShadowReceiver marker component suffers from this problem.

We should have an answer to these issues before we start embracing marker components throughout the engine / before we start using words like "idiomatic".

[ManevilleF]

Maybe bundles could provide an enum of the available marker components? This could be useful for the visual editor as well.
But then marker components should then be optional and Visible should disappear in favor of a Hidden component. Meaning that marker components are additional behaviours that can be toggled by the visual editor

[colepoirier]

@cart maybe something like slots for particular entities/bundles would be a solution to this? Like this entity supports Visible? Or is this a case that archetype invariants would solve so that you can specificy possible components of an entity @alice-i-cecile?

[alice-i-cecile]

@colepoirier see extended discussion on this point in #3833.

[colepoirier]

@colepoirier see extended discussion on this point in #3833.

Ah apologies @alice-i-cecile, I missed the migration of this discussion.

[alice-i-cecile]

@bevyengine/rendering-team, I'd like to unblock #2087 or (more likely) a replacement PR. Could I get another set of eyes on this to verify that it's good to merge, now that we have tentative blessing from Cart.

[superdump]

I still want to see benchmarks, at least for Visible, if not also ComputedVisible. Maybe some of us can do some and post results here? Maybe I’ll have a look at making some…

[superdump]

I've been testing using:

• [Merged by Bors] - many_cubes: Add a cube pattern suitable for benchmarking culling changes #4126

This PR at least doesn't seem to make any significant difference.

Unrelated to this PR I was trying out a few other ideas to try to optimise some bottlenecks for #4126 and I did try to see what the impact of using a ComputedVisible marker component would be. It tanked performance, but then I was clearing the ComputedVisible component every frame, and then adding it back. A more efficient way would be to iterate entities with and without it, and adjust them as appropriate (adding or removing the component). But, seeing that performance tanks for the case where all visible entities become not visible, and then become visible again (~11k visible cubes) makes me think using the marker approach for the computed visibility is not good. I wanted to try that anyway because one of the biggest bottlenecks is that we extract the material handle for every mesh whether it is visible or not, and the ExtractComponentPlugin accepts a filter parameter and I wanted to try it with With<ComputedVisible>. That clawed back a lot of performance, but it wasn't better. I suppose with only updating the components of the diff it could be a better choice in the usual case. I'll compare using ComputedVisible with delta component modification + handle extraction filtering with basically this PR with handle extraction filtering for materials using ComputedVisibility.is_visible.

[superdump]

Here are my test results on an M1 Max: https://gist.github.com/superdump/c103b45aa0a7b28a9519c4d20b193fc0

[superdump]

My conclusions:

• as expected, using a marker component for a bool state that changes regularly has performance hits vs other alternatives
• computed visibility should not use a marker component. it should use a bool, or the existing visible entities vecs
• if the visibility state of entities would be toggled frequently and en masse by users, this would incur a notable performance hit
• querying using With or Without is significantly faster than iterating all and testing a bool or iterating a Vec and getting the entity from a query
• the biggest performance gains come from optimising the bottlenecks (as in just doing the filtering regardless of how)
• the net results matter most

Recommendations:

• If we are concerned that users may want to toggle visibility en masse and frequently for some application/gameplay reason or so, then using adding/removing marker components on the order of thousands of entities or more incurs a noticeable performance hit.
• If we aren't concerned about that then using a marker component is fine.
• For ComputedVisibility we should either stick with what we have or remove it and leverage the VisibleEntities. I don't know if this has consequences for hierarchical propagation of visibility or not.

[alice-i-cecile]

If we are concerned that users may want to toggle visibility en masse and frequently for some application/gameplay reason or so

This is a real use-case: imagine you're attempting to toggle visibility of map layers in an RTS. That said, I'm not convinced it would be frequent, even if it is en-masse.

[hymm]

If we had propagated visibility, would it be less of an issue for many entities? or is there a use case that wouldn't have parent defined?

[superdump]

If we are concerned that users may want to toggle visibility en masse and frequently for some application/gameplay reason or so

This is a real use-case: imagine you're attempting to toggle visibility of map layers in an RTS. That said, I'm not convinced it would be frequent, even if it is en-masse.

There are render layers for that kind if thing I suppose. Then you’re flipping a bit in a mask on the camera and suddenly a whole slew of entities with render layers that intersect become computed visible. Good point!

[superdump]

If we had propagated visibility, would it be less of an issue for many entities? or is there a use case that wouldn't have parent defined?

I don’t understand enough about how hierarchies work now nor will work to comment. I saw that @james7132 has been thinking about hierarchies so perhaps they could comment?

[mcobzarenco]

Just for exhaustiveness of benchmark, would it be worth trying to add

#[component(storage = "SparseSet")]

on ComputedVisibility and rerunning the benchmarks? From what I understand, this should have worse iteration time (which is the common operation) , but better insert/remove speed.

[superdump]

Also, @mockersf suggested testing using sparse set storage for the COMPUTED (not vomited… autocorrect. Lol!) visibility component which supposedly has worse iteration performance but much better insertion/removal. I asked how the dense and sparseset storage works. Dense was said to be a vec with a hash map of entity to vec index. If using swap remove that sounds like constant time, but hashing has a cost.

[superdump]

Just for exhaustiveness of benchmark, would it be worth trying to add

#[component(storage = "SparseSet")]

on ComputedVisibility and rerunning the benchmarks? From what I understand, this should have worse iteration time (which is the common operation) , but better insert/remove speed.

Haha! See above. :) Yes, I’ll try it when I can.

[james7132]

If anything, that may make it worse, since you would need to dirty/remove the component from all of the children as well.

[mcobzarenco]

Haha! See above. :) Yes, I’ll try it when I can.

Haha @ concurrent comments

[mcobzarenco]

If anything, that may make it worse, since you would need to dirty/remove the component from all of the children as well.

Agree in theory, but given it's a one line change, I was curious to see the benchmarks, even if just for pedagogical reasons

[james7132]

Agree in theory, but given it's a one line change, I was curious to see the benchmarks, even if just for pedagogical reasons

To clarify, that was in response to the question about mixing this change with visibility inheritance. Not the sparse set storage test.

Using ComputedVisibility as a marker component with inheritance would, however, pretty strongly benefit from sparse set storage as adding/removing them en masse as a result of a propagated change would be sped up. Whether that is actually on par with just straight iteration over what is effectively a set of bools is something that needs to be benchmarked.

[superdump]

Use of SparseSet storage for ComputedVisible has been added to the gist. You may have to scroll the table as it's the last column. It is slower than default (Table?) storage for these usages.

[superdump]

@cart I think we decided against this in the end, right? If so, should we close it?