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

Replace the local text queues in the text systems with flags stored in a component #8549

Merged
merged 22 commits into from
May 8, 2023
Merged
Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
a8a820c
Use a single query instead of a `ParamSet` for `measure_text_system`
ickshonpe May 5, 2023
dfca89c
Replaced the `ParamSet` parameter with a single query in `text_system`.
ickshonpe May 5, 2023
4e1ade4
cargo fmt --all
ickshonpe May 5, 2023
9a424c7
The change detection predicate was checking for `!queued_text.contain…
ickshonpe May 5, 2023
b0620d0
removed left over debug variable
ickshonpe May 5, 2023
686e109
Renamed the `Local` parameter `queued_text` to `text_queue` in `measu…
ickshonpe May 5, 2023
baece54
fixed imports
ickshonpe May 5, 2023
d69d8ca
Renamed `new_queue` to `new_text_queue`.
ickshonpe May 5, 2023
6e93578
changes:
ickshonpe May 5, 2023
85eb848
* Added check to filter unchanged Text in `measure_text_system`.
ickshonpe May 5, 2023
7309bac
Remove the text queue from `text_system` and use the `TextFlags` comp…
ickshonpe May 5, 2023
de73323
* cargo fmt --all,
ickshonpe May 5, 2023
90427f2
Don't recompute text that needs a remeasure.
ickshonpe May 5, 2023
aca22ab
cargo fmy
ickshonpe May 5, 2023
253ec5d
fixed text flags for full remeasure
ickshonpe May 5, 2023
05eaf9d
Moved duplicate code from `measure_text_system` and `text_system` int…
ickshonpe May 5, 2023
396a59c
formatting
ickshonpe May 5, 2023
b0aa6d9
Added `TextFlags` doc comments explaining it is for internal use.
ickshonpe May 5, 2023
dc3e846
cargo fmt
ickshonpe May 5, 2023
9c2f299
Fixes for `too_many_arguments` and `needless_borrow` lints.
ickshonpe May 5, 2023
a4a3f65
Renamed the `remeasure` field of `TextFlags` to `generate_measure_func`
ickshonpe May 7, 2023
caf6c79
Renamings for `TextFlags` fields:
ickshonpe May 8, 2023
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
5 changes: 4 additions & 1 deletion crates/bevy_ui/src/node_bundles.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
//! This module contains basic node bundles used to build UIs

use crate::{
widget::{Button, UiImageSize},
widget::{Button, TextFlags, UiImageSize},
BackgroundColor, ContentSize, FocusPolicy, Interaction, Node, Style, UiImage, ZIndex,
};
use bevy_ecs::bundle::Bundle;
Expand Down Expand Up @@ -118,6 +118,8 @@ pub struct TextBundle {
pub text: Text,
/// Text layout information
pub text_layout_info: TextLayoutInfo,
/// Text system flags
pub text_flags: TextFlags,
/// The calculated size based on the given image
pub calculated_size: ContentSize,
/// Whether this node should block interaction with lower nodes
Expand Down Expand Up @@ -148,6 +150,7 @@ impl Default for TextBundle {
Self {
text: Default::default(),
text_layout_info: Default::default(),
text_flags: Default::default(),
calculated_size: Default::default(),
// Transparent background
background_color: BackgroundColor(Color::NONE),
Expand Down
251 changes: 161 additions & 90 deletions crates/bevy_ui/src/widget/text.rs
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
use crate::{ContentSize, Measure, Node, UiScale};
use bevy_asset::Assets;
use bevy_ecs::{
entity::Entity,
query::{Changed, Or, With},
system::{Local, ParamSet, Query, Res, ResMut},
prelude::{Component, DetectChanges},
query::With,
reflect::ReflectComponent,
system::{Local, Query, Res, ResMut},
world::{Mut, Ref},
};
use bevy_math::Vec2;
use bevy_reflect::{std_traits::ReflectDefault, Reflect};
use bevy_render::texture::Image;
use bevy_sprite::TextureAtlas;
use bevy_text::{
Expand All @@ -19,6 +22,27 @@ fn scale_value(value: f32, factor: f64) -> f32 {
(value as f64 * factor) as f32
}

/// Text system flags
alice-i-cecile marked this conversation as resolved.
Show resolved Hide resolved
///
/// Used internally by [`measure_text_system`] and [`text_system`] to schedule text for processing.
#[derive(Component, Debug, Clone, Reflect)]
#[reflect(Component, Default)]
pub struct TextFlags {
/// create a new measure for the text
remeasure: bool,
/// recompute the text
recompute: bool,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like remeasure should be called regenerate_measure_function or similar. "measuring" is when you call the measure function not when you generate it. recompute could then be called remeasure, but I think recompute is also fine.

Copy link
Contributor Author

@ickshonpe ickshonpe May 7, 2023

Choose a reason for hiding this comment

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

Yeah the terminology everywhere needs to be tightened up. I've been loose with it because I'm not really
clear on what's going to be intuitive for users etc, so just used short easy names. Even regenerate_measure_function is a bit misleading, since the flag is most commonly going to be used when a new text node entity is spawned but its fonts haven't loaded, so it is rescheduling the initial creation of a measure function until the next frame, not regenerating an existing measure function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed it to generate_measure_function, seems okay.

Copy link
Contributor Author

@ickshonpe ickshonpe May 8, 2023

Choose a reason for hiding this comment

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

Thought about this again, and I think these renamings are better as they capture the correct semantics:

  • generate_measure_func -> needs_new_measure_func
  • recompute -> needs_recompute


impl Default for TextFlags {
fn default() -> Self {
Self {
remeasure: true,
recompute: true,
}
}
}

#[derive(Clone)]
pub struct TextMeasure {
pub info: TextMeasureInfo,
Expand Down Expand Up @@ -54,20 +78,48 @@ impl Measure for TextMeasure {
}
}

#[inline]
fn create_text_measure(
fonts: &Assets<Font>,
text_pipeline: &mut TextPipeline,
scale_factor: f64,
text: Ref<Text>,
mut content_size: Mut<ContentSize>,
mut text_flags: Mut<TextFlags>,
) {
match text_pipeline.create_text_measure(
fonts,
&text.sections,
scale_factor,
text.alignment,
text.linebreak_behavior,
) {
Ok(measure) => {
content_size.set(TextMeasure { info: measure });

// Text measured, so set `TextFlags` to schedule a recompute
text_flags.remeasure = false;
text_flags.recompute = true;
}
Err(TextError::NoSuchFont) => {
// Try again next frame
text_flags.remeasure = true;
}
Err(e @ TextError::FailedToAddGlyph(_)) => {
panic!("Fatal error when processing text: {e}.");
}
};
}

/// Creates a `Measure` for text nodes that allows the UI to determine the appropriate amount of space
/// to provide for the text given the fonts, the text itself and the constraints of the layout.
pub fn measure_text_system(
mut queued_text: Local<Vec<Entity>>,
mut last_scale_factor: Local<f64>,
fonts: Res<Assets<Font>>,
windows: Query<&Window, With<PrimaryWindow>>,
ui_scale: Res<UiScale>,
mut text_pipeline: ResMut<TextPipeline>,
mut text_queries: ParamSet<(
Query<Entity, (Changed<Text>, With<Node>)>,
Query<Entity, (With<Text>, With<Node>)>,
Query<(&Text, &mut ContentSize)>,
)>,
mut text_query: Query<(Ref<Text>, &mut ContentSize, &mut TextFlags), With<Node>>,
) {
let window_scale_factor = windows
.get_single()
Expand All @@ -78,48 +130,85 @@ pub fn measure_text_system(

#[allow(clippy::float_cmp)]
if *last_scale_factor == scale_factor {
// Adds all entities where the text has changed to the local queue
for entity in text_queries.p0().iter() {
if !queued_text.contains(&entity) {
queued_text.push(entity);
// scale factor unchanged, only create new measures for modified text
for (text, content_size, text_flags) in text_query.iter_mut() {
if text.is_changed() || text_flags.remeasure {
create_text_measure(
&fonts,
&mut text_pipeline,
scale_factor,
text,
content_size,
text_flags,
);
}
}
} else {
// If the scale factor has changed, queue all text
for entity in text_queries.p1().iter() {
queued_text.push(entity);
}
// scale factor changed, remeasure all text
*last_scale_factor = scale_factor;
}

if queued_text.is_empty() {
return;
}

let mut new_queue = Vec::new();
let mut query = text_queries.p2();
for entity in queued_text.drain(..) {
if let Ok((text, mut content_size)) = query.get_mut(entity) {
match text_pipeline.create_text_measure(
for (text, content_size, text_flags) in text_query.iter_mut() {
create_text_measure(
&fonts,
&text.sections,
&mut text_pipeline,
scale_factor,
text.alignment,
text.linebreak_behavior,
) {
Ok(measure) => {
content_size.set(TextMeasure { info: measure });
}
Err(TextError::NoSuchFont) => {
new_queue.push(entity);
}
Err(e @ TextError::FailedToAddGlyph(_)) => {
panic!("Fatal error when processing text: {e}.");
}
};
text,
content_size,
text_flags,
);
}
}
}

#[allow(clippy::too_many_arguments)]
#[inline]
fn queue_text(
fonts: &Assets<Font>,
text_pipeline: &mut TextPipeline,
font_atlas_warning: &mut FontAtlasWarning,
font_atlas_set_storage: &mut Assets<FontAtlasSet>,
texture_atlases: &mut Assets<TextureAtlas>,
textures: &mut Assets<Image>,
text_settings: &TextSettings,
scale_factor: f64,
text: &Text,
node: Ref<Node>,
mut text_flags: Mut<TextFlags>,
mut text_layout_info: Mut<TextLayoutInfo>,
) {
if !text_flags.remeasure {
let node_size = Vec2::new(
scale_value(node.size().x, scale_factor),
scale_value(node.size().y, scale_factor),
);

match text_pipeline.queue_text(
fonts,
&text.sections,
scale_factor,
text.alignment,
text.linebreak_behavior,
node_size,
font_atlas_set_storage,
texture_atlases,
textures,
text_settings,
font_atlas_warning,
YAxisOrientation::TopToBottom,
) {
Err(TextError::NoSuchFont) => {
// There was an error processing the text layout, try again next frame
text_flags.recompute = true;
}
Err(e @ TextError::FailedToAddGlyph(_)) => {
panic!("Fatal error when processing text: {e}.");
}
Ok(info) => {
*text_layout_info = info;
text_flags.recompute = false;
}
}
}
*queued_text = new_queue;
}

/// Updates the layout and size information whenever the text or style is changed.
Expand All @@ -131,7 +220,6 @@ pub fn measure_text_system(
/// It does not modify or observe existing ones.
#[allow(clippy::too_many_arguments)]
pub fn text_system(
mut queued_text: Local<Vec<Entity>>,
mut textures: ResMut<Assets<Image>>,
mut last_scale_factor: Local<f64>,
fonts: Res<Assets<Font>>,
Expand All @@ -142,11 +230,7 @@ pub fn text_system(
mut texture_atlases: ResMut<Assets<TextureAtlas>>,
mut font_atlas_set_storage: ResMut<Assets<FontAtlasSet>>,
mut text_pipeline: ResMut<TextPipeline>,
mut text_queries: ParamSet<(
Query<Entity, Or<(Changed<Text>, Changed<Node>)>>,
Query<Entity, (With<Text>, With<Node>)>,
Query<(&Node, &Text, &mut TextLayoutInfo)>,
)>,
mut text_query: Query<(Ref<Node>, &Text, &mut TextLayoutInfo, &mut TextFlags)>,
) {
// TODO: Support window-independent scaling: https://github.com/bevyengine/bevy/issues/5621
let window_scale_factor = windows
Expand All @@ -156,58 +240,45 @@ pub fn text_system(

let scale_factor = ui_scale.scale * window_scale_factor;

#[allow(clippy::float_cmp)]
if *last_scale_factor == scale_factor {
// Adds all entities where the text or the style has changed to the local queue
for entity in text_queries.p0().iter() {
if !queued_text.contains(&entity) {
queued_text.push(entity);
// Scale factor unchanged, only recompute text for modified text nodes
for (node, text, text_layout_info, text_flags) in text_query.iter_mut() {
if node.is_changed() || text_flags.recompute {
queue_text(
&fonts,
&mut text_pipeline,
&mut font_atlas_warning,
&mut font_atlas_set_storage,
&mut texture_atlases,
&mut textures,
&text_settings,
scale_factor,
text,
node,
text_flags,
text_layout_info,
);
}
}
} else {
// If the scale factor has changed, queue all text
for entity in text_queries.p1().iter() {
queued_text.push(entity);
}
// Scale factor changed, recompute text for all text nodes
*last_scale_factor = scale_factor;
}

let mut new_queue = Vec::new();
let mut text_query = text_queries.p2();
for entity in queued_text.drain(..) {
if let Ok((node, text, mut text_layout_info)) = text_query.get_mut(entity) {
let node_size = Vec2::new(
scale_value(node.size().x, scale_factor),
scale_value(node.size().y, scale_factor),
);

match text_pipeline.queue_text(
for (node, text, text_layout_info, text_flags) in text_query.iter_mut() {
queue_text(
&fonts,
&text.sections,
scale_factor,
text.alignment,
text.linebreak_behavior,
node_size,
&mut text_pipeline,
&mut font_atlas_warning,
&mut font_atlas_set_storage,
&mut texture_atlases,
&mut textures,
text_settings.as_ref(),
&mut font_atlas_warning,
YAxisOrientation::TopToBottom,
) {
Err(TextError::NoSuchFont) => {
// There was an error processing the text layout, let's add this entity to the
// queue for further processing
new_queue.push(entity);
}
Err(e @ TextError::FailedToAddGlyph(_)) => {
panic!("Fatal error when processing text: {e}.");
}
Ok(info) => {
*text_layout_info = info;
}
}
&text_settings,
scale_factor,
text,
node,
text_flags,
text_layout_info,
);
}
}
*queued_text = new_queue;
}