Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add line height to TextFont #16614

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Cyborus04
Copy link

@Cyborus04 Cyborus04 commented Dec 2, 2024

Objective

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()
}

image

With font:

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

image

Migration Guide

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

@Cyborus04 Cyborus04 force-pushed the custom-line-height branch 2 times, most recently from 2904af4 to 52c76ff Compare December 2, 2024 20:11
Comment on lines 290 to 291
/// If not set, the line height will be 1.2 * `font_size`.
pub line_height: Option<f32>,
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

I like that! Best of both worlds

Copy link
Author

@Cyborus04 Cyborus04 Dec 2, 2024

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@ickshonpe ickshonpe Dec 2, 2024

Choose a reason for hiding this comment

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

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.

@Cyborus04 Cyborus04 force-pushed the custom-line-height branch 2 times, most recently from 2344bdb to fb378bc Compare December 2, 2024 22:44
@ickshonpe 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
Copy link
Contributor

@coreh coreh left a comment

Choose a reason for hiding this comment

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

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 Show resolved Hide resolved
crates/bevy_text/src/text.rs Outdated Show resolved Hide resolved

impl Default for LineHeight {
fn default() -> Self {
LineHeight::Scalar(1.2)
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

What would that do?

crates/bevy_text/src/pipeline.rs Outdated Show resolved Hide resolved
@Cyborus04 Cyborus04 force-pushed the custom-line-height branch 3 times, most recently from 83f0e91 to bb38174 Compare December 10, 2024 03:26
@BenjaminBrienen
Copy link
Contributor

@coreh everything check out?

@coreh
Copy link
Contributor

coreh commented Dec 23, 2024

At a quick glance, LGTM! 👍

@Cyborus04
Copy link
Author

do the labels need updated?

@BenjaminBrienen
Copy link
Contributor

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

@BenjaminBrienen BenjaminBrienen added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged D-Straightforward Simple bug fixes and API improvements, docs, test and examples and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Dec 24, 2024
@Cyborus04
Copy link
Author

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
Copy link
Contributor

Weird. FYI @bushrat011899

@bushrat011899
Copy link
Contributor

@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
Copy link
Author

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

@BenjaminBrienen 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
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-Feature A new feature, making something new possible D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants