Make each `TextSection` its own entity. #14572

Olle-Lukowski · 2024-08-01T15:36:50Z

Objective

This change makes each TextSection its own entity, which was discussed to be the next step for bevy_text in Discord.

Fixes #7714

Solution

Changed the Text component to hold no sections instead of a Vec of TextSection.
Made TextSection a component that should be added to children of a Text.
Adjustments to make the code compile again.

Testing

Tested out some of the examples that I ported to the new API, they all look fine.

Migration Guide

Any usage of TextBundle::from_section or Text::from_section should be replaced with something like this:

//old
commands.spawn(TextBundle::from_section("My Text", TextSection::default()));

// new
commands.spawn(TextBundle::default()).with_child(TextSection::from("My Text"));

Same goes for the from_sections methods.

alice-i-cecile · 2024-08-01T16:08:06Z

Alice 🌹: So @Preon, currently, text is handled via the Text component, which is composed of multiple TextSection parts in a vec
[10:07 AM] 
OP
 Alice 🌹: Both @DreamerTalin and @cart have independently found that this is very frustrating to work with in more complex workflows: you end up with two separate hierarchies that need to be managed
[10:07 AM]Preon: yeah that makes sense
[10:08 AM] 
OP
 Alice 🌹: Instead, each TextSection should be its own entity, with a consistent TextStyle
[10:09 AM] 
OP
 Alice 🌹: In 90% of cases, a chunk of text will have a single text style: we should optimize for making that case easy and ergonomic

Context for why this should be done, from Discord

alice-i-cecile · 2024-08-01T16:09:29Z

This work was previously mentioned in #14437. As discussed above, bringing text sections into the entity hierarchy is required to make them play nicely with scenes, and thus the BSN entity-spawning tools.

Olle-Lukowski · 2024-08-01T17:41:16Z

I think I will wait with porting over all examples for two reasons:

The API will probably change based on suggestions.
It is quite some work, that I don't want to re-do.

I did port some of the examples to show the new API, and those will be kept up to date for any changes that happen.

UkoeHB · 2024-08-05T17:53:11Z

crates/bevy_ui/src/widget/text.rs

+        let sections = if let Some(children) = children {
+            children_changed = children.is_changed();
+
+            text2d_section_query.iter_many(&children).collect()


This vec can be cached in a Local. Then use .clear() and .extend().

Ah right, will do that!

UkoeHB · 2024-08-05T17:54:18Z

crates/bevy_ui/src/widget/text.rs

+        let sections = if let Some(children) = children {
+            children_changed = children.is_changed();
+            text_section_query.iter_many(&children).collect()
+        } else {
+            vec![]
+        };


Same here, cache the vec.

UkoeHB · 2024-08-05T17:56:15Z

crates/bevy_text/src/text2d.rs

+            let section = text2d_section_query
+                .get(children.unwrap()[*section_index])
+                .unwrap();


It is not valid to access via [] here, because non-TextSection children may be interleaved. You need to find the position in the children that filters for entities with TextSection.

Hmm, I guess that's true. What would be the best approach for that?

UkoeHB · 2024-08-05T17:56:59Z

crates/bevy_text/src/text2d.rs

+        let sections = if let Some(children) = children {
+            children_changed = children.is_changed();
+            section_query.iter_many(children.iter()).collect()
+        } else {
+            vec![]
+        };


Here as well, cache the vec.

UkoeHB · 2024-08-05T17:58:05Z

crates/bevy_ui/src/render/mod.rs

+            let section = text2d_section_query
+                .get(children.unwrap()[*section_index])
+                .unwrap();


Invalid children access, need to filter for entities with TextSection.

UkoeHB · 2024-08-05T17:59:45Z

crates/bevy_ui/src/accessibility.rs

-            let values = text
-                .sections
+        if let Ok(maybe_children) = texts.get(*child) {
+            let mut sections = Vec::new();


Cache this vec if possible. Or convert to a chained iterator for the values collection.

UkoeHB · 2024-08-05T18:03:47Z

examples/2d/2d_shapes.rs

+        .spawn(TextBundle {
+            style: Style {
                position_type: PositionType::Absolute,
                top: Val::Px(12.0),
                left: Val::Px(12.0),
                ..default()
-            }),
-    );
+            },
+            ..default()
+        })
+        .with_child(TextSection::new(
+            "Press space to toggle wireframes",
+            TextStyle::default(),
+        ));


