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 the ability to control font smoothing #15368

Merged
merged 12 commits into from
Sep 23, 2024

Conversation

coreh
Copy link
Contributor

@coreh coreh commented Sep 22, 2024

Objective

Solution

  • Introduce the FontSmoothing enum, with two possible variants (FontSmoothing::None and FontSmoothing::AntiAliased):
    • This is based on -webkit-font-smoothing, in line with our practice of adopting CSS-like properties/names for UI;
    • I could have gone instead for the font-smooth property that's also supported by browsers, but didn't since it's also non-standard, has an uglier name, and doesn't allow controlling the type of antialias applied.
    • Having an enum instead of e.g. a boolean, leaves the path open for adding FontSmoothing::SubpixelAntiAliased in the future, without a breaking change;
  • Add all the necessary plumbing to get the FontSmoothing information to where we rasterize the glyphs and store them in the atlas;
  • Change the font atlas key to also take into account the smoothing setting, not only font and font size;
  • Since COSMIC Text doesn't support controlling font smoothing, we roll out our own threshold-based “implementation”:
    • This has the downside of looking ugly for “regular” vector fonts ⚠️, since it doesn't properly take the hinting information into account like a proper implementation on the rasterizer side would.
    • However, for fonts that have been specifically authored to be pixel fonts, (a common use case in games!) this is not as big of a problem, since all lines are vertical/horizontal, and close to the final pixel boundaries (as long as the font is used at a multiple of the size originally intended by the author)
    • Once COSMIC exposes this functionality, we can switch to using it directly, and get better results;
  • Use a nearest neighbor sampler for atlases with font smoothing disabled, so that you can scale the text via transform and still get the pixelated look;
  • Add a convenience method to Text for setting the font smoothing;
  • Add a demonstration of using the FontSmoothing property to the text2d example.

Testing

  • Did you test these changes? If so, how?
    • Yes. Via the text2dexample, and also in my game.
  • Are there any parts that need more testing?
    • I'd like help from someone for testing this on devices/OSs with fractional scaling (Android/Windows)
  • How can other people (reviewers) test your changes? Is there anything specific they need to know?
    • Both via the text2d example and also by using it directly on your projects.
  • If relevant, what platforms did you test these changes on, and are there any important ones you can't test?
    • macOS

Showcase

commands.spawn(Text2dBundle {
    text: Text::from_section("Hello, World!", default())
        .with_font_smoothing(FontSmoothing::None),
    ..default()
});

Screenshot 2024-09-22 at 12 33 39

image

Migration Guide

  • Text now contains a font_smoothing: FontSmoothing property, make sure to include it or add ..default() when using the struct directly;
  • FontSizeKey has been renamed to FontAtlasKey, and now also contains the FontSmoothing setting;
  • The following methods now take an extra font_smoothing: FontSmoothing argument:
    • FontAtlas::new()
    • FontAtlasSet::add_glyph_to_atlas()
    • FontAtlasSet::get_glyph_atlas_info()
    • FontAtlasSet::get_outlined_glyph_texture()

@alice-i-cecile alice-i-cecile added A-UI Graphical user interfaces, styles, layouts, and widgets A-Text Rendering and layout for characters S-Needs-Review Needs reviewer attention (from anyone!) to move forward C-Feature A new feature, making something new possible M-Needs-Release-Note Work that should be called out in the blog due to impact M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Sep 22, 2024
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Great; this is useful and I'm happy with this. Really quite simple! I would prefer if we can document how this relates to #15170 as well; maybe add cross-links to both the settings added here and those added there?

I think both are worth including, since exposing different levels of granularity can be important.

@coreh
Copy link
Contributor Author

coreh commented Sep 22, 2024

I'll add some cross-linking between the two. 👍

Looking at the UI antialias setting, it seems to be camera driven. Would it be desirable to also have the same for this? We won't be able to do that currently, unless we refactor the font atlases to also be per-camera. (Which might be necessary down the line for supporting non-global scaling factors per-view)

@alice-i-cecile
Copy link
Member

IMO not at the current time: I'd prefer to have a simple fix in, and work towards a broader, more controversial refactor later.

@alice-i-cecile
Copy link
Member

When we're writing the release notes we can and should cover how all of these options interact with each other :) Not fully documented (book chapter on pixel art games please), but it will at least be googleable.

@coreh
Copy link
Contributor Author

coreh commented Sep 22, 2024

Do we already have a Camera -> Image target -> Sprite -> Camera -> Window pixel perfect scaling example? It could probably be combined with that.

@alice-i-cecile
Copy link
Member

https://github.com/bevyengine/bevy/blob/main/examples/2d/pixel_grid_snap.rs is the closest. I'd really like to flesh it out though, yeah.

@UkoeHB
Copy link
Contributor

UkoeHB commented Sep 22, 2024

(Which might be necessary down the line for supporting non-global scaling factors per-view)

Can't you do this by rendering to a texture and then scaling the texture? That's part of what #15256 would make easy, although I could be missing something about the rendering precision.

EDIT: Actually, #15256 would probably scale the UI normally (and scale UI element positions relative to some fixed point) and then fit the texture size to line up with the adjusted contents. So no 'stretching' of the texture.

@@ -209,6 +210,18 @@ impl TextPipeline {
.map(move |layout_glyph| (layout_glyph, run.line_y))
})
.try_for_each(|(layout_glyph, line_y)| {
let mut layout_glyph = layout_glyph.clone();
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you expect the compiler to elide this clone when not needed? Otherwise I would move the new variable outside the loop and branch on font_smoothing to either update the outer variable and get a reference, or use the layout_glyph reference from the iterator.

Copy link
Contributor Author

@coreh coreh Sep 22, 2024

Choose a reason for hiding this comment

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

Wasn't able to get the temp variable outside the loop, because it couldn't be left initialized across the closure boundary, and the LayoutGlyph type doesn't have a Default implementation, so instead I create it inside the closure and conditionally shadow the layout_glyph with a reference to it (or with its original reference), which should have the same effect of preventing the extra clone when font smoothing is on.

@lewiszlw lewiszlw 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 Sep 23, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Sep 23, 2024
Copy link
Contributor

@TotalKrill TotalKrill left a comment

Choose a reason for hiding this comment

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

Very nice, well documented and FontAtlasKey is a much better name!

Merged via the queue into bevyengine:main with commit 8e3db95 Sep 23, 2024
27 checks passed
@alice-i-cecile
Copy link
Member

Thank you to everyone involved with the authoring or reviewing of this PR! This work is relatively important and needs release notes! Head over to bevyengine/bevy-website#1694 if you'd like to help out.

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 A-UI Graphical user interfaces, styles, layouts, and widgets C-Feature A new feature, making something new possible M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide M-Needs-Release-Note Work that should be called out in the blog due to impact S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pixelated text font is blurry
5 participants