Make `Visibility` optional in most built-in systems

[djeedai]

What problem does this solve or what need does it fill?

Many built-in systems (e.g. extract_text2d_sprite() query a core component they want to update (e.g. Text) with an associated unconditional Visibility component. If removed, the system doesn't run on the Entity. However, most components (and especially the ones directly related to rendering) should be assumed visible by default. The ability to dynamically control the visibility of a render item should be an additional opt-in feature, not a default one.

What solution would you like?

Replace Visibility with Option<Visibility> in the query of most built-in systems, and treat None as Visibility::is_visible == true.

if !maybe_visibility.map_or(true, |vis| vis.is_visible) {
    continue; // skip this entity
}

What alternative(s) have you considered?

Do nothing. This forces adding many unused Visibility components for all entities that are never going to be hidden (they might be deleted, but not dynamically shown/hidden).

Additional context

The more "mandatory" components in a built-in system, the more difficult it is to reuse the built-in Bevy components. I'm trying to use Text to render some text, but with some properties (notably, the position, but also the visibility) controlled by my own framework. I could go rewrite from scratch a text rendering pipeline, but it feels like reusing the existing one is probably the best course of action. However currently the extract_text2d_sprite() system forces adding 3 extra components in addition of Text, which I argue is too much and at least Visibility should be optional (if not others; alignment could also be made optional for the cases where there's no need to align).

[afonsolage]

I like the idea to have fewer built-in mandatory components. If this goes on, we may end with tons of mandatory components in the future and when users add something, it expects to be visible, as default behavior.

But If that's the case, I think we should rename (again) the Visibility component and set for something like Invisible or Hidden, dunno, but I doesn't seems right to have something visible when Option<Visibility> is None.

[LoopyAshy]

I feel this change is a fair and smart change overall considering not everything needs skippable visibility. I do agree with afonsolage regarding renaming the component to Invisible, Invisibility, Hidden or NoRender or whatever term feels most natural while removing the field within it (simply making it a marker component) however I do wonder if that may have a negative effect on performance (if certain entities remove and add it often) so simply renaming the field to enabled is probably preferred.

[djeedai]

No, we should certainly not change the behavior of Visibility when present. I'm not at all proposing that Visibility becomes a boolean component that if present shows/hides; it's well known that adding/removing components all the time is very bad for performance.

What I'm proposing is that:

• if the user expects to dynamically toggle the visibility of a render item, then the Visibility component is added to serve that purpose, as it currently is used
• if on the other hand the user knows for a fact it won't ever be trying to hide a render item until it's despawned, then there's no point having a Visibility component with is_visible stuck forever to true; instead we can remove the component altogether and assume a default visibility (and because it's a render item, it's visible by default, because the entire point of a render item is to be rendered)

There's a major difference between items whose visibility is dynamically and periodically/often toggled at runtime (for example, the check mark of a checkbox widget in UI) and items which are visible from the moment they're spawned to the moment they're despawned. And using visibility for hiding items for longer periods (like, hide the main menu while playing the game) is not desirable, because you often want to recover the resources used by those items not in use (especially, the GPU resources). In that latter case, Visibility is useless; it will remain with is_visible == true for the entire lifetime of the entity.

[djeedai]

I'd like to get the opinion of rendering folks on this. @james7132 or @superdump maybe when you have a minute? (thanks!) I can start a PR change as an example, but I'd like to make sure there's agreement in principle before I start any work, to avoid wasted time :)

[ManevilleF]

Relevant Mr about marker components: #3796

[djeedai]

For clarity, this is the code I'm using in my own system where I query maybe_visibility: &Option<Visibility> instead of just Visibility:

if !maybe_visibility.map_or(true, |vis| vis.is_visible) {
    continue; // skip this entity
}

[djeedai]

Adding another argument. I just discovered in the dev docs that UiCameraConfig exactly follows this pattern: it's optional and visibility is assumed by default if not present.

[superdump]

I think @cart decided to go in a different direction when we merged the recent visibility inheritance changes, added visibility to all rendered bundles, and added SpatialBundle which includes Transform, GlobalTransform, Visibility, and ComputedVisibility. The direction there is that visibility is mandatory for things that need to be rendered, and a core part of the user-facing functionality around position, orientation, scale, visibility.

[djeedai]

What was the rationale? If this was already acted as the solution, then this issue should probably be closed as "Won't Fix". And that direction conflicts with the UiCameraConfig then, which should probably be reviewed.