It might be nice if you could optionally put the first text section on the parent entity.

We initially had this supported, but in discord we decided to not do it.

Olle-Lukowski · 2024-08-21T16:20:17Z

Marking this as ready for review, the main thing that we still need to do is find a way to prevent repeated allocations in the systems where the text is enqueued.

ickshonpe · 2024-08-21T17:15:59Z

crates/bevy_text/src/text.rs

@@ -127,7 +52,7 @@ impl Text {
 }

 /// Contains the value of the text in a section and how it should be styled.
-#[derive(Debug, Default, Clone, Reflect)]
+#[derive(Component, Debug, Default, Clone, Reflect)]
 #[reflect(Default)]
 pub struct TextSection {


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

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.

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?

honestly great idea!

I know but I think it's really necessary.

Could you elaborate on this?

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.

@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?

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

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.

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.

TotalKrill

The docs should really indicate the new relationship between the components. And the component should be renamed since it has completely changed its meaning with this change.

And the newly created issue for this should indicate what was lost and point to this PR so it can be tracked

TotalKrill · 2024-08-21T17:02:16Z

crates/bevy_text/src/text.rs

@@ -127,7 +52,7 @@ impl Text {
 }

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


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

TotalKrill · 2024-08-21T17:03:45Z

crates/bevy_text/src/text.rs

@@ -127,7 +52,7 @@ impl Text {
 }

 /// Contains the value of the text in a section and how it should be styled.
-#[derive(Debug, Default, Clone, Reflect)]


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

TotalKrill · 2024-08-21T17:10:17Z

crates/bevy_ui/src/node_bundles.rs

@@ -207,26 +205,6 @@ pub struct TextBundle {

 #[cfg(feature = "bevy_text")]
 impl TextBundle {
-    /// Create a [`TextBundle`] from a single section.


As discussed in discord.

The removal of these ergonomics is a huge loss. both on TextBundle and on Text and should be addressed before 0.15 if this PR is to land

ickshonpe · 2024-08-21T17:29:44Z

crates/bevy_text/src/text.rs

@@ -29,8 +29,6 @@ impl Default for CosmicBuffer {
 #[derive(Component, Debug, Clone, Default, Reflect)]
 #[reflect(Component, Default)]
 pub struct Text {


Suggested change

pub struct Text {

pub struct TextNode {

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.

What do we think of TextLayout?

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

Olle-Lukowski · 2024-08-22T08:22:17Z

There are currently warnings about the children of a UI node not having UI components themselves, how do I fix these?

2024-08-22T08:10:22.518891Z  WARN bevy_ui::layout::ui_surface: Unstyled child `10v1` in a UI entity hierarchy. You are using an entity without UI components as a child of an entity with UI components, results may be unexpected.

ickshonpe · 2024-08-22T09:09:14Z

There are currently warnings about the children of a UI node not having UI components themselves, how do I fix these?
2024-08-22T08:10:22.518891Z  WARN bevy_ui::layout::ui_surface: Unstyled child `10v1` in a UI entity hierarchy. You are using an entity without UI components as a child of an entity with UI components, results may be unexpected.

Not tested it but changing these queries in ui_layout_system:

children_query: Query<(Entity, Ref<Children>), With<Node>>,
just_children_query: Query<&Children>,

to filter for the text bundle's marker component with:

children_query: Query<(Entity, Ref<Children>), (With<Node>, Without<TextLayout>)>,
just_children_query: Query<&Children, Without<TextLayout>>,

should be sufficient.

alice-i-cecile · 2024-08-22T12:11:16Z

Can you do a pass on the comments and resolve those that are addressed to make this easier to review? :)

tigregalis · 2024-08-22T13:37:05Z

Is this the right time to rename sections to spans?

alice-i-cecile · 2024-08-22T14:07:52Z

Is this the right time to rename sections to spans?

~~IMO yes.~~ Edit: I don't feel strongly, and getting this PR as small as possible would be nice.

…_))`

Text entity tidy

UkoeHB · 2024-09-02T23:33:24Z

See this discussion for a required-components-based redesign.

alice-i-cecile · 2024-10-09T18:41:07Z

Merging #15591, which continued this work.

Olle-Lukowski added 2 commits August 1, 2024 17:30

Initial API change (many broken examples)

72967ea

Fixed docs & flex dir in example

cfa3db0

alice-i-cecile added this to the 0.15 milestone Aug 1, 2024

alice-i-cecile added C-Code-Quality A section of code that is hard to understand or change C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Text Rendering and layout for characters labels Aug 1, 2024

alice-i-cecile added D-Complex Quite challenging from either a design or technical perspective. Ask for help! X-Blessed Has a large architectural impact or tradeoffs, but the design has been endorsed by decision makers labels Aug 1, 2024

alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged M-Needs-Release-Note Work that should be called out in the blog due to impact labels Aug 1, 2024

Updated some examples

bca4cf6

Olle-Lukowski and others added 7 commits August 2, 2024 12:18

Reworked API to make TextSection seperate from Text

388ab63

Doc fix

6482b45

Merge branch 'main' into text_entity

67d9737

Merge branch 'bevyengine:main' into text_entity

548fd5a

Got rid of special case for TextSection handling

abbaac1

Fixed 20 examples

e231822

Another 20 examples fixed

49ae77c

UkoeHB suggested changes Aug 5, 2024

View reviewed changes

Olle-Lukowski and others added 7 commits August 13, 2024 15:44

Merge branch 'main' into text_entity

7cda895

Fixed some more examples

345bc14

Merge branch 'main' into text_entity

dcc89cf

Finished up the examples

6a8bfbd

Fixed last example!

25d230e

Final example for real this time

57d4dc9

Merge branch 'main' into text_entity

ec5d3d6

Olle-Lukowski marked this pull request as ready for review August 21, 2024 16:20

Olle-Lukowski changed the title ~~WIP PR for making each TextSection its own entity.~~ Make each TextSection its own entity. Aug 21, 2024

Olle-Lukowski added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Aug 21, 2024

ickshonpe self-requested a review August 21, 2024 16:40

Olle-Lukowski mentioned this pull request Aug 21, 2024

Improve ergonomics for single-section Texts #14854

Closed

ickshonpe suggested changes Aug 21, 2024

View reviewed changes

TotalKrill suggested changes Aug 21, 2024

View reviewed changes

ickshonpe reviewed Aug 21, 2024

View reviewed changes

alice-i-cecile self-requested a review August 22, 2024 01:25

reduce allocations

e15f9c5

Filter out warnings from ui_layout

7b1a490

ickshonpe mentioned this pull request Aug 22, 2024

Split up TextSection into separate components #14875

Open

tigregalis added 4 commits August 22, 2024 23:55

terser text2d test

4a327a8

make calc_name use fewer allocations

d2948c5

use default() not TextStyle::default() in `TextSection::new(..., …

f8cd239

…_))`

use default() instead of cloning default TextStyle

0826981

tigregalis mentioned this pull request Aug 22, 2024

Text entity tidy Olle-Lukowski/bevy#1

Merged

Merge pull request #1 from tigregalis/text-entity-tidy

fb550c1

Text entity tidy

alice-i-cecile closed this Oct 9, 2024

		@@ -127,7 +52,7 @@ impl Text {
		}

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

Make each TextSection its own entity. #14572

Make each TextSection its own entity. #14572

Conversation

Olle-Lukowski commented Aug 1, 2024 • edited Loading

Objective

Solution

Testing

Migration Guide

alice-i-cecile commented Aug 1, 2024

alice-i-cecile commented Aug 1, 2024

Olle-Lukowski commented Aug 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Olle-Lukowski commented Aug 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ickshonpe Aug 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rparrett Aug 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TotalKrill left a comment • edited Loading

Choose a reason for hiding this comment

TotalKrill Aug 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Olle-Lukowski commented Aug 22, 2024

ickshonpe commented Aug 22, 2024 • edited Loading

alice-i-cecile commented Aug 22, 2024

tigregalis commented Aug 22, 2024

alice-i-cecile commented Aug 22, 2024 • edited Loading

UkoeHB commented Sep 2, 2024

alice-i-cecile commented Oct 9, 2024

Make each `TextSection` its own entity. #14572

Make each `TextSection` its own entity. #14572

Olle-Lukowski commented Aug 1, 2024 •

edited

Loading

Olle-Lukowski commented Aug 1, 2024 •

edited

Loading

ickshonpe Aug 21, 2024 •

edited

Loading

rparrett Aug 22, 2024 •

edited

Loading

TotalKrill left a comment •

edited

Loading

TotalKrill Aug 21, 2024 •

edited

Loading

ickshonpe commented Aug 22, 2024 •

edited

Loading

alice-i-cecile commented Aug 22, 2024 •

edited

Loading