add line height to `TextFont` #16614

Cyborus04 · 2024-12-02T19:41:38Z

Objective

Allow users to customize the line height of text.
Implements Let users control the line height of their text #16085

Solution

Add a line_height field to TextFont to feed into cosmic_text's Metrics.

Testing

Tested on my own game, and worked exactly as I wanted.
My game is only 2D, so I only tested Text2d. Text still needs tested, but I imagine it'll work fine.
An example is available here

Showcase

Click to view showcase

With font:

TextFont {
    font: /* unimportant */,
    font_size: 16.0,
    line_height: None,
    ..default()
}

With font:

TextFont {
    font: /* unimportant */,
    font_size: 16.0,
    line_height: Some(16.0),
    ..default()
}

Migration Guide

TextFont now has a line_height field. Any instantiation of TextFont that doesn't have ..default() will need to add this field.

ickshonpe · 2024-12-02T21:11:54Z

crates/bevy_text/src/text.rs

+    /// If not set, the line height will be 1.2 * `font_size`.
+    pub line_height: Option<f32>,


Is there a point to making this optional, it could just be an f32 with a default value of 1.2 instead?

It's measured in pixels, not multiples of the font size (though that might be a better interface). Working with Options makes it simpler to implement too IMO, but changing it wouldn't be a huge deal

Oh I see it's not a scalar. I think then it would better if there was a LineHeight enum maybe with Px and Scalar variants or something.

I like that! Best of both worlds

in that case, should it be a field of TextFont, or its own component? Being a separate component would make it so it's not a breaking change at all, but I'm not sure yet how to do that

Otherwise it looks good, the UI text debug examples are still working with different line heights, don't see any problems.

in that case, should it be a field of TextFont, or its own component? Being a separate component would make it so it's not a breaking change at all, but I'm not sure yet how to do that

The current direction atm with Bevy seems to be less granular APIs so a field on TextFont is preferable probably. A field is more discoverable than a separate component as well.

coreh

Nice! Left a couple comments, mostly about the LineHeight enum, but also about a potential issue with the line height math.

crates/bevy_text/src/text.rs

coreh · 2024-12-06T20:29:16Z

crates/bevy_text/src/text.rs

