Text refactor #470

rfuest · 2020-11-11T18:59:33Z

Thank you for helping out with embedded-graphics development! Please:

Check that you've added passing tests and documentation
Add a CHANGELOG.md entry in the Unreleased section under the appropriate heading (Added, Fixed, etc) if your changes affect the public API
Run rustfmt on the project
Run just build (Linux/macOS only) and make sure it passes. If you use Windows, check that CI passes once you've opened the PR.

PR description

This PR adds support for external text renderers to e-g. TextStyle was converted to a trait that can be also implemented in other crates. To make drawing more efficient text renderers get direct access to the draw target and can use optimizations like fill_contiguous. Renderers can implement TextStylePixels to also provide a pixel iterator, but this isn't required.

Are the names MonospacedTextStyle and MonospacedTextStyleBuilder too long? Do you have a better suggestion?

jamwaffles · 2020-11-11T19:02:50Z

I'm a bit frazzled at the moment so I'll get round to a review later (sorry), but as for

Are the names MonospacedTextStyle and MonospacedTextStyleBuilder too long? Do you have a better suggestion?

How about MonoTextStyle and MonoTextStyleBuilder? You could change MonospacedFont to MonoFont, etc as well. Mono might be a bit ambiguous at this point, but my hope is that if it's used everywhere for all the monospaced fonts, the name along with all the documentation around it will be clear.

rfuest · 2020-11-11T19:03:49Z

I've also implemented an example of an external text renderer in my fork of the simulator repo: https://github.com/rfuest/simulator/blob/text-refactor/examples/text-seven-segment.rs

It can be used to draw seven segment displays with any scale:

The code is too complex to keep it as an example, but I think it could be useful to release the renderer as a separate crate.

rfuest · 2020-11-11T19:06:17Z

I'm a bit frazzled at the moment so I'll get round to a review later (sorry), but as for

Never mind, it's only a draft PR and you can review it when it's done.

How about MonoTextStyle and MonoTextStyleBuilder? You could change MonospacedFont to MonoFont, etc as well. Mono might be a bit ambiguous at this point, but my hope is that if it's used everywhere for all the monospaced fonts, the name along with all the documentation around it will be clear.

Sounds good to me, thanks for the suggestion.

src/fonts/mod.rs

src/fonts/monospaced_pixels.rs

bugadani · 2020-11-13T13:47:33Z

src/fonts/monospaced_pixels.rs

+                    break Some(Pixel(Point::new(x, y), color));
+                }
+            } else if let Some(current_char) = self.current_char {
+                let color = if F::character_pixel(


While it's not a huge difference, this lookup has a bit of perf impact. In embedded-text, I made a glyph renderer struct that contains the pixel offset instead of the character and avoids some unnecessary re-calculations for each pixel. Also I think it would be nice if this code could be reused in embedded-text, but I think I might implement that after this is merged :)

I want to try to reuse the RawDataIter that is used to draw images. This would reduce some code duplication but I'm not sure about the performance impact yet. If that doesn't work out we can look into optimizing this code.

While it's not a huge difference, this lookup has a bit of perf impact.

Did you see a perf impact running just bench font? I realise they're contrived benchmarks and perhaps not representative of realworld performance, but the text benches improve ~10% for me on this branch compared to master.

CHANGELOG.md

jamwaffles · 2020-11-15T11:16:08Z

src/fonts/monospaced_pixels.rs

+                    break Some(Pixel(Point::new(x, y), color));
+                }
+            } else if let Some(current_char) = self.current_char {
+                let color = if F::character_pixel(


While it's not a huge difference, this lookup has a bit of perf impact.

Did you see a perf impact running just bench font? I realise they're contrived benchmarks and perhaps not representative of realworld performance, but the text benches improve ~10% for me on this branch compared to master.

jamwaffles

My eyes have gone a bit square looking at the word Mono over and over again, but looks good overall.

One thing that's missing that I think is important is an example of implementing a custom font. I don't know if it's possible to add one that's short enough to show in the docs, but it would really help tie all the traits and structs together.

There's a reference to the text-custom-font simulator example somewhere (I can't find it in the diff now for some reason) which might be worth turning into a link too.

src/fonts/mod.rs

src/fonts/monospaced_pixels.rs

src/style/mono_text_style.rs

rfuest · 2020-11-15T14:09:51Z

I couldn't reply to this above:

Did you see a perf impact running just bench font? I realise they're contrived benchmarks and perhaps not representative of realworld performance, but the text benches improve ~10% for me on this branch compared to master.

I hadn't noticed that before, but I also get some performance improvements. Not quite 10%, but I hadn't expected any performance gains at all, so the 3%-7% are also welcome.

Since the benches were updated to use a framebuffer they should be pretty representative, but only for displays drivers with a framebuffer. Perhaps we should look into also adding benches for other displays, where the number of draw calls or the amount of data that has to be transmitted are better metrics.

Co-authored-by: James Waples <[email protected]>

rfuest · 2020-11-15T15:48:58Z

One thing that's missing that I think is important is an example of implementing a custom font. I don't know if it's possible to add one that's short enough to show in the docs, but it would really help tie all the traits and structs together.

There's a reference to the text-custom-font simulator example somewhere (I can't find it in the diff now for some reason) which might be worth turning into a link too.

