Conversation
|
Note that my original conception of ghost nodes is that they would work in all rendering contexts, not just UI, however this is somewhat controversial. |
b391283 to
99b9678
Compare
| /// The text's sections | ||
| pub sections: Vec<TextSection>, | ||
| #[require(ComputedTextBlock, TextLayoutInfo)] | ||
| pub struct TextBlock { |
There was a problem hiding this comment.
The name TextBlock doesn't seem very intuitive, the name implies to me it would be the primary text component holding the string, not the arrangment settings?
There was a problem hiding this comment.
It's not great but this is what we have in the rework discussion so I will leave it as-is unless something better is proposed.
There was a problem hiding this comment.
This is the defining component for a "text block", which is a UI entity that "receives and lays out text". ComputedTextBlock is where that text is received / that is what holds the final string. Given that there are multiple top-level text types which actually the hold string, we can't really call any of those TextBlock, as we need a central unopinionated reusable component, and the top-level text types are all opinionated.
Open to alternative names, but I don't see TextBlock as a particularly bad name for "the UI component that indicates that an enitty recieves text to be laid out".
There was a problem hiding this comment.
TextBlock is pretty awkward in practice, from my experience migrating examples. TextLayout would make a lot more sense. You don't actually add TextBlock to an entity and get a text block... you get a partial text block that needs a root text component and text style to function.
There was a problem hiding this comment.
you get a partial text block that needs a root text component and text style to function.
Not super important, but when I pitched this design I intended for TextBlock to be the "source of truth text span receiver component" that served its function independently of specific "root text" components. Of course I didn't explicitly call that out and I think the current impl is totally fine. But if we went that route, it would make TextBlock make sense (and would also probably make building future TextBlock types easier). Plenty of open questions there though in terms of implementation / I don't think we need to go down that road in this PR.
crates/bevy_text/src/text.rs
Outdated
| /// How the text should linebreak when running out of the bounds determined by `max_size`. | ||
| pub linebreak: LineBreak, | ||
| /// The antialiasing method to use when rendering text. | ||
| pub font_smoothing: FontSmoothing, |
There was a problem hiding this comment.
I think don't think the font smoothing setting belongs here, it doesn't affect the arrangement of the text. It would be better stored in an elidable component of its own or in TextStyle.
There was a problem hiding this comment.
Moved to TextStyle since smoothing is applied per-font.
| /// the font face, the font size, and the color. | ||
| #[derive(Component, Clone, Debug, Reflect)] | ||
| #[reflect(Component, Default, Debug)] | ||
| pub struct TextStyle { |
There was a problem hiding this comment.
The color should be in a separate component TextColor, so that changes to the color don't trigger change detection and a relayout of the text.
There was a problem hiding this comment.
Maybe TextStyle should be renamed as well to TextFont or something. It might be more consistant maybe with the UI Style component if TextBlock took over the name TextStyle.
There was a problem hiding this comment.
Yeah this makes sense to me. I'm cool with TextFont and TextColor.
There was a problem hiding this comment.
This is fine with me but can we do it in a separate PR? This one is already quite huge. The migration from TextStyle to TextFont/TextColor will by itself be a big chore and I call 'not it'.
olukowski
left a comment
There was a problem hiding this comment.
Other than the fact that I don't think TextNEW is a good name (I don't think this was intended even), this LGTM!
crates/bevy_ui/src/widget/text.rs
Outdated
| Visibility, // TODO: Remove when Node uses required components. | ||
| Transform // TODO: Remove when Node uses required components. | ||
| )] | ||
| pub struct TextNEW(pub String); |
There was a problem hiding this comment.
I really dislike how the first span is a different type. I'd prefer if it just required the first text span to be a child too.
Could we do something like put the require on a marker component and have a constructor that returns a tuple (Marker, TextSpan)?
There was a problem hiding this comment.
The original proposal had a universal span component but cart wanted to consolidate: #15014 (comment)
There was a problem hiding this comment.
I really dislike how the first span is a different type. I'd prefer if it just required the first text span to be a child too.
Text from my perspective exists to be the "standalone single span of text". A huge percentage of text cases receive no benefit from the added flexibility of TextBlock->TextSpan nesting. People won't want to pay that cost unless they have to.
The suggested "constructor that returns two components" pattern feels really nasty to me, especially in the context of the scene system. People don't expect constructors to behave that way, and I'd prefer it if we didn't embrace that pattern in Bevy, especially for something as fundamental as text. The init dataflow should be as simple as spawning the components you want explicitly.
If we aren't already doing it, we could make text.0 for the default empty string behave as "no span", so the first child of Text::default() -> TextSpan is at index 0.
It wasn't a part of my examples, but we could implement support for (Node, TextBlock) [ TextSpan ] as an option (and/or a new TextBlockNode type (which could require Node)) . Functionally TextBlock was intended to be the "arbitrary TextSpan collector" that worked across contexts. My thought when writing the proposal was that it would all be backed by the same fundamental system, and that system could be applied anywhere TextBlock and TextSpan are nested. That isn't how the impl ended up (which is fine given that I didn't explicitly express that need). I don't think changing to make that possible is necessary, as Text::default() -> TextSpan feels totally fine to me logically. But we can discuss it!
There was a problem hiding this comment.
If we aren't already doing it, we could make text.0 for the default empty string behave as "no span", so the first child of Text::default() -> TextSpan is at index 0.
I disagree with this, it creates ambiguity and conceptual branching. Note that it's common to set a default-empty span and then later write text to it.
There was a problem hiding this comment.
I disagree with this, it creates ambiguity and conceptual branching. Note that it's common to set a default-empty span and then later write text to it.
I don't have strong opinions on that. Just pitching an alternative path that allows Text::default()->TextSpan to behave as if the first TextSpan child is actually the first span. I do agree that it makes the "empty Text to non-empty" change harder to reason about, as it would behave differently than an empty child TextSpan.
crates/bevy_ui/src/widget/text.rs
Outdated
| Visibility, // TODO: Remove when Node uses required components. | ||
| Transform // TODO: Remove when Node uses required components. | ||
| )] | ||
| pub struct TextNEW(pub String); |
There was a problem hiding this comment.
Another thing would be to call it TextRootSpan. it's uglier but less confusing.
There was a problem hiding this comment.
See my comment here: https://github.com/bevyengine/bevy/pull/15591/files#r1786443025. Giving this a "bad name" would fundamentally conflict with the goal of making single-span text easy and ergonomic to define.
crates/bevy_ui/src/widget/text.rs
Outdated
| */ | ||
| #[derive(Component, Debug, Default, Clone, Deref, DerefMut, Reflect)] | ||
| #[reflect(Component, Default, Debug)] | ||
| #[require(TextStyle, GhostNode, Visibility(hidden_visibility))] |
There was a problem hiding this comment.
I'd prefer if TextStyle were elidable and if missing it just uses the TextStyle of its parent or the root text span.
There was a problem hiding this comment.
I like this idea but want to see what @cart has to say. There's also some added complexity for editing text spans to consider.
There was a problem hiding this comment.
I think its worth considering adding inheritance, but I think we should spec that out separately / give it its own PR rather than hold this one up with more design work. There are some implications to consider, such as using enums (ex: TextStyle::Inherit) vs component presence.
| /// The text's sections | ||
| pub sections: Vec<TextSection>, | ||
| #[require(ComputedTextBlock, TextLayoutInfo)] | ||
| pub struct TextBlock { |
There was a problem hiding this comment.
This is the defining component for a "text block", which is a UI entity that "receives and lays out text". ComputedTextBlock is where that text is received / that is what holds the final string. Given that there are multiple top-level text types which actually the hold string, we can't really call any of those TextBlock, as we need a central unopinionated reusable component, and the top-level text types are all opinionated.
Open to alternative names, but I don't see TextBlock as a particularly bad name for "the UI component that indicates that an enitty recieves text to be laid out".
| /// the font face, the font size, and the color. | ||
| #[derive(Component, Clone, Debug, Reflect)] | ||
| #[reflect(Component, Default, Debug)] | ||
| pub struct TextStyle { |
There was a problem hiding this comment.
Yeah this makes sense to me. I'm cool with TextFont and TextColor.
crates/bevy_ui/src/widget/text.rs
Outdated
| Visibility, // TODO: Remove when Node uses required components. | ||
| Transform // TODO: Remove when Node uses required components. | ||
| )] | ||
| pub struct TextNEW(pub String); |
There was a problem hiding this comment.
See my comment here: https://github.com/bevyengine/bevy/pull/15591/files#r1786443025. Giving this a "bad name" would fundamentally conflict with the goal of making single-span text easy and ergonomic to define.
crates/bevy_ui/src/widget/text.rs
Outdated
| Visibility, // TODO: Remove when Node uses required components. | ||
| Transform // TODO: Remove when Node uses required components. | ||
| )] | ||
| pub struct TextNEW(pub String); |
There was a problem hiding this comment.
I really dislike how the first span is a different type. I'd prefer if it just required the first text span to be a child too.
Text from my perspective exists to be the "standalone single span of text". A huge percentage of text cases receive no benefit from the added flexibility of TextBlock->TextSpan nesting. People won't want to pay that cost unless they have to.
The suggested "constructor that returns two components" pattern feels really nasty to me, especially in the context of the scene system. People don't expect constructors to behave that way, and I'd prefer it if we didn't embrace that pattern in Bevy, especially for something as fundamental as text. The init dataflow should be as simple as spawning the components you want explicitly.
If we aren't already doing it, we could make text.0 for the default empty string behave as "no span", so the first child of Text::default() -> TextSpan is at index 0.
It wasn't a part of my examples, but we could implement support for (Node, TextBlock) [ TextSpan ] as an option (and/or a new TextBlockNode type (which could require Node)) . Functionally TextBlock was intended to be the "arbitrary TextSpan collector" that worked across contexts. My thought when writing the proposal was that it would all be backed by the same fundamental system, and that system could be applied anywhere TextBlock and TextSpan are nested. That isn't how the impl ended up (which is fine given that I didn't explicitly express that need). I don't think changing to make that possible is necessary, as Text::default() -> TextSpan feels totally fine to me logically. But we can discuss it!
crates/bevy_ui/src/widget/text.rs
Outdated
| */ | ||
| #[derive(Component, Debug, Default, Clone, Deref, DerefMut, Reflect)] | ||
| #[reflect(Component, Default, Debug)] | ||
| #[require(TextStyle, GhostNode, Visibility(hidden_visibility))] |
There was a problem hiding this comment.
I think its worth considering adding inheritance, but I think we should spec that out separately / give it its own PR rather than hold this one up with more design work. There are some implications to consider, such as using enums (ex: TextStyle::Inherit) vs component presence.
crates/bevy_ui/src/widget/text.rs
Outdated
| */ | ||
| #[derive(Component, Debug, Default, Clone, Deref, DerefMut, Reflect)] | ||
| #[reflect(Component, Default, Debug)] | ||
| #[require(TextStyle, GhostNode, Visibility(hidden_visibility))] |
There was a problem hiding this comment.
Can we discuss why GhostNode is here? My theory is that this is to support scenarios like Text -> TextSpan -> Node -> TextSpan.
- It feels like
TextSpanshould always be a "leaf" from a UI perspective, other than having other child TextSpans. I can't think of a scenario where arbitrary hierarchy like this makes sense. GhostNodeis still a bit contentious / we're considering it experimental. I'd prefer it if we didn't take a dependency on it for "real things" yet.- By removing the GhostNode requirement, we can go back to having a single TextSpan type that works across contexts, which seems much nicer to me.
There was a problem hiding this comment.
Can we discuss why GhostNode is here?
We need text spans to not participate in layout. The alternative is Display::None but then you start lugging around useless Node and Style components. GhostNode is currently the most well-suited to the task.
By removing the GhostNode requirement, we can go back to having a single TextSpan type that works across contexts, which seems much nicer to me.
Not without some serious design analysis, since we need a good way to skip entities during layout. See my comment on the GhostNode PR.
I can't think of a scenario where arbitrary hierarchy like this makes sense.
In the distant future we may have 'in-line UI' in text blocks. But there is no design around that now (it may not be in-tree, or may use different relations) so I agree text spans can be considered simple leafs.
There was a problem hiding this comment.
We need text spans to not participate in layout.
Why wouldn't you want them to participate in UI layout? The main text node defines the available / desired space for text, which communicates sizes both upwards and downwards the tree.
There was a problem hiding this comment.
Why wouldn't you want them to participate in UI layout? The main text node defines the available / desired space for text, which communicates sizes both upwards and downwards the tree.
I lied a little, they do participate in layout, but not directly. Spans are collected into a cosmic-text buffer which knows how to manage text, and then the buffer is 'measured' when doing taffy layout (taffy needs to ignore spans).
There was a problem hiding this comment.
However, I'm leaning toward the idea that text spans as individual entities is not worth the trouble. Perhaps they can just be consolidated into a component and then the whole question of ghost nodes becomes moot. Need to avoid landing back on TextSection ugliness...
There was a problem hiding this comment.
yes, it is the reason I also researched and PoC-d a text create, and yes, that PR is the proper solution instead of ghost nodes. Except that is mile-stoned for 0.16.0. so maybe next year
Also, I think text measurement should be provided with a bounding box which should be coming from the node constraints. The only issue here is that the constraints don't exist before text is measured so to me the problem really is that calcs are done late and only once.
I would also like to point out that the entity per span is not the entire solution for text, as you cannot make a GPU renderer for it if its chopped up in entities (not without a render texture rendering an entire UI block - but that prevents splatting text on any part of the UV). However, if text is chopped up and has its own hierarchy, GPU rendering is much easier once you grab the entire text tree.
There was a problem hiding this comment.
We need text spans to not participate in layout. The alternative is Display::None but then you start lugging around useless Node and Style components. GhostNode is currently the most well-suited to the task.
TextSpans aren't UI Nodes (or at least, have no reason to be as they are "metadata" / not rendered directly). Why would they participate in layout?
In the distant future we may have 'in-line UI' in text blocks. But there is no design around that now (it may not be in-tree, or may use different relations) so I agree text spans can be considered simple leafs.
Even in those cases, I think it probably could serve as an independent UI root. Id prefer to punt that scenario down the road until we actually choose to implement it, in favor of the simpler design here.
I think for this PR we should:
- Remove the GhostNode require
- Remove the "arbitrary propagation of TextSpan" up the hierarchy. TextSpans only propagate up other TextSpans until they either hit a TextBlock (success) or hit a parent without a TextSpan or TextBlock (invalid).
- Remove TextSpan2d and embrace TextSpan as the "one context-free span type".
If you're burnt out on changes let me know. Happy to hop in.
There was a problem hiding this comment.
TextSpans aren't UI Nodes (or at least, have no reason to be as they are "metadata" / not rendered directly). Why would they participate in layout?
They are not, but they are in-tree and the layout algorithm walks the tree. Currently we issue a warning (or panic?) if any in-tree entity is missing Node. Whether or not to 'ignore' entities without Node is part of the discussion around ghost nodes. I'm leaning toward EidLoi's position that hierarchies should be self-contained structures and that non-uniform entities should be attached with tailored relationships. So, text spans should be attached as an ancillary hierarchy that doesn't use Parent/Child (maybe ParentSpan/ChildSpan).
Remove the "arbitrary propagation of TextSpan" up the hierarchy.
This was already fixed, it was a bug.
Remove TextSpan2d and embrace TextSpan as the "one context-free span type".
Only possible if changes are maybe to UI layout to make ghost nodes implicit instead of/in addition to explicit. Or if we introduce a different relationship structure.
If you're burnt out on changes let me know. Happy to hop in.
Not quite yet but I probably only make big changes that I'm confident are improving the situation.
There was a problem hiding this comment.
Currently we issue a warning (or panic?) if any in-tree entity is missing Node.
I think UI nodes having non UI children should probably be valid. Its when a UI node has non-UI children who then have UI nodes where we have problems.
Whether or not to 'ignore' entities without Node is part of the discussion around ghost nodes. I'm leaning toward EidLoi's position that hierarchies should be self-contained structures and that non-uniform entities should be attached with tailored relationships. So, text spans should be attached as an ancillary hierarchy that doesn't use Parent/Child (maybe ParentSpan/ChildSpan).
I agree that this is a path worth exploring. But it feels pretty non-viable without both relations and scene construction tools that make multi-hierarchy management (aka multi-relation construction) easier. The current Bevy paradigm is "one hierarchy" and we should design under that constraint until that changes.
This was already fixed, it was a bug.
Excellent.
Only possible if changes are maybe to UI layout to make ghost nodes implicit instead of/in addition to explicit. Or if we introduce a different relationship structure.
Having thought on this, I think the path forward is removing GhostNode and making replacing the no UI->NonUI layout constraint with a no UI->NonUi->UI constraint. Then we unify on a single TextSpan type as it can be a non-ui contextless component.
Not a fan of using GhostNode to enable these "leaf-like" cases because we're heavily considering reverting that changeset (it is still experimental), and I think these "leaf-like" cases should be valid.
There was a problem hiding this comment.
Having thought on this, I think the path forward is removing GhostNode and making replacing the no UI->NonUI layout constraint with a no UI->NonUi->UI constraint. Then we unify on a single TextSpan type as it can be a non-ui contextless component.
This is fine with me, but changing the layout algorithm should not be part of this PR. Can you open a new PR supporting the changes you want? Then I will rebase once that's merged and consolidate the span types.
…pper (#15740) # Objective - Closes #15720 ## Solution Wrap the handle in a new wrapper component: `UiMaterialHandle` It's not possible to match the naming convention of `MeshMaterial3d/2d` here with the trait already being called `UiMaterial` Should we consider renaming to `Material3d/2dHandle` and `Mesh3d/2d` to `Mesh3d/2dHandle`? - This shouldn't have any merge conflicts with #15591 ## Testing Tested the `ui_material` example ## Migration Guide Let's defer the migration guide to the required component port. I just want to yeet the `Component` impl on `Handle` in the meantime :)
# Objective As discussed in #15591, this warning prevents us from storing leaf nodes without a `Style` component. Because text sections (as distinct entities) should not be laid out using `taffy`, this warning is incorrect. Users may also have other uses for doing this, and this should generally increase flexibility without posing particularly serious correctness concerns. ## Solution - removed warning about non-UI children with UI parents - improved the warning about UI parents with non-UI parents - this warning should stay, for now, as it results in a genuine failure to perform `taffy` layout - that said, we should be clearer about the cause and potentially harmful results of this! ## Testing I inserted an empty entity into the hierarchy in the `button` example as a leaf node, and it ran with no warnings.
I'd prefer to keep the current design, which is much more explicit and does not further increase the complexity of |
# Objective - Improve clarity when spawning a text block. See [this discussion](#15591). ## Solution - Rename `TextBlock` to `TextLayout`.
# Objective Fixes a mistake in the migration done in #15591. ## Solution Restore a line of instructions that was accidentally dropped. ## Testing `cargo run --example motion_blur` Tested that instructions make sense and text updates correctly when keys are pressed.
# Objective Fixes #15832 ## Solution It seems that this was just a transliteration mistake during #15591. Update the correct text span index. ## Testing I tested on macos with: `cargo run --example gamepad_viewer` - without gamepad connected - with gamepad connected - disconnecting and reconnecting gamepad while running
# Objective Cleanup naming and docs, add missing migration guide after #15591 All text root nodes now use `Text` (UI) / `Text2d`. All text readers/writers use `Text<Type>Reader`/`Text<Type>Writer` convention. --- ## Migration Guide Doubles as #15591 migration guide. Text bundles (`TextBundle` and `Text2dBundle`) were removed in favor of `Text` and `Text2d`. Shared configuration fields were replaced with `TextLayout`, `TextFont` and `TextColor` components. Just `TextBundle`'s additional field turned into `TextNodeFlags` component, while `Text2dBundle`'s additional fields turned into `TextBounds` and `Anchor` components. Text sections were removed in favor of hierarchy-based approach. For root text entities with `Text` or `Text2d` components, child entities with `TextSpan` will act as additional text sections. To still access text spans by index, use the new `TextUiReader`, `Text2dReader` and `TextUiWriter`, `Text2dWriter` system parameters.
# Objective Fixes #16978 While testing, discovered that the morph weight interface in `scene_viewer` has been broken for a while (panics when loaded model has morph weights), probably since #15591. Fixed that too. While testing, saw example text in morph interface with [wrong padding](https://bevyengine.org/learn/contribute/helping-out/creating-examples/#visual-guidelines). Fixed that too. Left the small font size because there may be a lot of morphs to display, so that seems intentional. ## Solution Use normal queries and bail early ## Testing Morph interface can be tested with ``` cargo run --example scene_viewer assets/models/animated/MorphStressTest.gltf ``` ## Discussion I noticed that this fix is different than what is happening in #16976. Feel free to discard this for an alternative fix. I opened this anyway to document the issue with morph weight display. This is on top of #16966 which is required to test. --------- Co-authored-by: François Mockers <francois.mockers@vleue.com> Co-authored-by: François Mockers <mockersf@gmail.com>
Fixes #16978 While testing, discovered that the morph weight interface in `scene_viewer` has been broken for a while (panics when loaded model has morph weights), probably since #15591. Fixed that too. While testing, saw example text in morph interface with [wrong padding](https://bevyengine.org/learn/contribute/helping-out/creating-examples/#visual-guidelines). Fixed that too. Left the small font size because there may be a lot of morphs to display, so that seems intentional. Use normal queries and bail early Morph interface can be tested with ``` cargo run --example scene_viewer assets/models/animated/MorphStressTest.gltf ``` I noticed that this fix is different than what is happening in #16976. Feel free to discard this for an alternative fix. I opened this anyway to document the issue with morph weight display. This is on top of #16966 which is required to test. --------- Co-authored-by: François Mockers <francois.mockers@vleue.com> Co-authored-by: François Mockers <mockersf@gmail.com>
# Objective Fixes bevyengine#16978 While testing, discovered that the morph weight interface in `scene_viewer` has been broken for a while (panics when loaded model has morph weights), probably since bevyengine#15591. Fixed that too. While testing, saw example text in morph interface with [wrong padding](https://bevyengine.org/learn/contribute/helping-out/creating-examples/#visual-guidelines). Fixed that too. Left the small font size because there may be a lot of morphs to display, so that seems intentional. ## Solution Use normal queries and bail early ## Testing Morph interface can be tested with ``` cargo run --example scene_viewer assets/models/animated/MorphStressTest.gltf ``` ## Discussion I noticed that this fix is different than what is happening in bevyengine#16976. Feel free to discard this for an alternative fix. I opened this anyway to document the issue with morph weight display. This is on top of bevyengine#16966 which is required to test. --------- Co-authored-by: François Mockers <francois.mockers@vleue.com> Co-authored-by: François Mockers <mockersf@gmail.com>
# Objective Fixes bevyengine#16978 While testing, discovered that the morph weight interface in `scene_viewer` has been broken for a while (panics when loaded model has morph weights), probably since bevyengine#15591. Fixed that too. While testing, saw example text in morph interface with [wrong padding](https://bevyengine.org/learn/contribute/helping-out/creating-examples/#visual-guidelines). Fixed that too. Left the small font size because there may be a lot of morphs to display, so that seems intentional. ## Solution Use normal queries and bail early ## Testing Morph interface can be tested with ``` cargo run --example scene_viewer assets/models/animated/MorphStressTest.gltf ``` ## Discussion I noticed that this fix is different than what is happening in bevyengine#16976. Feel free to discard this for an alternative fix. I opened this anyway to document the issue with morph weight display. This is on top of bevyengine#16966 which is required to test. --------- Co-authored-by: François Mockers <francois.mockers@vleue.com> Co-authored-by: François Mockers <mockersf@gmail.com>
Ready for review. Examples migration progress: 100%.
Objective
Solution
This implements cart's proposal faithfully.
Extra changes:
EntityCommands::commands_mutthat returns a mutable reference. This is a blocker for extension methods that return something other thanself. Note thatsickle_ui'sUiBuilder::commandsreturns a mutable reference for this reason.Testing
Showcase
TODO: showcase-worthy
Migration Guide
TODO: very breaking
Accessing text spans by index
Text sections are now text sections on different entities in a hierarchy, Use the new
TextReaderandTextWritersystem parameters to access spans by index.Before:
After:
Iterating text spans
Text spans are now entities in a hierarchy, so the new
UiTextReaderandUiTextWritersystem parameters provide ways to iterate that hierarchy. TheUiTextReader::itermethod will give you a normal iterator over spans, andUiTextWriter::for_eachlets you visit each of the spans.