+
+impl Default for LineHeight {
+    fn default() -> Self {
+        LineHeight::Scalar(1.2)


To make it easier to expand the feature set in the future (by propagating inherited line height across a hierarchy etc) without having to do a breaking change, would it make sense to have a separate LineHeight::Auto variant as the default?

What would that do?

crates/bevy_text/src/pipeline.rs

BenjaminBrienen · 2024-12-23T23:04:30Z

@coreh everything check out?

coreh · 2024-12-23T23:06:44Z

At a quick glance, LGTM! 👍

Cyborus04 · 2024-12-24T15:46:02Z

do the labels need updated?

BenjaminBrienen · 2024-12-24T16:22:43Z

You need to fix the no-std build. It can be in final review after 2 community approvals.

Cyborus04 · 2024-12-24T17:25:40Z

I'm not sure what I need to do to fix the nostd build, the error was in a different crate than my changes

BenjaminBrienen · 2024-12-24T17:35:43Z

Weird. FYI @bushrat011899

bushrat011899 · 2024-12-24T20:44:17Z

@Cyborus04 just tested your PR locally, it'll pass the no_std checks once you update this PR to the latest changes on main. I don't have a good answer as to why you need to upstream from main, but that should fix the issue.

Cyborus04 · 2024-12-24T22:51:25Z

I had a suspicion that's all I needed to do, I just wasn't at a computer to do so then

rparrett · 2025-01-05T19:53:35Z

crates/bevy_text/src/text.rs

+    /// Set line height to a specific number of pixels
+    Px(f32),
+    /// Set line height to a multiple of the font size
+    Scalar(f32),


I am not sure that the name Scalar makes sense here.

Maybe Relative? Em? I will ask other UI people to weigh in.

FontRelative gets my vote.

Relative / Em might be a bit confusing and FontRelative seems a bit odd to me.

RelativeToFontSize ? It's a bit long but completely unambiguous.

In CSS, the equivalent here seems to be just a unitless number. So this at least shouldn't be called Em, since that behaves differently.
https://developer.mozilla.org/en-US/docs/Web/CSS/line-height#values

I think RelativeToFont or Factor would be my suggestion

Yeah RelativeToFont seems fine to me.

rparrett · 2025-01-06T23:41:39Z

Making a note that the Showcase section of the PR description still has older syntax from a previous iteration of this PR.

# Objective Followup from #16614 `TextFont` has builder methods for its other fields. Add `with_line_height` for consistency. ## Solution Add it

# Objective - Allow users to customize the line height of text. - Implements bevyengine#16085 ## Solution - Add a `line_height` field to `TextFont` to feed into `cosmic_text`'s `Metrics`. ## Testing - Tested on my own game, and worked exactly as I wanted. - My game is only 2D, so I only tested `Text2d`. `Text` still needs tested, but I imagine it'll work fine. - An example is available [here](https://code.cartoon-aa.xyz/Cyborus/custom-line-height-example) --- ## Showcase <details> <summary>Click to view showcase</summary> With font: ```rust TextFont { font: /* unimportant */, font_size: 16.0, line_height: None, ..default() } ``` ![image](https://github.com/user-attachments/assets/d12d8334-72ae-44b4-9b2e-993bbfd19da6) With font: ```rust TextFont { font: /* unimportant */, font_size: 16.0, line_height: Some(16.0), ..default() } ``` ![image](https://github.com/user-attachments/assets/6bc843b0-b633-4c30-bf77-6bbad774c1e5) </details> ## Migration Guide `TextFont` now has a `line_height` field. Any instantiation of `TextFont` that doesn't have `..default()` will need to add this field.

# Objective Followup from bevyengine#16614 `TextFont` has builder methods for its other fields. Add `with_line_height` for consistency. ## Solution Add it

Cyborus04 force-pushed the custom-line-height branch 2 times, most recently from 2904af4 to 52c76ff Compare December 2, 2024 20:11

ickshonpe suggested changes Dec 2, 2024

View reviewed changes

Cyborus04 force-pushed the custom-line-height branch 2 times, most recently from 2344bdb to fb378bc Compare December 2, 2024 22:44

ickshonpe added A-Text Rendering and layout for characters C-Feature A new feature, making something new possible S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Dec 2, 2024

coreh reviewed Dec 6, 2024

View reviewed changes

Cyborus04 force-pushed the custom-line-height branch 3 times, most recently from 83f0e91 to bb38174 Compare December 10, 2024 03:26

coreh approved these changes Dec 23, 2024

View reviewed changes

Cyborus04 force-pushed the custom-line-height branch from bb38174 to c63bfe5 Compare December 24, 2024 22:50

BenjaminBrienen 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 Dec 25, 2024

rparrett reviewed Jan 5, 2025

View reviewed changes

feat: add line height to TextFont

f4516ee

Cyborus04 force-pushed the custom-line-height branch from c63bfe5 to f4516ee Compare January 6, 2025 18:37

alice-i-cecile added this to the 0.16 milestone Jan 6, 2025

BenjaminBrienen approved these changes Jan 6, 2025

View reviewed changes

alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jan 6, 2025

alice-i-cecile approved these changes Jan 6, 2025

View reviewed changes

alice-i-cecile added this pull request to the merge queue Jan 6, 2025

alice-i-cecile added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Jan 6, 2025

Merged via the queue into bevyengine:main with commit 4ba09f3 Jan 6, 2025
30 checks passed

rparrett mentioned this pull request Jan 6, 2025

Add builder method for TextFont::line_height #17203

Merged

github-merge-queue bot pushed a commit that referenced this pull request Jan 7, 2025

Add builder method for TextFont::line_height (#17203)

f896ad8

# Objective Followup from #16614 `TextFont` has builder methods for its other fields. Add `with_line_height` for consistency. ## Solution Add it

rparrett mentioned this pull request Jan 10, 2025

Let users control the line height of their text #16085

Closed

		/// If not set, the line height will be 1.2 * `font_size`.
		pub line_height: Option<f32>,

Uh oh!

add line height to TextFont #16614

add line height to TextFont #16614

Uh oh!

Conversation

Cyborus04 commented Dec 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Solution

Testing

Showcase

Migration Guide

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Cyborus04 Dec 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ickshonpe Dec 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coreh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

BenjaminBrienen commented Dec 23, 2024

Uh oh!

coreh commented Dec 23, 2024

Uh oh!

Cyborus04 commented Dec 24, 2024

Uh oh!

BenjaminBrienen commented Dec 24, 2024

Uh oh!

Cyborus04 commented Dec 24, 2024

Uh oh!

BenjaminBrienen commented Dec 24, 2024

Uh oh!

bushrat011899 commented Dec 24, 2024

Uh oh!

Cyborus04 commented Dec 24, 2024

Uh oh!

rparrett Jan 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ickshonpe Jan 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jondolf Jan 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rparrett commented Jan 6, 2025

Uh oh!

Uh oh!

add line height to `TextFont` #16614

add line height to `TextFont` #16614

Cyborus04 commented Dec 2, 2024 •

edited

Loading

Cyborus04 Dec 2, 2024 •

edited

Loading

ickshonpe Dec 2, 2024 •

edited

Loading

rparrett Jan 5, 2025 •

edited

Loading

ickshonpe Jan 5, 2025 •

edited

Loading

Jondolf Jan 5, 2025 •

edited

Loading