An example in the docs wouldn't be much shorter than text-custom-font. If it's OK, I would like to create a new issue for the doc improvements and wait until the text refactor is finished.

jamwaffles · 2020-11-15T15:52:00Z

An example in the docs wouldn't be much shorter than text-custom-font. If it's OK, I would like to create a new issue for the doc improvements and wait until the text refactor is finished.

I thought that might be the case. No worries - we can add more in later.

jamwaffles

Looks good from my side. I don't know if @bugadani had anything else to add?

bugadani · 2020-11-15T16:07:38Z

Don't wait for me, I can't review in depth :(

rfuest · 2020-11-15T16:15:29Z

I've also implemented an example of an external text renderer in my fork of the simulator repo: https://github.com/rfuest/simulator/blob/text-refactor/examples/text-seven-segment.rs

It can be used to draw seven segment displays with any scale:

The code is too complex to keep it as an example, but I think it could be useful to release the renderer as a separate crate.

@jamwaffles If I decide to convert this example into a reusable crate do you think it would be useful enough to be an official e-g project or should I release it myself?

jamwaffles · 2020-11-15T16:19:39Z

I think it'd be right at home in the e-g org if that's what you mean. Would it end up being published on crates.io?

As complex as it is, it would also be a good example for those looking to implement a custom font.

rfuest · 2020-11-15T16:26:10Z

I think it'd be right at home in the e-g org if that's what you mean.

Yes, that's what I meant.

Would it end up being published on crates.io?

Yes. IMO a freely scalable seven segment font could be useful for different projects.

As complex as it is, it would also be a good example for those looking to implement a custom font.

Just to be clear: It's not a custom font, but a custom text renderer. But I agree that it could still be a good example for how an external renderer can be integrated.

jamwaffles · 2020-11-15T16:28:29Z

Yes, that's what I meant.

Yes. IMO a freely scalable seven segment font could be useful for different projects.

Definitely! You should be able to make repositories in the e-g org, so feel free to do so when you have time. Let me know if you run into permissions errors.

Just to be clear: It's not a custom font, but a custom text renderer. But I agree that it could still be a good example for how an external renderer can be integrated.

Right, renderer not font. I'll do my best to be clearer with the terminology from now on :)

rfuest · 2020-11-15T16:32:23Z

I'll do my best to be clearer with the terminology from now on :)

I just wanted to make sure we talk about the same thing ;).

bugadani reviewed Nov 13, 2020

View reviewed changes

src/fonts/mod.rs Outdated Show resolved Hide resolved

bugadani mentioned this pull request Nov 13, 2020

Add support for transparent characters #439

Merged

5 tasks

bugadani reviewed Nov 13, 2020

View reviewed changes

src/fonts/monospaced_pixels.rs Outdated Show resolved Hide resolved

bugadani reviewed Nov 13, 2020

View reviewed changes

rfuest force-pushed the text-refactor branch 2 times, most recently from 1a40674 to 784d7f7 Compare November 14, 2020 16:32

rfuest marked this pull request as ready for review November 14, 2020 16:43

rfuest requested a review from jamwaffles November 14, 2020 16:43

jamwaffles reviewed Nov 15, 2020

View reviewed changes

jamwaffles requested changes Nov 15, 2020

View reviewed changes

src/fonts/mod.rs Show resolved Hide resolved

src/fonts/mod.rs Outdated Show resolved Hide resolved

src/fonts/monospaced_pixels.rs Show resolved Hide resolved

src/style/mono_text_style.rs Outdated Show resolved Hide resolved

rfuest and others added 9 commits November 15, 2020 16:39

Rename Font to MonospacedFont

d831792

Remove variable glyph width support

7caa129

Rename TextStyle to MonospacedTextStyle

7078689

Move text rendering into TextStyle impl

8011b31

Accept any TextStyle in Text::into_styled

e56fd1d

Rename Monospaced... types to Mono...

3220ecd

Add docs and changelog entry

75738a9

Add Copy supertrait to MonoFont

92c3580

Apply suggestions from code review

6ee2f79

Co-authored-by: James Waples <[email protected]>

rfuest force-pushed the text-refactor branch from 9d20762 to 6ee2f79 Compare November 15, 2020 15:39

rfuest requested a review from jamwaffles November 15, 2020 15:49

jamwaffles approved these changes Nov 15, 2020

View reviewed changes

rfuest merged commit 696c4da into embedded-graphics:master Nov 15, 2020

rfuest mentioned this pull request Nov 15, 2020

Improve fonts module documentation #480

Open

This was referenced Nov 16, 2020

Allowing for different font implementations #329

Closed

Variable width font styling #268

Closed

Remove variable width glyph support for internal font renderer? #469

Closed

Feature request/idea: specifying space width for variable width fonts #443

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Text refactor #470

Text refactor #470

rfuest commented Nov 11, 2020

jamwaffles commented Nov 11, 2020

rfuest commented Nov 11, 2020

rfuest commented Nov 11, 2020

bugadani Nov 13, 2020

rfuest Nov 13, 2020

jamwaffles Nov 15, 2020

jamwaffles Nov 15, 2020

jamwaffles left a comment

rfuest commented Nov 15, 2020

rfuest commented Nov 15, 2020

jamwaffles commented Nov 15, 2020

jamwaffles left a comment

bugadani commented Nov 15, 2020

rfuest commented Nov 15, 2020

jamwaffles commented Nov 15, 2020

rfuest commented Nov 15, 2020

jamwaffles commented Nov 15, 2020

rfuest commented Nov 15, 2020

Text refactor #470

Text refactor #470

Conversation

rfuest commented Nov 11, 2020

PR description

jamwaffles commented Nov 11, 2020

rfuest commented Nov 11, 2020

rfuest commented Nov 11, 2020

bugadani Nov 13, 2020

Choose a reason for hiding this comment

rfuest Nov 13, 2020

Choose a reason for hiding this comment

jamwaffles Nov 15, 2020

Choose a reason for hiding this comment

jamwaffles Nov 15, 2020

Choose a reason for hiding this comment

jamwaffles left a comment

Choose a reason for hiding this comment

rfuest commented Nov 15, 2020

rfuest commented Nov 15, 2020

jamwaffles commented Nov 15, 2020

jamwaffles left a comment

Choose a reason for hiding this comment

bugadani commented Nov 15, 2020

rfuest commented Nov 15, 2020

jamwaffles commented Nov 15, 2020

rfuest commented Nov 15, 2020

jamwaffles commented Nov 15, 2020

rfuest commented Nov 15, 2020