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.

Sign up for GitHub

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

Jump to bottom

Make sprites and text2d work with RenderLayers (add ComputedVisibility) #4007

Closed
wants to merge 4 commits into from
Closed

Make sprites and text2d work with RenderLayers (add ComputedVisibility) #4007

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8bceb1c
Quick and dirty ComputedVisiblity support for sprites, text2d
johanhelsing Feb 20, 2022
6377ee3
Use a HashSet to speed up visibility checking
johanhelsing Feb 21, 2022
1ddb82d
Remove resolved todo
johanhelsing Feb 22, 2022
759c292
Don't reallocate visible sprite entities map every frame
johanhelsing Feb 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions crates/bevy_sprite/src/bundle.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use crate::{
use bevy_asset::Handle;
use bevy_ecs::bundle::Bundle;
use bevy_render::{
prelude::ComputedVisibility,
texture::{Image, DEFAULT_IMAGE_HANDLE},
view::Visibility,
};
Expand All @@ -18,6 +19,7 @@ pub struct SpriteBundle {
pub texture: Handle<Image>,
/// User indication of whether an entity is visible
pub visibility: Visibility,
pub computed_visibility: ComputedVisibility,
}

impl Default for SpriteBundle {
Expand All @@ -28,6 +30,7 @@ impl Default for SpriteBundle {
global_transform: Default::default(),
texture: DEFAULT_IMAGE_HANDLE.typed(),
visibility: Default::default(),
computed_visibility: Default::default(),
}
}
}
Expand Down
32 changes: 24 additions & 8 deletions crates/bevy_sprite/src/render/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,12 @@ use bevy_render::{
render_resource::*,
renderer::{RenderDevice, RenderQueue},
texture::{BevyDefault, Image},
view::{Msaa, ViewUniform, ViewUniformOffset, ViewUniforms, Visibility},
view::{Msaa, ViewUniform, ViewUniformOffset, ViewUniforms, Visibility, VisibleEntities},
RenderWorld,
};
use bevy_transform::components::GlobalTransform;
use bevy_utils::FloatOrd;
use bevy_utils::HashMap;
use bevy_utils::{HashMap, HashSet};
use bytemuck::{Pod, Zeroable};
use copyless::VecHelper;

