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

Use subimages to render MonoFont text #573

Merged
merged 1 commit into from
Apr 1, 2021

Conversation

rfuest
Copy link
Member

@rfuest rfuest commented Mar 28, 2021

This draft PR contains some of the planned changes to the text rendering API. It depends on #572 and cannot be merged before that PR, but also needs some polish.

All monospaced fonts are now stored as MonoFont stucts instead of using one type per font. This should make it easier to dynamically use different fonts and also allows fonts to be created at runtime. Because the renderer now uses Image and SubImage internally its possible to extend the mono font render to support different storage formats in the future.

The Drawable trait was changed to support target specific drawables. This makes it possible to implement target specific text renders that were suggested by @ReeceStevens on the matrix channel. See test/text_renderer_target_specific.rs for an example.

@bugadani This will at least break the e-t examples, but might also cause other issues.

@rfuest rfuest added the wip Work in progress label Mar 28, 2021
@bugadani
Copy link
Member

This will at least break the e-t examples, but might also cause other issues.

Go right ahead, breaking things is fun :)

colors: C,
}

impl<'a, T: DrawTarget, C> MonoFontDrawTarget<'a, T, C> {
Copy link
Member

@bugadani bugadani Mar 28, 2021

Choose a reason for hiding this comment

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

I think you lost me here. Is this a DrawTarget that limits or extends the allowed operations? Both sound weird to me. The difference between the three implementations is the colors used, which I think would have a better place in a paint-like object.

At this point I'm actually more worried about embedded-gui. This way I don't think it'll be possible to use two different font sets in a gui? Or maybe I'm missing something atm, like labels creating their own draw targets.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think you lost me here. Is this a DrawTarget that limits or extends the allowed operations?

Maybe the name MonoFontDrawTarget is a bit confusing. MonoFontDrawTarget is a draw target adapter similar to the ColorConverted draw target adapter:

/// Creates a color conversion draw target.
///
/// A color conversion draw target is used to draw drawables with a different color type to a
/// draw target. The drawable color type must implement `Into<C>`, where `C` is the draw
/// target color type.
///
/// # Performance
///
/// Color conversion can be expensive on embedded hardware and should be avoided if possible.
/// Using the same color type for drawables and the draw target makes sure that no unnecessary
/// color conversion is used. But in some cases color conversion will be required, for example,
/// to draw images with a color format only known at runtime.
///
/// # Examples
///
/// This example draws a `BinaryColor` image to an `Rgb888` display.
///
/// ```
/// use embedded_graphics::{
/// image::{Image, ImageRaw},
/// mock_display::MockDisplay,
/// pixelcolor::{BinaryColor, Rgb888},
/// prelude::*,
/// };
///
/// /// The image data.
/// const DATA: &[u8] = &[
/// 0b11110000, //
/// 0b10010000, //
/// 0b10010000, //
/// 0b11110000, //
/// ];
///
/// // Create a `BinaryColor` image from the image data.
/// let raw_image = ImageRaw::<BinaryColor>::new(DATA, 4, 4);
/// let image = Image::new(&raw_image, Point::zero());
///
/// // Create a `Rgb888` display.
/// let mut display = MockDisplay::<Rgb888>::new();
///
/// // The image can't directly be drawn to the draw target because they use different
/// // color type. This will fail to compile:
/// // image.draw(&mut display)?;
///
/// // To fix this `color_converted` is added to enable color conversion.
/// image.draw(&mut display.color_converted())?;
/// #
/// # let mut expected = MockDisplay::from_pattern(&[
/// # "WWWW", //
/// # "WKKW", //
/// # "WKKW", //
/// # "WWWW", //
/// # ]);
/// #
/// # display.assert_eq(&expected);
/// #
/// # Ok::<(), core::convert::Infallible>(())
/// ```
fn color_converted<C>(&mut self) -> ColorConverted<'_, Self, C>
where
C: PixelColor + Into<Self::Color>;

It is only used internally and doesn't affect the external API. In contrast to the generic ColorConverted adapter, MonoFontDrawTarget can convert the draw calls made by a BinaryColor image drawable into different draw calls, depending on the colors which are set in a MonoTextStyle. If only one color (text or bg) is set it will call draw_iter on the parent target and if both colors are set it can call the more efficient fill_contiguous. To reduce the runtime overhead for this distinction there are three different impls of MonoFontDrawTarget.

The difference between the three implementations is the colors used, which I think would have a better place in a paint-like object.

The advantage of implementing the color conversion as a draw target adapter is that the font rendering becomes independent of the image drawable implementation. While MonoFont currently only supports ImageRaws to store the glyph image it wouldn't be hard to extend this to other image formats in the future. This would make it possible to load fonts from a BMP file or a custom compressed storage format. But it would add a type parameter to MonoFont, which this PR tried to get rid of, so that I opted not to implement this at the moment.

At this point I'm actually more worried about embedded-gui. This way I don't think it'll be possible to use two different font sets in a gui? Or maybe I'm missing something atm, like labels creating their own draw targets.

What do you mean by "font sets"? Fonts from different font renderers?

Copy link
Member

Choose a reason for hiding this comment

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

What do you mean by "font sets"? Fonts from different font renderers?

Yes. But I think I simply misunderstood what this PR means. That's the curse of a cursory glance. You're saying this is internal only so I guess my worries are invalid. I'll need to put in more time to check things, though.

Thanks for the info!

@rfuest rfuest force-pushed the font-subimage branch 3 times, most recently from 32df0a4 to c171aa5 Compare March 31, 2021 15:49
@rfuest rfuest removed the wip Work in progress label Mar 31, 2021
@rfuest rfuest marked this pull request as ready for review March 31, 2021 15:57
@rfuest
Copy link
Member Author

rfuest commented Mar 31, 2021

I've removed the support for target specific text renderers from this PR, because it caused some problems in other places. I might revisit that feature in another PR.

But the rest of this PR is ready for review now.

@rfuest rfuest requested a review from jamwaffles March 31, 2021 15:59
@rfuest rfuest changed the title Text refactor Use subimages to render MonoFont text Mar 31, 2021
Copy link
Member

@jamwaffles jamwaffles left a comment

Choose a reason for hiding this comment

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

LGTM

@rfuest rfuest merged commit 1a8ce93 into embedded-graphics:master Apr 1, 2021
@rfuest rfuest deleted the font-subimage branch April 1, 2021 15:39
@bugadani
Copy link
Member

To be honest, this change (transforming the fonts from types into constants) is a painful one. For one, I had to sprinkle around three lifetime parameters in half of my platform-dependent code in embedded-gui, and since MonoTextStyleBuilder needs a reference (to a constant...) I'm still not sure how to get my lib to compile. I wonder if my life would be easier of these were static globals instead of constants. I think the compiler might still be smart enough to remove the fonts that are not used, at least in release builds - not sure about debug, though.

On thing that's particularly hard to do is factory functions. I can't just say &FONT_6X10 in them, because then I would have a reference to a local. I could make my own static, but that's not particularly nice, or I could pass the font by value, which probably isn't optimal when a gui is label-heavy.

@rfuest
Copy link
Member Author

rfuest commented Apr 21, 2021

I'll need to think about this some more before I can give an adequate answer.

@bugadani
Copy link
Member

bugadani commented Apr 24, 2021

@rfuest don't think too hard about this just yet. I just noticed how NULL_FONT is done and I think I must be wrong here. I'll try to make embedded-gui work with what's currently been implemented here.

Edit: it's certainly not impossible, I have made embedded-gui work. I'm not sure how general my solution is at this moment, but I was certainly wrong when I said one can't hold a reference to a const.

@rfuest
Copy link
Member Author

rfuest commented May 24, 2021

@bugadani Has this issue been resolved entirely or do I still need to look into this?

@bugadani
Copy link
Member

I think it's been resolved, thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants