Implicit text roots #21496
Implicit text roots #21496
Conversation
…ut` relationship for all text entities
* Added query filters to queries in `ui_layout_system`, so that non-root text nodes are ignored by layout, otherwise text composed from multiple sections isn't rendered.
| #[reflect(Component, Default, Debug, PartialEq, Clone)] | ||
| #[require(Node, TextLayout, TextFont, TextColor, TextNodeFlags, ContentSize)] | ||
| #[require( | ||
| Node, |
There was a problem hiding this comment.
This changeset feels weird to me because Text is a Node, which logically takes up space in the layout, has a style, etc. However in the "Text serving the role of TextSpan" case , we really don't want it to behave like one, as that would produce confusing results. It also creates a new class of internal (or external) bug where we forget to exclude "textspan-like Text" when considering a collection of Nodes. Likewise, Text2d behaves logically like a "sprite". Making it not behave like one when nested feels very odd to me, and is also a new potential source of bugs.
There was a problem hiding this comment.
Unfortunately, the PR description doesn't include the (rather lengthy) justification for this change.
There was a problem hiding this comment.
Yeah I just published this last night after I got it working and then went to bed, this does need more explanation. I'll write something up.
| mut targets_query: Query<&mut TextTarget>, | ||
| children_query: Query<&Children, With<TextSection>>, | ||
| ) { | ||
| for (output_entity, mut entities, layout) in entities_query.iter_mut() { |
There was a problem hiding this comment.
Doing this "scan, build list, and compare" every frame for every text tree in the UI feels overly expensive to me. I don't believe we were paying this price in the previous system.
There was a problem hiding this comment.
Yeah it's deliberately simple, walking and collecting the entities is easy to understand and test. I'm not sure heavily optimising everything is worth it atm until all the new text features are added.
Needs more profiling, but even so I think overall this branch is slightly more efficient than main. A problem with the old design is that it can't tell in isolation whether a TextSpan entity is part of a Text or Text2d hierarchy, so it processes them twice.
There was a problem hiding this comment.
I think there are probably ways to improve the TextSpan implementation too though, I just don't feel it's worth the effort because I dislike the API so much.
alice-i-cecile
added
A-UI
|
It looks like your PR has been selected for a highlight in the next release blog post, but you didn't provide a release note. Please review the instructions for writing release notes, then expand or revise the content in the release notes directory to showcase your changes. |
4 participants
Objective
Retire the
TextSpancomponent, useTextandText2dentities for both root and non-root text entities.Solution
Identify text roots by checking their parent, if a text entity's parent is a non-text entity, it's a root.
Text layouts are now stored on a separate entity that is automatically spawned and associated with the root via a new relation
TextRoot.Testing
All existing examples (I hope) should work be working without any changes except renaming
TextSpantoTextorText2ddepending on context.