Expand Down Expand Up @@ -173,6 +173,7 @@ impl SpecializedRenderPipeline for SpritePipeline {

#[derive(Component, Clone, Copy)]
pub struct ExtractedSprite {
pub entity: Entity,
pub transform: GlobalTransform,
pub color: Color,
/// Select an area of the texture
Expand Down Expand Up @@ -224,8 +225,15 @@ pub fn extract_sprite_events(
pub fn extract_sprites(
mut render_world: ResMut<RenderWorld>,
texture_atlases: Res<Assets<TextureAtlas>>,
sprite_query: Query<(&Visibility, &Sprite, &GlobalTransform, &Handle<Image>)>,
sprite_query: Query<(
Entity,
&Visibility,
Copy link
Contributor

Choose a reason for hiding this comment

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

ComputedVisibility needs to be used for extraction.

&Sprite,
&GlobalTransform,
&Handle<Image>,
)>,
atlas_query: Query<(
Entity,
&Visibility,
Copy link
Contributor

Choose a reason for hiding this comment

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

ComputedVisibility needs to be used for extraction here too.

&TextureAtlasSprite,
&GlobalTransform,
Expand All @@ -234,12 +242,13 @@ pub fn extract_sprites(
) {
let mut extracted_sprites = render_world.resource_mut::<ExtractedSprites>();
extracted_sprites.sprites.clear();
for (visibility, sprite, transform, handle) in sprite_query.iter() {
for (entity, visibility, sprite, transform, handle) in sprite_query.iter() {
if !visibility.is_visible {
continue;
}
// PERF: we don't check in this function that the `Image` asset is ready, since it should be in most cases and hashing the handle is expensive
extracted_sprites.sprites.alloc().init(ExtractedSprite {
entity,
color: sprite.color,
transform: *transform,
// Use the full texture
Expand All @@ -252,13 +261,14 @@ pub fn extract_sprites(
anchor: sprite.anchor.as_vec(),
});
}
for (visibility, atlas_sprite, transform, texture_atlas_handle) in atlas_query.iter() {
for (entity, visibility, atlas_sprite, transform, texture_atlas_handle) in atlas_query.iter() {
if !visibility.is_visible {
continue;
}
if let Some(texture_atlas) = texture_atlases.get(texture_atlas_handle) {
let rect = Some(texture_atlas.textures[atlas_sprite.index as usize]);
extracted_sprites.sprites.alloc().init(ExtractedSprite {
entity,
color: atlas_sprite.color,
transform: *transform,
// Select the area in the texture atlas
Expand Down Expand Up @@ -347,8 +357,9 @@ pub fn queue_sprites(
gpu_images: Res<RenderAssets<Image>>,
msaa: Res<Msaa>,
mut extracted_sprites: ResMut<ExtractedSprites>,
mut views: Query<&mut RenderPhase<Transparent2d>>,
mut views: Query<(&VisibleEntities, &mut RenderPhase<Transparent2d>)>,
events: Res<SpriteAssetEvents>,
mut visible_entities_map: Local<HashSet<Entity>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn’t a map, it’s a set.

) {
// If an image has changed, the GpuImage has (probably) changed
for event in &events.images {
Expand Down Expand Up @@ -389,11 +400,13 @@ pub fn queue_sprites(
let mut index = 0;
let mut colored_index = 0;

// FIXME: VisibleEntities is ignored
for mut transparent_phase in views.iter_mut() {
for (visible_entities, mut transparent_phase) in views.iter_mut() {
let extracted_sprites = &mut extracted_sprites.sprites;
let image_bind_groups = &mut *image_bind_groups;

visible_entities_map.clear();
Copy link
Member

@james7132 james7132 Jun 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Consider using a FixedBitset instead of a HashMap here. A contains check using that should be faster than a HashSet, it doesn't involve a O(capacity) scan in the worst case and does not involve a hash on each lookup. Instead of storing the full entity, convert the u32 Entity ID into a usize index, and use that to set and check the FixedBitset.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that that suggestion should be an improvement to performance. I want to try to figure out if there is a way to restructure so that the set can be avoided.

@james7132 did you ever try using a fixed bit set for visible entities? I seem to remember that you did but I don’t remember.

Also, random thought that is not thought through and that may not be good but could be, is it worth storing a fixed bit set of views in which an entity is visible, per entity, instead of a fixed bit set of entities visible per-view? Maybe not as maybe the per-view set is small enough to stay in cache where the per-entity sets would be loaded in as part of iteration… I don’t know.

Copy link
Member

Choose a reason for hiding this comment

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

VisibleEntities IMO shouldn't, though I haven't tried. It'd require an &Entities on the other end to resolve the Entity from the ID, which may or may not be desirable depending on the downstream use case.

visible_entities_map.extend(visible_entities.iter().copied());

transparent_phase.items.reserve(extracted_sprites.len());

// Sort sprites by z for correct transparency and then by handle to improve batching
Expand Down Expand Up @@ -422,6 +435,9 @@ pub fn queue_sprites(
// Batches are merged later (in `batch_phase_system()`), so that they can be interrupted
// by any other phase item (and they can interrupt other items from batching).
for extracted_sprite in extracted_sprites.iter() {
if !visible_entities_map.contains(&extracted_sprite.entity) {
continue;
}
let new_batch = SpriteBatch {
image_handle_id: extracted_sprite.image_handle_id,
colored: extracted_sprite.color != Color::WHITE,
Expand Down
4 changes: 3 additions & 1 deletion crates/bevy_text/src/text2d.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use bevy_ecs::{
};
use bevy_math::{Vec2, Vec3};
use bevy_reflect::Reflect;
use bevy_render::{texture::Image, view::Visibility, RenderWorld};
use bevy_render::{prelude::ComputedVisibility, texture::Image, view::Visibility, RenderWorld};
use bevy_sprite::{Anchor, ExtractedSprite, ExtractedSprites, TextureAtlas};
use bevy_transform::prelude::{GlobalTransform, Transform};
use bevy_utils::HashSet;
Expand Down Expand Up @@ -58,6 +58,7 @@ pub struct Text2dBundle {
pub text_2d_size: Text2dSize,
pub text_2d_bounds: Text2dBounds,
pub visibility: Visibility,
pub computed_visibility: ComputedVisibility,
}

pub fn extract_text2d_sprite(
Expand Down Expand Up @@ -111,6 +112,7 @@ pub fn extract_text2d_sprite(
let transform = text_transform.mul_transform(glyph_transform);

extracted_sprites.sprites.push(ExtractedSprite {
entity,
transform,
color,
rect,
Expand Down