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

Text2d visibility fix #9708

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions crates/bevy_sprite/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ impl Plugin for SpritePlugin {
.init_resource::<SpecializedRenderPipelines<SpritePipeline>>()
.init_resource::<SpriteMeta>()
.init_resource::<ExtractedSprites>()
.init_resource::<ExtractedSpriteBatches>()
.init_resource::<SpriteAssetEvents>()
.add_render_command::<Transparent2d, DrawSprite>()
.add_systems(
Expand Down
63 changes: 61 additions & 2 deletions crates/bevy_sprite/src/render/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -333,6 +333,27 @@ pub struct ExtractedSprites {
pub sprites: SparseSet<Entity, ExtractedSprite>,
}

/// Allows extraction of multiply sprites for a single entity that all share that entity's id for visibility resolution.
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
/// Allows extraction of multiply sprites for a single entity that all share that entity's id for visibility resolution.
/// Allows the extraction of multiple sprites sharing the same entity id for visibility resolution.

I think there's a typo here (multiply -> multiple). I'm also leaving a suggestion to rephrase the comment, hopefully it still conveys the same meaning.

/// Not designed for efficiency, extracting sprite entities individually should be more performant.
#[derive(Resource, Default)]
pub struct ExtractedSpriteBatches {
ickshonpe marked this conversation as resolved.
Show resolved Hide resolved
/// maps the entity id for each batch to a range of indices into `sprite_ids`
pub batches: SparseSet<Entity, Range<usize>>,
/// set of all the extracted sprites from every batch
pub sprites: SparseSet<Entity, ExtractedSprite>,
/// ids of empty entities used to identify the individual sprites in each batch
pub sprite_ids: Vec<Entity>,
}

impl ExtractedSpriteBatches {
/// Clear all batches
pub fn clear(&mut self) {
self.batches.clear();
self.sprites.clear();
self.sprite_ids.clear();
}
}

#[derive(Resource, Default)]
pub struct SpriteAssetEvents {
pub images: Vec<AssetEvent<Image>>,
Expand Down Expand Up @@ -505,6 +526,7 @@ pub fn queue_sprites(
pipeline_cache: Res<PipelineCache>,
msaa: Res<Msaa>,
extracted_sprites: Res<ExtractedSprites>,
extracted_batches: Res<ExtractedSpriteBatches>,
mut views: Query<(
&mut RenderPhase<Transparent2d>,
&VisibleEntities,
Expand Down Expand Up @@ -559,7 +581,7 @@ pub fn queue_sprites(

transparent_phase
.items
.reserve(extracted_sprites.sprites.len());
.reserve(extracted_sprites.sprites.len() + extracted_batches.sprites.len());

for (entity, extracted_sprite) in extracted_sprites.sprites.iter() {
if !view_entities.contains(entity.index() as usize) {
Expand Down Expand Up @@ -590,6 +612,38 @@ pub fn queue_sprites(
});
}
}

for (batch_entity, sprite_entities) in extracted_batches.batches.iter() {
if !view_entities.contains(batch_entity.index() as usize) {
Copy link
Member

Choose a reason for hiding this comment

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

This could use a bit of a comment: it's really quite unusual to care about the index of an Entity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original code wasn't commented either, I'm confident these changes work and don't mess anything up but I don't know enough about the rendering internals to be explaining anything to anyone I think.

continue;
}
for sprite_entity in &extracted_batches.sprite_ids[sprite_entities.clone()] {
let extracted_sprite = extracted_batches.sprites.get(*sprite_entity).unwrap();
// These items will be sorted by depth with other phase items
let sort_key = FloatOrd(extracted_sprite.transform.translation().z);

// Add the item to the render phase
if extracted_sprite.color != Color::WHITE {
transparent_phase.add(Transparent2d {
draw_function: draw_sprite_function,
pipeline: colored_pipeline,
entity: *sprite_entity,
sort_key,
// batch_size will be calculated in prepare_glyphs
batch_size: 0,
});
} else {
transparent_phase.add(Transparent2d {
draw_function: draw_sprite_function,
pipeline,
entity: *sprite_entity,
sort_key,
// batch_size will be calculated in prepare_glyphs
batch_size: 0,
});
}
}
}
}
}

Expand All @@ -605,6 +659,7 @@ pub fn prepare_sprites(
mut image_bind_groups: ResMut<ImageBindGroups>,
gpu_images: Res<RenderAssets<Image>>,
extracted_sprites: Res<ExtractedSprites>,
extracted_batches: Res<ExtractedSpriteBatches>,
mut phases: Query<&mut RenderPhase<Transparent2d>>,
events: Res<SpriteAssetEvents>,
) {
Expand Down Expand Up @@ -648,7 +703,11 @@ pub fn prepare_sprites(
// Compatible items share the same entity.
for item_index in 0..transparent_phase.items.len() {
let item = &transparent_phase.items[item_index];
let Some(extracted_sprite) = extracted_sprites.sprites.get(item.entity) else {
let Some(extracted_sprite) = extracted_sprites
.sprites
.get(item.entity)
.or_else(|| extracted_batches.sprites.get(item.entity))
else {
// If there is a phase item that is not a sprite, then we must start a new
// batch to draw the other phase item(s) and to respect draw order. This can be
// done by invalidating the batch_image_handle
Expand Down
43 changes: 25 additions & 18 deletions crates/bevy_text/src/text2d.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ use bevy_render::{
view::{InheritedVisibility, ViewVisibility, Visibility},
Extract,
};
use bevy_sprite::{Anchor, ExtractedSprite, ExtractedSprites, TextureAtlas};
use bevy_sprite::{Anchor, ExtractedSprite, ExtractedSpriteBatches, TextureAtlas};
use bevy_transform::prelude::{GlobalTransform, Transform};
use bevy_utils::HashSet;
use bevy_window::{PrimaryWindow, Window, WindowScaleFactorChanged};
Expand Down Expand Up @@ -80,11 +80,12 @@ pub struct Text2dBundle {

pub fn extract_text2d_sprite(
mut commands: Commands,
mut extracted_sprites: ResMut<ExtractedSprites>,
mut extracted_batches: ResMut<ExtractedSpriteBatches>,
texture_atlases: Extract<Res<Assets<TextureAtlas>>>,
windows: Extract<Query<&Window, With<PrimaryWindow>>>,
text2d_query: Extract<
Query<(
Entity,
&ViewVisibility,
&Text,
&TextLayoutInfo,
Expand All @@ -93,25 +94,28 @@ pub fn extract_text2d_sprite(
)>,
>,
) {
extracted_batches.clear();
// TODO: Support window-independent scaling: https://github.com/bevyengine/bevy/issues/5621
let scale_factor = windows
.get_single()
.map(|window| window.resolution.scale_factor() as f32)
.unwrap_or(1.0);
let scaling = GlobalTransform::from_scale(Vec3::splat(scale_factor.recip()));

for (view_visibility, text, text_layout_info, anchor, global_transform) in text2d_query.iter() {
for (entity, view_visibility, text, text_layout_info, anchor, global_transform) in
text2d_query.iter()
{
if !view_visibility.get() {
continue;
}

let start = extracted_batches.sprite_ids.len();
let text_anchor = -(anchor.as_vec() + 0.5);
let alignment_translation = text_layout_info.size * text_anchor;
let transform = *global_transform
* scaling
* GlobalTransform::from_translation(alignment_translation.extend(0.));
let mut color = Color::WHITE;
let mut current_section = usize::MAX;
let mut color = Color::WHITE;
for PositionedGlyph {
position,
atlas_info,
Expand All @@ -123,22 +127,25 @@ pub fn extract_text2d_sprite(
color = text.sections[*section_index].style.color.as_rgba_linear();
current_section = *section_index;
}

let atlas = texture_atlases.get(&atlas_info.texture_atlas).unwrap();

extracted_sprites.sprites.insert(
commands.spawn_empty().id(),
ExtractedSprite {
transform: transform * GlobalTransform::from_translation(position.extend(0.)),
color,
rect: Some(atlas.textures[atlas_info.glyph_index]),
custom_size: None,
image_handle_id: atlas.texture.id(),
flip_x: false,
flip_y: false,
anchor: Anchor::Center.as_vec(),
},
);
let sprite_id = commands.spawn_empty().id();
let sprite = ExtractedSprite {
transform: transform * GlobalTransform::from_translation(position.extend(0.)),
color,
rect: Some(atlas.textures[atlas_info.glyph_index]),
custom_size: None,
image_handle_id: atlas.texture.id(),
flip_x: false,
flip_y: false,
anchor: Anchor::Center.as_vec(),
};
extracted_batches.sprite_ids.push(sprite_id);
extracted_batches.sprites.insert(sprite_id, sprite);
}
let indices = start..extracted_batches.sprite_ids.len();
extracted_batches.batches.insert(entity, indices);
}
}

Expand Down