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

Make sprites respect RenderLayers #5114

Closed
wants to merge 4 commits into from
Closed

Conversation

kserz
Copy link

@kserz kserz commented Jun 27, 2022

Objective

Make sprites respect RenderLayers. Alternative to #4007.

Solution

  • Add ComputedVisibility to SpriteBundle, SpriteSheetBundle, Text2dBundle and sprites extract stage. Potentially useful for dropping sprites by AABB checks.
  • At sprites queue stage use RenderLayers directly.
  • As a side fix: sorting of extracted sprites is done once for all the views as it is not changed in between.

Compared to #4007:

  • Pros: no performance penalty on many_sprites.
  • Cons: some duplicate logic between visibility computation and sprites.

@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen C-Performance A change motivated by improving speed, memory usage or compile times C-Usability A simple quality-of-life change that makes Bevy easier to use labels Jun 27, 2022
@@ -58,21 +62,31 @@ pub struct Text2dBundle {
pub text_2d_size: Text2dSize,
pub text_2d_bounds: Text2dBounds,
pub visibility: Visibility,
pub coputed_visibility: Visibility,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pub coputed_visibility: Visibility,
pub computed_visibility: Visibility,

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work, thanks for the contribution. Frustum culling is not enabled in 2D still, correct?

I think this is a relatively important fix, and I like the code changes.

@alice-i-cecile
Copy link
Member

Without any performance losses, I don't think this is a controversial change. @johanhelsing, could I get your review over here?

@superdump
Copy link
Contributor

On the one hand, this adds support for render layers in an inexpensive way and avoids VisibleEntities. On the other hand, ComputedVisibility here is global visibility, not per-view. If there were only one view, which is perhaps the most common, particularly for 2D(?), it could be sufficient. I don’t remember off the top of my head if simply adding ComputedVisibility and an Aabb is sufficient for culling. If it is then this would allow opt-in culling. I think we should just do 2d- and 3d-specific culling code as 2d is so much simpler than 3d.

@alice-i-cecile alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Jun 27, 2022
@alice-i-cecile
Copy link
Member

On the other hand, ComputedVisibility here is global visibility, not per-view

I don't think this is sufficient. This will break things like split screen right?

I don’t remember off the top of my head if simply adding ComputedVisibility and an Aabb is sufficient for culling.

This would need to be an oriented bounding box, but basically yes.

I think we should just do 2d- and 3d-specific culling code as 2d is so much simpler than 3d.

I agree; I think that the duplication is worth the performance gains and will allow us to iterate on 2D transform types more easily.

@kserz
Copy link
Author

kserz commented Jun 27, 2022

As far as I can tell, AABB's are not computed for sprites currently, so culling is likely not working.

I'm not sure about addition of ComputedVisibility as well. On the one hand, it is not a requirement for RenderLayers to work. On the other hand, all the sprites not present in any view will be dropped earlier. Currently by layers only in case of dynamic layers switching, I'm not sure whether it's useful. With the addition of AABB's that would be more beneficial to reduce the amount of extracted sprites and sorting time.

I can remove ComputedVisibility from sprites for now and leave it until there is a clear benefit. Any opinions?

@kserz
Copy link
Author

kserz commented Jun 27, 2022

On the other hand, ComputedVisibility here is global visibility, not per-view

I don't think this is sufficient. This will break things like split screen right?

Probably I'm not understanding this. Split screen is actually my goal for this change: support for RenderLayers should allow rendering different sets of sprites for different players(cameras). For meshes (3d & 2d) it is already working through ComputedVisibibilty. For sprites it will work by accounting layers directly, and ComputedVisibility will be used as a kind of optimization.

@alice-i-cecile
Copy link
Member

Sorry, I was unclear. I was concerned that storing a single ComputedVisibility list would prevent us from performing frustum culling using it in cases where more than one viewport exists.

I'm not sure about addition of ComputedVisibility as well. On the one hand, it is not a requirement for RenderLayers to work. On the other hand, all the sprites not present in any view will be dropped earlier

Sounds like we could split out adding ComputedVisibility into a separate PR with its own motivation. Could we see what this change set looks like without the ComputedVisibility changes in a new commit?

@superdump
Copy link
Contributor

On the other hand, ComputedVisibility here is global visibility, not per-view

I don't think this is sufficient. This will break things like split screen right?

It won’t break it, but all extracted sprites would be drawn for all 2d cameras I think? So it wouldn’t perform as well.

I don’t remember off the top of my head if simply adding ComputedVisibility and an Aabb is sufficient for culling.

This would need to be an oriented bounding box, but basically yes.

The orientation is applied from the Transform component which the sprite has. Adding an Aabb component is then sufficient for the culling code to pick it up and do frustum culling.

@superdump
Copy link
Contributor

I need some more time to think about what the right choice is here but I think minimally just adding RenderLayer support by extracting it and using it when drawing stuff for each view should be ok.

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm very happy with this as a simple incremental improvement. Adding to the milestone; this flagship feature should work in 2D on release.

@alice-i-cecile alice-i-cecile added this to the Bevy 0.8 milestone Jun 28, 2022
@alice-i-cecile alice-i-cecile removed the X-Controversial There is active debate or serious implications around merging this PR label Jun 28, 2022
Copy link
Member

@james7132 james7132 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No glaring issues seen here, though ideally we shouldn't extract any sprites that aren't visible.

@james7132 james7132 added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jun 30, 2022
@alice-i-cecile
Copy link
Member

@superdump I'm waiting on you to merge this.

@cart
Copy link
Member

cart commented Jul 12, 2022

Before merging, I'd like to compare this to the fully unified visibility inheritance / ComputedVisibility everywhere impl im working on over in #5146. (see my rationale there).

@cart
Copy link
Member

cart commented Jul 12, 2022

We should definitely merge one of these impls for 0.8 though. Which one matters way less than including one of them.

@alice-i-cecile alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Jul 13, 2022
bors bot pushed a commit that referenced this pull request Jul 15, 2022
…support (#5310)

# Objective

Fixes #4907. Fixes #838. Fixes #5089.
Supersedes #5146. Supersedes #2087. Supersedes #865. Supersedes #5114

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.

Additionally, the semantics of `Visibility` vs `ComputedVisibility` are inconsistent across entity types. 3D meshes use `ComputedVisibility` as the "definitive" visibility component, with `Visibility` being just one data source. Sprites just use `Visibility`, which means they can't feed off of `ComputedVisibility` data, such as culling information, RenderLayers, and (added in this pr) visibility inheritance information.

## Solution

Splits `ComputedVisibilty::is_visible` into `ComputedVisibilty::is_visible_in_view` and `ComputedVisibilty::is_visible_in_hierarchy`. For each visible entity, `is_visible_in_hierarchy` is computed by propagating visibility down the hierarchy. The `ComputedVisibility::is_visible()` function combines these two booleans for the canonical "is this entity visible" function.

Additionally, all entities that have `Visibility` now also have `ComputedVisibility`.  Sprites, Lights, and UI entities now use `ComputedVisibility` when appropriate.

This means that in addition to visibility inheritance, everything using Visibility now also supports RenderLayers. Notably, Sprites (and other 2d objects) now support `RenderLayers` and work properly across multiple views.

Also note that this does increase the amount of work done per sprite. Bevymark with 100,000 sprites on `main` runs in `0.017612` seconds and this runs in `0.01902`. That is certainly a gap, but I believe the api consistency and extra functionality this buys us is worth it. See [this thread](#5146 (comment)) for more info. Note that #5146 in combination with #5114 _are_ a viable alternative to this PR and _would_ perform better, but that comes at the cost of api inconsistencies and doing visibility calculations in the "wrong" place. The current visibility system does have potential for performance improvements. I would prefer to evolve that one system as a whole rather than doing custom hacks / different behaviors for each feature slice.

Here is a "split screen" example where the left camera uses RenderLayers to filter out the blue sprite.

![image](https://user-images.githubusercontent.com/2694663/178814868-2e9a2173-bf8c-4c79-8815-633899d492c3.png)


Note that this builds directly on #5146 and that @james7132 deserves the credit for the baseline visibility inheritance work. This pr moves the inherited visibility field into `ComputedVisibility`, then does the additional work of porting everything to `ComputedVisibility`. See my [comments here](#5146 (comment)) for rationale. 

## Follow up work

* Now that lights use ComputedVisibility, VisibleEntities now includes "visible lights" in the entity list. Functionally not a problem as we use queries to filter the list down in the desired context. But we should consider splitting this out into a separate`VisibleLights` collection for both clarity and performance reasons. And _maybe_ even consider scoping `VisibleEntities` down to `VisibleMeshes`?.
* Investigate alternative sprite rendering impls (in combination with visibility system tweaks) that avoid re-generating a per-view fixedbitset of visible entities every frame, then checking each ExtractedEntity. This is where most of the performance overhead lives. Ex: we could generate ExtractedEntities per-view using the VisibleEntities list, avoiding the need for the bitset.
* Should ComputedVisibility use bitflags under the hood? This would cut down on the size of the component, potentially speed up the `is_visible()` function, and allow us to cheaply expand ComputedVisibility with more data (ex: split out local visibility and parent visibility, add more culling classes, etc).
---

## Changelog

* ComputedVisibility now takes hierarchy visibility into account.
* 2D, UI and Light entities now use the ComputedVisibility component.

## Migration Guide

If you were previously reading `Visibility::is_visible` as the "actual visibility" for sprites or lights, use `ComputedVisibilty::is_visible()` instead:

```rust
// before (0.7)
fn system(query: Query<&Visibility>) {
  for visibility in query.iter() {
    if visibility.is_visible {
       log!("found visible entity");
    }
  }
}

// after (0.8)
fn system(query: Query<&ComputedVisibility>) {
  for visibility in query.iter() {
    if visibility.is_visible() {
       log!("found visible entity");
    }
  }
}
``` 


Co-authored-by: Carter Anderson <mcanders1@gmail.com>
@james7132
Copy link
Member

Superseded by #5310.

@james7132 james7132 closed this Jul 15, 2022
inodentry pushed a commit to IyesGames/bevy that referenced this pull request Aug 8, 2022
…support (bevyengine#5310)

# Objective

Fixes bevyengine#4907. Fixes bevyengine#838. Fixes bevyengine#5089.
Supersedes bevyengine#5146. Supersedes bevyengine#2087. Supersedes bevyengine#865. Supersedes bevyengine#5114

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.

Additionally, the semantics of `Visibility` vs `ComputedVisibility` are inconsistent across entity types. 3D meshes use `ComputedVisibility` as the "definitive" visibility component, with `Visibility` being just one data source. Sprites just use `Visibility`, which means they can't feed off of `ComputedVisibility` data, such as culling information, RenderLayers, and (added in this pr) visibility inheritance information.

## Solution

Splits `ComputedVisibilty::is_visible` into `ComputedVisibilty::is_visible_in_view` and `ComputedVisibilty::is_visible_in_hierarchy`. For each visible entity, `is_visible_in_hierarchy` is computed by propagating visibility down the hierarchy. The `ComputedVisibility::is_visible()` function combines these two booleans for the canonical "is this entity visible" function.

Additionally, all entities that have `Visibility` now also have `ComputedVisibility`.  Sprites, Lights, and UI entities now use `ComputedVisibility` when appropriate.

This means that in addition to visibility inheritance, everything using Visibility now also supports RenderLayers. Notably, Sprites (and other 2d objects) now support `RenderLayers` and work properly across multiple views.

Also note that this does increase the amount of work done per sprite. Bevymark with 100,000 sprites on `main` runs in `0.017612` seconds and this runs in `0.01902`. That is certainly a gap, but I believe the api consistency and extra functionality this buys us is worth it. See [this thread](bevyengine#5146 (comment)) for more info. Note that bevyengine#5146 in combination with bevyengine#5114 _are_ a viable alternative to this PR and _would_ perform better, but that comes at the cost of api inconsistencies and doing visibility calculations in the "wrong" place. The current visibility system does have potential for performance improvements. I would prefer to evolve that one system as a whole rather than doing custom hacks / different behaviors for each feature slice.

Here is a "split screen" example where the left camera uses RenderLayers to filter out the blue sprite.

![image](https://user-images.githubusercontent.com/2694663/178814868-2e9a2173-bf8c-4c79-8815-633899d492c3.png)


Note that this builds directly on bevyengine#5146 and that @james7132 deserves the credit for the baseline visibility inheritance work. This pr moves the inherited visibility field into `ComputedVisibility`, then does the additional work of porting everything to `ComputedVisibility`. See my [comments here](bevyengine#5146 (comment)) for rationale. 

## Follow up work

* Now that lights use ComputedVisibility, VisibleEntities now includes "visible lights" in the entity list. Functionally not a problem as we use queries to filter the list down in the desired context. But we should consider splitting this out into a separate`VisibleLights` collection for both clarity and performance reasons. And _maybe_ even consider scoping `VisibleEntities` down to `VisibleMeshes`?.
* Investigate alternative sprite rendering impls (in combination with visibility system tweaks) that avoid re-generating a per-view fixedbitset of visible entities every frame, then checking each ExtractedEntity. This is where most of the performance overhead lives. Ex: we could generate ExtractedEntities per-view using the VisibleEntities list, avoiding the need for the bitset.
* Should ComputedVisibility use bitflags under the hood? This would cut down on the size of the component, potentially speed up the `is_visible()` function, and allow us to cheaply expand ComputedVisibility with more data (ex: split out local visibility and parent visibility, add more culling classes, etc).
---

## Changelog

* ComputedVisibility now takes hierarchy visibility into account.
* 2D, UI and Light entities now use the ComputedVisibility component.

## Migration Guide

If you were previously reading `Visibility::is_visible` as the "actual visibility" for sprites or lights, use `ComputedVisibilty::is_visible()` instead:

```rust
// before (0.7)
fn system(query: Query<&Visibility>) {
  for visibility in query.iter() {
    if visibility.is_visible {
       log!("found visible entity");
    }
  }
}

// after (0.8)
fn system(query: Query<&ComputedVisibility>) {
  for visibility in query.iter() {
    if visibility.is_visible() {
       log!("found visible entity");
    }
  }
}
``` 


Co-authored-by: Carter Anderson <mcanders1@gmail.com>
james7132 added a commit to james7132/bevy that referenced this pull request Oct 28, 2022
…support (bevyengine#5310)

# Objective

Fixes bevyengine#4907. Fixes bevyengine#838. Fixes bevyengine#5089.
Supersedes bevyengine#5146. Supersedes bevyengine#2087. Supersedes bevyengine#865. Supersedes bevyengine#5114

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.

Additionally, the semantics of `Visibility` vs `ComputedVisibility` are inconsistent across entity types. 3D meshes use `ComputedVisibility` as the "definitive" visibility component, with `Visibility` being just one data source. Sprites just use `Visibility`, which means they can't feed off of `ComputedVisibility` data, such as culling information, RenderLayers, and (added in this pr) visibility inheritance information.

## Solution

Splits `ComputedVisibilty::is_visible` into `ComputedVisibilty::is_visible_in_view` and `ComputedVisibilty::is_visible_in_hierarchy`. For each visible entity, `is_visible_in_hierarchy` is computed by propagating visibility down the hierarchy. The `ComputedVisibility::is_visible()` function combines these two booleans for the canonical "is this entity visible" function.

Additionally, all entities that have `Visibility` now also have `ComputedVisibility`.  Sprites, Lights, and UI entities now use `ComputedVisibility` when appropriate.

This means that in addition to visibility inheritance, everything using Visibility now also supports RenderLayers. Notably, Sprites (and other 2d objects) now support `RenderLayers` and work properly across multiple views.

Also note that this does increase the amount of work done per sprite. Bevymark with 100,000 sprites on `main` runs in `0.017612` seconds and this runs in `0.01902`. That is certainly a gap, but I believe the api consistency and extra functionality this buys us is worth it. See [this thread](bevyengine#5146 (comment)) for more info. Note that bevyengine#5146 in combination with bevyengine#5114 _are_ a viable alternative to this PR and _would_ perform better, but that comes at the cost of api inconsistencies and doing visibility calculations in the "wrong" place. The current visibility system does have potential for performance improvements. I would prefer to evolve that one system as a whole rather than doing custom hacks / different behaviors for each feature slice.

Here is a "split screen" example where the left camera uses RenderLayers to filter out the blue sprite.

![image](https://user-images.githubusercontent.com/2694663/178814868-2e9a2173-bf8c-4c79-8815-633899d492c3.png)


Note that this builds directly on bevyengine#5146 and that @james7132 deserves the credit for the baseline visibility inheritance work. This pr moves the inherited visibility field into `ComputedVisibility`, then does the additional work of porting everything to `ComputedVisibility`. See my [comments here](bevyengine#5146 (comment)) for rationale. 

## Follow up work

* Now that lights use ComputedVisibility, VisibleEntities now includes "visible lights" in the entity list. Functionally not a problem as we use queries to filter the list down in the desired context. But we should consider splitting this out into a separate`VisibleLights` collection for both clarity and performance reasons. And _maybe_ even consider scoping `VisibleEntities` down to `VisibleMeshes`?.
* Investigate alternative sprite rendering impls (in combination with visibility system tweaks) that avoid re-generating a per-view fixedbitset of visible entities every frame, then checking each ExtractedEntity. This is where most of the performance overhead lives. Ex: we could generate ExtractedEntities per-view using the VisibleEntities list, avoiding the need for the bitset.
* Should ComputedVisibility use bitflags under the hood? This would cut down on the size of the component, potentially speed up the `is_visible()` function, and allow us to cheaply expand ComputedVisibility with more data (ex: split out local visibility and parent visibility, add more culling classes, etc).
---

## Changelog

* ComputedVisibility now takes hierarchy visibility into account.
* 2D, UI and Light entities now use the ComputedVisibility component.

## Migration Guide

If you were previously reading `Visibility::is_visible` as the "actual visibility" for sprites or lights, use `ComputedVisibilty::is_visible()` instead:

```rust
// before (0.7)
fn system(query: Query<&Visibility>) {
  for visibility in query.iter() {
    if visibility.is_visible {
       log!("found visible entity");
    }
  }
}

// after (0.8)
fn system(query: Query<&ComputedVisibility>) {
  for visibility in query.iter() {
    if visibility.is_visible() {
       log!("found visible entity");
    }
  }
}
``` 


Co-authored-by: Carter Anderson <mcanders1@gmail.com>
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
…support (bevyengine#5310)

# Objective

Fixes bevyengine#4907. Fixes bevyengine#838. Fixes bevyengine#5089.
Supersedes bevyengine#5146. Supersedes bevyengine#2087. Supersedes bevyengine#865. Supersedes bevyengine#5114

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.

Additionally, the semantics of `Visibility` vs `ComputedVisibility` are inconsistent across entity types. 3D meshes use `ComputedVisibility` as the "definitive" visibility component, with `Visibility` being just one data source. Sprites just use `Visibility`, which means they can't feed off of `ComputedVisibility` data, such as culling information, RenderLayers, and (added in this pr) visibility inheritance information.

## Solution

Splits `ComputedVisibilty::is_visible` into `ComputedVisibilty::is_visible_in_view` and `ComputedVisibilty::is_visible_in_hierarchy`. For each visible entity, `is_visible_in_hierarchy` is computed by propagating visibility down the hierarchy. The `ComputedVisibility::is_visible()` function combines these two booleans for the canonical "is this entity visible" function.

Additionally, all entities that have `Visibility` now also have `ComputedVisibility`.  Sprites, Lights, and UI entities now use `ComputedVisibility` when appropriate.

This means that in addition to visibility inheritance, everything using Visibility now also supports RenderLayers. Notably, Sprites (and other 2d objects) now support `RenderLayers` and work properly across multiple views.

Also note that this does increase the amount of work done per sprite. Bevymark with 100,000 sprites on `main` runs in `0.017612` seconds and this runs in `0.01902`. That is certainly a gap, but I believe the api consistency and extra functionality this buys us is worth it. See [this thread](bevyengine#5146 (comment)) for more info. Note that bevyengine#5146 in combination with bevyengine#5114 _are_ a viable alternative to this PR and _would_ perform better, but that comes at the cost of api inconsistencies and doing visibility calculations in the "wrong" place. The current visibility system does have potential for performance improvements. I would prefer to evolve that one system as a whole rather than doing custom hacks / different behaviors for each feature slice.

Here is a "split screen" example where the left camera uses RenderLayers to filter out the blue sprite.

![image](https://user-images.githubusercontent.com/2694663/178814868-2e9a2173-bf8c-4c79-8815-633899d492c3.png)


Note that this builds directly on bevyengine#5146 and that @james7132 deserves the credit for the baseline visibility inheritance work. This pr moves the inherited visibility field into `ComputedVisibility`, then does the additional work of porting everything to `ComputedVisibility`. See my [comments here](bevyengine#5146 (comment)) for rationale. 

## Follow up work

* Now that lights use ComputedVisibility, VisibleEntities now includes "visible lights" in the entity list. Functionally not a problem as we use queries to filter the list down in the desired context. But we should consider splitting this out into a separate`VisibleLights` collection for both clarity and performance reasons. And _maybe_ even consider scoping `VisibleEntities` down to `VisibleMeshes`?.
* Investigate alternative sprite rendering impls (in combination with visibility system tweaks) that avoid re-generating a per-view fixedbitset of visible entities every frame, then checking each ExtractedEntity. This is where most of the performance overhead lives. Ex: we could generate ExtractedEntities per-view using the VisibleEntities list, avoiding the need for the bitset.
* Should ComputedVisibility use bitflags under the hood? This would cut down on the size of the component, potentially speed up the `is_visible()` function, and allow us to cheaply expand ComputedVisibility with more data (ex: split out local visibility and parent visibility, add more culling classes, etc).
---

## Changelog

* ComputedVisibility now takes hierarchy visibility into account.
* 2D, UI and Light entities now use the ComputedVisibility component.

## Migration Guide

If you were previously reading `Visibility::is_visible` as the "actual visibility" for sprites or lights, use `ComputedVisibilty::is_visible()` instead:

```rust
// before (0.7)
fn system(query: Query<&Visibility>) {
  for visibility in query.iter() {
    if visibility.is_visible {
       log!("found visible entity");
    }
  }
}

// after (0.8)
fn system(query: Query<&ComputedVisibility>) {
  for visibility in query.iter() {
    if visibility.is_visible() {
       log!("found visible entity");
    }
  }
}
``` 


Co-authored-by: Carter Anderson <mcanders1@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Performance A change motivated by improving speed, memory usage or compile times C-Usability A simple quality-of-life change that makes Bevy easier to use S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants