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 each TextSection its own entity. #14572

Closed
wants to merge 24 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
72967ea
Initial API change (many broken examples)
Olle-Lukowski Aug 1, 2024
cfa3db0
Fixed docs & flex dir in example
Olle-Lukowski Aug 1, 2024
bca4cf6
Updated some examples
Olle-Lukowski Aug 1, 2024
388ab63
Reworked API to make `TextSection` seperate from `Text`
Olle-Lukowski Aug 2, 2024
6482b45
Doc fix
Olle-Lukowski Aug 2, 2024
67d9737
Merge branch 'main' into text_entity
Olle-Lukowski Aug 2, 2024
548fd5a
Merge branch 'bevyengine:main' into text_entity
Olle-Lukowski Aug 3, 2024
abbaac1
Got rid of special case for `TextSection` handling
Olle-Lukowski Aug 3, 2024
e231822
Fixed 20 examples
Olle-Lukowski Aug 3, 2024
49ae77c
Another 20 examples fixed
Olle-Lukowski Aug 3, 2024
7cda895
Merge branch 'main' into text_entity
Olle-Lukowski Aug 13, 2024
345bc14
Fixed some more examples
Olle-Lukowski Aug 21, 2024
dcc89cf
Merge branch 'main' into text_entity
Olle-Lukowski Aug 21, 2024
6a8bfbd
Finished up the examples
Olle-Lukowski Aug 21, 2024
25d230e
Fixed last example!
Olle-Lukowski Aug 21, 2024
57d4dc9
Final example for real this time
Olle-Lukowski Aug 21, 2024
ec5d3d6
Merge branch 'main' into text_entity
Olle-Lukowski Aug 21, 2024
e15f9c5
reduce allocations
Olle-Lukowski Aug 22, 2024
7b1a490
Filter out warnings from ui_layout
Olle-Lukowski Aug 22, 2024
4a327a8
terser text2d test
tigregalis Aug 22, 2024
d2948c5
make `calc_name` use fewer allocations
tigregalis Aug 22, 2024
f8cd239
use `default()` not `TextStyle::default()` in `TextSection::new(..., …
tigregalis Aug 22, 2024
0826981
use `default()` instead of cloning default TextStyle
tigregalis Aug 22, 2024
fb550c1
Merge pull request #1 from tigregalis/text-entity-tidy
Olle-Lukowski Aug 23, 2024
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
38 changes: 19 additions & 19 deletions crates/bevy_dev_tools/src/fps_overlay.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,8 @@ use bevy_ecs::{
system::{Commands, Query, Res, Resource},
};
use bevy_hierarchy::{BuildChildren, ChildBuild};
use bevy_text::{Font, Text, TextSection, TextStyle};
use bevy_ui::{
node_bundles::{NodeBundle, TextBundle},
PositionType, Style, ZIndex,
};
use bevy_text::{Font, TextSection, TextStyle};
use bevy_ui::{node_bundles::TextBundle, PositionType, Style, ZIndex};
use bevy_utils::default;

/// Global [`ZIndex`] used to render the fps overlay.
Expand Down Expand Up @@ -78,7 +75,7 @@ struct FpsText;

fn setup(mut commands: Commands, overlay_config: Res<FpsOverlayConfig>) {
commands
.spawn(NodeBundle {
.spawn((TextBundle {
style: Style {
// We need to make sure the overlay doesn't affect the position of other UI nodes
position_type: PositionType::Absolute,
Expand All @@ -87,35 +84,38 @@ fn setup(mut commands: Commands, overlay_config: Res<FpsOverlayConfig>) {
// Render overlay on top of everything
z_index: ZIndex::Global(FPS_OVERLAY_ZINDEX),
..default()
})
},))
.with_children(|c| {
c.spawn(TextSection::new(
"FPS: ",
overlay_config.text_config.clone(),
));

c.spawn((
TextBundle::from_sections([
TextSection::new("FPS: ", overlay_config.text_config.clone()),
TextSection::from_style(overlay_config.text_config.clone()),
]),
TextSection::from_style(overlay_config.text_config.clone()),
FpsText,
));
});
}

fn update_text(diagnostic: Res<DiagnosticsStore>, mut query: Query<&mut Text, With<FpsText>>) {
for mut text in &mut query {
fn update_text(
diagnostic: Res<DiagnosticsStore>,
mut query: Query<&mut TextSection, With<FpsText>>,
) {
for mut section in &mut query {
if let Some(fps) = diagnostic.get(&FrameTimeDiagnosticsPlugin::FPS) {
if let Some(value) = fps.smoothed() {
text.sections[1].value = format!("{value:.2}");
section.value = format!("{value:.2}");
}
}
}
}

fn customize_text(
overlay_config: Res<FpsOverlayConfig>,
mut query: Query<&mut Text, With<FpsText>>,
mut query: Query<&mut TextSection, With<FpsText>>,
) {
for mut text in &mut query {
for section in text.sections.iter_mut() {
section.style = overlay_config.text_config.clone();
}
for mut section in &mut query {
section.style = overlay_config.text_config.clone();
}
}
1 change: 1 addition & 0 deletions crates/bevy_text/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ bevy_asset = { path = "../bevy_asset", version = "0.15.0-dev" }
bevy_color = { path = "../bevy_color", version = "0.15.0-dev" }
bevy_derive = { path = "../bevy_derive", version = "0.15.0-dev" }
bevy_ecs = { path = "../bevy_ecs", version = "0.15.0-dev" }
bevy_hierarchy = { path = "../bevy_hierarchy", version = "0.15.0-dev" }
bevy_math = { path = "../bevy_math", version = "0.15.0-dev" }
bevy_reflect = { path = "../bevy_reflect", version = "0.15.0-dev", features = [
"bevy",
Expand Down
18 changes: 9 additions & 9 deletions crates/bevy_text/src/pipeline.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use std::sync::Arc;

use bevy_asset::{AssetId, Assets};
use bevy_ecs::{component::Component, reflect::ReflectComponent, system::Resource};
use bevy_ecs::{component::Component, reflect::ReflectComponent, system::Resource, world::Ref};
use bevy_math::{UVec2, Vec2};
use bevy_reflect::{std_traits::ReflectDefault, Reflect};
use bevy_render::texture::Image;
Expand Down Expand Up @@ -61,7 +61,7 @@ impl TextPipeline {
pub fn update_buffer(
&mut self,
fonts: &Assets<Font>,
sections: &[TextSection],
sections: &[(usize, Ref<TextSection>)],
linebreak_behavior: BreakLineOn,
bounds: TextBounds,
scale_factor: f64,
Expand All @@ -72,7 +72,7 @@ impl TextPipeline {

// return early if the fonts are not loaded yet
let mut font_size = 0.;
for section in sections {
for (_, section) in sections {
if section.style.font_size > font_size {
font_size = section.style.font_size;
}
Expand All @@ -85,7 +85,7 @@ impl TextPipeline {

// Load Bevy fonts into cosmic-text's font system.
// This is done as as separate pre-pass to avoid borrow checker issues
for section in sections.iter() {
for (_, section) in sections {
load_font_to_fontdb(section, font_system, &mut self.map_handle_to_font_id, fonts);
}

Expand All @@ -97,14 +97,13 @@ impl TextPipeline {
// in cosmic-text.
let spans: Vec<(&str, Attrs)> = sections
.iter()
.enumerate()
.filter(|(_section_index, section)| section.style.font_size > 0.0)
.map(|(section_index, section)| {
(
&section.value[..],
get_attrs(
section,
section_index,
*section_index,
font_system,
&self.map_handle_to_font_id,
scale_factor,
Expand Down Expand Up @@ -146,7 +145,7 @@ impl TextPipeline {
pub fn queue_text(
&mut self,
fonts: &Assets<Font>,
sections: &[TextSection],
sections: &[(usize, Ref<TextSection>)],
scale_factor: f64,
text_alignment: JustifyText,
linebreak_behavior: BreakLineOn,
Expand Down Expand Up @@ -185,7 +184,7 @@ impl TextPipeline {
.map(|(layout_glyph, line_y)| {
let section_index = layout_glyph.metadata;

let font_handle = sections[section_index].style.font.clone_weak();
let font_handle = sections[section_index].1.style.font.clone_weak();
let font_atlas_set = font_atlas_sets.sets.entry(font_handle.id()).or_default();

let physical_glyph = layout_glyph.physical((0., 0.), 1.);
Expand Down Expand Up @@ -238,10 +237,11 @@ impl TextPipeline {
///
/// Produces a [`TextMeasureInfo`] which can be used by a layout system
/// to measure the text area on demand.
#[allow(clippy::too_many_arguments)]
pub fn create_text_measure(
&mut self,
fonts: &Assets<Font>,
sections: &[TextSection],
sections: &[(usize, Ref<TextSection>)],
scale_factor: f64,
linebreak_behavior: BreakLineOn,
buffer: &mut CosmicBuffer,
Expand Down
77 changes: 1 addition & 76 deletions crates/bevy_text/src/text.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,6 @@ impl Default for CosmicBuffer {
#[derive(Component, Debug, Clone, Default, Reflect)]
#[reflect(Component, Default)]
pub struct Text {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
pub struct Text {
pub struct TextNode {

Copy link
Contributor

Choose a reason for hiding this comment

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

This suggestion seems confusing because the component is also used in Text2dBundle, and we don't use this naming convention in any other UI bundles.

Copy link
Contributor

Choose a reason for hiding this comment

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

What do we think of TextLayout?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll go with TextLayout, makes the most sense to me.

/// The text's sections
pub sections: Vec<TextSection>,
/// The text's internal alignment.
/// Should not affect its position within a container.
pub justify: JustifyText,
Expand All @@ -39,79 +37,6 @@ pub struct Text {
}

impl Text {
/// Constructs a [`Text`] with a single section.
///
/// ```
/// # use bevy_asset::Handle;
/// # use bevy_color::Color;
/// # use bevy_text::{Font, Text, TextStyle, JustifyText};
/// #
/// # let font_handle: Handle<Font> = Default::default();
/// #
/// // Basic usage.
/// let hello_world = Text::from_section(
/// // Accepts a String or any type that converts into a String, such as &str.
/// "hello world!",
/// TextStyle {
/// font: font_handle.clone().into(),
/// font_size: 60.0,
/// color: Color::WHITE,
/// },
/// );
///
/// let hello_bevy = Text::from_section(
/// "hello world\nand bevy!",
/// TextStyle {
/// font: font_handle.into(),
/// font_size: 60.0,
/// color: Color::WHITE,
/// },
/// ) // You can still add text justifaction.
/// .with_justify(JustifyText::Center);
/// ```
pub fn from_section(value: impl Into<String>, style: TextStyle) -> Self {
Self {
sections: vec![TextSection::new(value, style)],
..default()
}
}

/// Constructs a [`Text`] from a list of sections.
///
/// ```
/// # use bevy_asset::Handle;
/// # use bevy_color::Color;
/// # use bevy_color::palettes::basic::{RED, BLUE};
/// # use bevy_text::{Font, Text, TextStyle, TextSection};
/// #
/// # let font_handle: Handle<Font> = Default::default();
/// #
/// let hello_world = Text::from_sections([
/// TextSection::new(
/// "Hello, ",
/// TextStyle {
/// font: font_handle.clone().into(),
/// font_size: 60.0,
/// color: BLUE.into(),
/// },
/// ),
/// TextSection::new(
/// "World!",
/// TextStyle {
/// font: font_handle.into(),
/// font_size: 60.0,
/// color: RED.into(),
/// },
/// ),
/// ]);
/// ```
pub fn from_sections(sections: impl IntoIterator<Item = TextSection>) -> Self {
Self {
sections: sections.into_iter().collect(),
..default()
}
}

/// Returns this [`Text`] with a new [`JustifyText`].
pub const fn with_justify(mut self, justify: JustifyText) -> Self {
self.justify = justify;
Expand All @@ -127,7 +52,7 @@ impl Text {
}

/// Contains the value of the text in a section and how it should be styled.
Copy link
Contributor

@TotalKrill TotalKrill Aug 21, 2024

Choose a reason for hiding this comment

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

This is the first time we are adding a parent/child hierarchy requirement as far as I know, the docs should indicate this relationship on the related components.

Also with this change, the name "Text" is actually more confusing than helping, since it only contains the information for how text will be justified, and contains no text whatsoever

#[derive(Debug, Default, Clone, Reflect)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Same on this component, should probably indicate that this component is required to be a child of the other Component.

#[derive(Component, Debug, Default, Clone, Reflect)]
#[reflect(Default)]
pub struct TextSection {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be split up I think, TextStyle needs to be a separate elidable component imo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure how I feel about that, it would require a LOT more changes, that I don't want to update 100 examples for again.

Copy link
Contributor

@ickshonpe ickshonpe Aug 21, 2024

Choose a reason for hiding this comment

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

I know but I think it's really necessary.

Maybe if TextSection is made into a bundle with Text (and rename the existing component, I agree with TotalKrill that it's potentially confusing) and TextStyle components it would just work without any changes to most of the examples?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

honestly great idea!

Copy link
Contributor

Choose a reason for hiding this comment

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

I know but I think it's really necessary.

Could you elaborate on this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I know but I think it's really necessary.

Could you elaborate on this?

Different styling options, with Text2d style propagation and responsive sizes might not make sense or need different solutions. More granular change detection.

Copy link
Contributor

@rparrett rparrett Aug 22, 2024

Choose a reason for hiding this comment

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

@ickshonpe
That sounds good to me, but it also sounds like something that should be discussed/designed. Would be it acceptable to catalog this in an issue and implement it in a followup PR?

Copy link
Member

Choose a reason for hiding this comment

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

Totally agree on splitting this, strongly feel this should be moved to followup.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep I've no problem with moving this to a followup, it would be good if we can avoid multiple rewrites of the api and examples though.

Copy link
Contributor

Choose a reason for hiding this comment

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

It probably should be split up more even, previously it's really bugged me how changing the colour of a single section causes a relayout of the whole text node. Separate TextColor component disappears that problem.

/// The content (in `String` form) of the text in the section.
Expand Down
Loading