Skip to content

Conversation

ickshonpe
Copy link
Contributor

@ickshonpe ickshonpe commented Sep 23, 2025

Objective

Propagate text styles down the tree, instead of setting them per node.

Work towards #21175

Solution

  • New component ComputedTextStyle, required on Text, Text2d, and TextSpan.
  • Unrequire TextFont and TextColor from Text, Text2d, and TextSpan.
  • New system update_text_styles. It updates each ComputedTextStyle, from either the current entity or the nearest ancestor with a TextFont or TextColor.
  • New resource DefaultTextStyle, used if update_text_styles can't find any ancestor with a TextColor or TextFont.

update_text_styles isn't optimised at all yet. Every frame (there's no change detection), from every text entity, it walks up the tree until it finds a TextColor and TextFont to use. It's okay as a reference implementation though, more concerned with synchronisation errors at this stage.

Most existing code should work without changes. Because they are no longer required, TextFont and TextColor aren't automatically inserted on text entities when ellided, so instead of the default font, they will use the font and color they inherit from their ancestors instead. And queries for TextFont and TextColor that assume the components presence may also fail now.

Testing

The text2d, feathers and testbed_ui examples all seem to be working correctly with only minor changes.

* Removed `TextFont` and `TextColor` from the `Text`, `Text2d`, and `TextSpan` requires, replaced with `ComputedTextStyle`.
* `update_text_styles` updates the `ComputedTextStyle`s each frame from the text entities nearest ancestors with `TextFont` or `TextColor` components.
Copy link
Contributor

Your PR caused a change in the graphical output of an example or rendering test. This might be intentional, but it could also mean that something broke!
You can review it at https://pixel-eagle.com/project/B04F67C0-C054-4A6F-92EC-F599FEC2FD1D?filter=PR-21181

If it's expected, please add the M-Deliberate-Rendering-Change label.

If this change seems unrelated to your PR, you can consider updating your PR to target the latest main branch, either by rebasing or merging main into it.

@TimJentzsch TimJentzsch added C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Text Rendering and layout for characters S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 23, 2025
/// default font
pub font: TextFont,
/// default color
pub color: Color,
Copy link
Contributor

Choose a reason for hiding this comment

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

This might also be a good place to stash the global default font size, although that might be a different PR. Maybe a TODO?

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 was thinking the global default font size would just be taken from the font_size from the DefaultTextStyle's TextFont field. I left this part deliberately underdeveloped though because I wasn't sure what we wanted and it's probably better to deal with in another PR.

@viridia
Copy link
Contributor

viridia commented Sep 24, 2025

I propose adding a release note that covers all of the various font-related work we'd like to do. I'm happy to help with writing the text if needed.

@ickshonpe ickshonpe mentioned this pull request Sep 25, 2025
#[derive(Component, Default, Clone, Debug, Reflect)]
#[reflect(Component, Default)]
#[require(ThemedText, PropagateOver::<TextFont>::default())]
#[require(ThemedText)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we even need ThemedText any more?

Copy link
Contributor Author

@ickshonpe ickshonpe Sep 25, 2025

Choose a reason for hiding this comment

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

Doesn't look like it, I thought I'd leave it for now though, and focus this PR on the text module changes.

@ickshonpe
Copy link
Contributor Author

I propose adding a release note that covers all of the various font-related work we'd like to do. I'm happy to help with writing the text if needed.

Yeah, that would be a big help, there are so many changes, I'm not looking forward to it. I've got another couple of PRs that should be published tomorrow, that cover the relative font sizes and text span rework.

@viridia
Copy link
Contributor

viridia commented Sep 26, 2025

I was thinking we should file a cover bug which links to all the various font-related changes we want to make in 0.18.

@alice-i-cecile

@ickshonpe
Copy link
Contributor Author

ickshonpe commented Sep 26, 2025

I don't feel that happy with the name of the DefaultTextStyle resource. Maybe something like GlobalTextStyle be better? It needs to be clear that this isn't a Default text style that gets applied if you don't explicitly style a text node. Instead it's the initial text style values that are propagated down each hierarchy supporting text that is overridden by the TextFont and TextColor components.

Maybe IntialTextStyle or PropagatedTextStyle would be okay, I'm not sure. Or GlobalPropagatedTextStyle but it's getting a bit long then.

Also it should be possible to set a default text style per text implementation as well. Perhaps by adding a type parameter, have DefaultTextStyle<()> be global, which is overridden for Text or Text2d if the DefaultTextStyle<Text> or DefaultTextStyle<Text2d> resources are present, respectively. I'll leave this for a follow up though.

@viridia
Copy link
Contributor

viridia commented Sep 27, 2025

I don't have a problem with the name DefaultTextStyle, but I don't think I really grasp the distinction you are making.

It needs to be clear that this isn't a Default text style that gets applied if you don't explicitly style a text node.

My understanding is that it's an inherited default: it applies to any text entity that doesn't have an explicit style, nor has any ancestor that has an explicit style set.

Part of the problem is that we don't have a word to describe the domain that encompasses all user interface hierarchies. If all Window entities were children of a common parent entity, then we wouldn't need resources, we could just stick these components on the common parent.

Having a separate default for Text2D entities does make sense.

I also think that introducing ComputedTextStyle weakens the rationale for having TextFont contain both font-face and font size. That is: I think that the intuitive API would be one in which font face and font size are specified separately; however in a world where we use components to also track the lifetime of font atlases, it makes sense to combine them in a single component. However, if ComputedTextStyle is taking over the job of tracking those lifetimes, then that reason kind of disappears.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Text Rendering and layout for characters C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Needs-Review Needs reviewer attention (from anyone!) to move forward
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants