ImageSharp.Drawing icon indicating copy to clipboard operation
ImageSharp.Drawing copied to clipboard

Rich text rendering

Open tocsoft opened this issue 2 years ago • 5 comments

Prerequisites

  • [x] I have written a descriptive pull-request title
  • [x] I have verified that there are no overlapping pull-requests open
  • [ ] I have verified that I am following matches the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules :cop:.
  • [ ] I have provided test coverage for my change (where applicable)

Description

This build on the SixLabors/Fonts#251 code to extend the capability to also altering the color / outlines. DrawRichTextWithMixOfPensAndBrushes_Solid500x200_(255,255,255,255)_RichText-F(40)

TODO

  • [x] Figure out what's causing the w/v shifting.
  • [x] Change back to Fonts package once SixLabors/Fonts#251 is merged.
  • [ ] Ensure we have enough test coverage

tocsoft avatar Mar 04 '22 19:03 tocsoft

Figure out what's causing the w/v shifting. This looks like it was due to the version of Open Sans we where using in this repo, using the version from the Fonts repo resolved that issue (didn't investigate too much on how/why they where different).

Maybe a hinting issue changing the version of the font did break a lot of tests however they all look like relative mine pixel shifting things but they all looked good to me

tocsoft avatar Mar 05 '22 13:03 tocsoft

This looks like it was due to the version of Open Sans we where using in this repo, using the version from the Fonts repo resolved that issue (didn't investigate too much on how/why they where different).

Ah yes, there were definitely issues with the one we were using and our layout engine was suffering for it. That's why we switched. I must have forgotten to update the one in drawing to match.

JimBobSquarePants avatar Mar 05 '22 14:03 JimBobSquarePants

@tocsoft I've just merged the latest main into this branch which allows us to update to the version of Fonts supporting this feature. I've also done a little cleanup and added comments.

One thing I'd really like to investigate with this is consolidation of drawing text with or without a path so that we can allow drawing of rich text along a path.

JimBobSquarePants avatar Apr 16 '22 14:04 JimBobSquarePants

I'm am quite confident it can be done, I've spent a couple of hours on it and got close but then failed... so i'm confident it'll be possible...the difficulty is caused by our internal/custom GlyphRenderer that render/caches the individual glyphs prior to compositing them together onto to image. (this is to speed up processing, at the cost of working memory, as we can avoid rasterizing the same glyph multiple times for a large body of text) This custom renderer has a pipeline that causes the rendering to be translated towards (0,0) first for the rasterization (why sometimes we had chopped glyphs) before being rendered to the correct pixel position during composition.

Something in that pipeline in introducing compounded translations for me at the moment ... so I'm thinking on going back to first principle and rebuild up the process ensuring the additional rotation and translation can be applied on the way through.

On in conclusion Bad news i've not get it working, good news I am confident it is possible to enable path rendering with rich text.

tocsoft avatar Apr 16 '22 18:04 tocsoft

Thanks for giving it an initial crack. (I’m wondering whether the compounding transforms are a result of Clear()?)

First principal sounds wise though. I did that a while back with convolution and ended up with a far more efficient and simple solution than I had previously.

JimBobSquarePants avatar Apr 17 '22 04:04 JimBobSquarePants

Is there anything I can do to help here? I've just updated to the latest Fonts build but didn't want to touch anything you were still tinkering with.

JimBobSquarePants avatar Oct 30 '22 04:10 JimBobSquarePants

@tocsoft I've cleaned up all the pens and brushes to use the inheritance pattern you designed in conjunction with the changes brought via #257

I can have a crack at the processor if you like but I'm not sure how successful I'll be. If you have any guidance, it would be much appreciated.

P.S I managed to corrupt LFS files with previous updates from main as long paths do not seem to be supported despite the config set to support them. I've fixed those files using the ones from main.

JimBobSquarePants avatar Mar 09 '23 01:03 JimBobSquarePants

I've gotten everything working. Just need to update the test reference images and add new ones. The new renderer is more accurate than the old one.

JimBobSquarePants avatar Mar 15 '23 03:03 JimBobSquarePants

@tocsoft When you have time can you have a look over this? I can't add you as a reviewer since you originally opened the PR.

JimBobSquarePants avatar Mar 15 '23 12:03 JimBobSquarePants

Codecov Report

Merging #211 (f6dbef1) into main (dc7b553) will decrease coverage by 1%. The diff coverage is 77%.

:exclamation: Current head f6dbef1 differs from pull request most recent head 5d116ce. Consider uploading reports for the commit 5d116ce to get more accurate results

@@         Coverage Diff          @@
##           main   #211    +/-   ##
====================================
- Coverage    71%    71%    -1%     
====================================
  Files        89     95     +6     
  Lines      5395   5628   +233     
  Branches   1098   1153    +55     
====================================
+ Hits       3872   4005   +133     
- Misses     1304   1387    +83     
- Partials    219    236    +17     
Flag Coverage Δ
unittests 71% <77%> (-1%) :arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/ImageSharp.Drawing/Processing/ImageBrush.cs 82% <0%> (-6%) :arrow_down:
...ageSharp.Drawing/Processing/LinearGradientBrush.cs 86% <0%> (-14%) :arrow_down:
src/ImageSharp.Drawing/Processing/PatternBrush.cs 77% <0%> (-6%) :arrow_down:
...ageSharp.Drawing/Processing/RadialGradientBrush.cs 80% <0%> (-20%) :arrow_down:
src/ImageSharp.Drawing/Processing/RecolorBrush.cs 88% <0%> (-8%) :arrow_down:
...ImageSharp.Drawing/Processing/PathGradientBrush.cs 82% <10%> (-5%) :arrow_down:
src/ImageSharp.Drawing/Processing/GradientBrush.cs 80% <20%> (-6%) :arrow_down:
src/ImageSharp.Drawing/Processing/SolidBrush.cs 89% <33%> (-5%) :arrow_down:
...ImageSharp.Drawing/Shapes/Text/BaseGlyphBuilder.cs 57% <40%> (-31%) :arrow_down:
src/ImageSharp.Drawing/Shapes/Text/TextBuilder.cs 62% <40%> (-38%) :arrow_down:
... and 23 more

... and 1 file with indirect coverage changes

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

codecov[bot] avatar Mar 15 '23 12:03 codecov[bot]

@tocsoft I've fixed everything based upon feedback. I've also done additional work on decoration positioning to exactly match the CSS approach.

JimBobSquarePants avatar Mar 16 '23 03:03 JimBobSquarePants

@JimBobSquarePants 👍 everything looks good to me...not spotting any other issues. :shipit:

tocsoft avatar Mar 16 '23 08:03 tocsoft

@JimBobSquarePants 👍 everything looks good to me...not spotting any other issues. :shipit:

I had one more trick up my sleeve. I'd spotted that rendering was a bit off vertically with smaller fonts and while fixing it realized we could only cache glyphs along a single line. With the last commit any glyph with matching bounds following the default transform can use a previous render. This makes a huge difference and brings us much closer to System.Drawing.

Given we don't have access to the fast blenders here I'm pretty happy!

BenchmarkDotNet=v0.13.1, OS=Windows 10.0.22621
11th Gen Intel Core i7-11370H 3.30GHz, 1 CPU, 8 logical and 4 physical cores
.NET SDK=7.0.201
  [Host]     : .NET Core 3.1.32 (CoreCLR 4.700.22.55902, CoreFX 4.700.22.56512), X64 RyuJIT
  DefaultJob : .NET Core 3.1.32 (CoreCLR 4.700.22.55902, CoreFX 4.700.22.56512), X64 RyuJIT


|                                         Method | TextIterations |       Mean |     Error |    StdDev | Ratio | RatioSD |      Gen 0 |     Gen 1 |    Gen 2 |     Allocated |
|----------------------------------------------- |--------------- |-----------:|----------:|----------:|------:|--------:|-----------:|----------:|---------:|--------------:|
|             'System.Drawing Draw Text Outline' |             10 |   2.918 ms | 0.0114 ms | 0.0101 ms |  1.00 |    0.00 |          - |         - |        - |         622 B |
| 'ImageSharp Draw Text Outline - Cached Glyphs' |             10 |   4.901 ms | 0.0267 ms | 0.0237 ms |  1.68 |    0.01 |   421.8750 |  140.6250 |        - |   2,669,828 B |
|         'ImageSharp Draw Text Outline - Naive' |             10 |  25.740 ms | 0.4913 ms | 0.5045 ms |  8.85 |    0.17 |  1000.0000 |         - |        - |  11,499,112 B |
|                                                |                |            |           |           |       |         |            |           |          |               |
|             'System.Drawing Draw Text Outline' |            100 |  26.539 ms | 0.3611 ms | 0.3016 ms |  1.00 |    0.00 |          - |         - |        - |       7,432 B |
| 'ImageSharp Draw Text Outline - Cached Glyphs' |            100 |  69.846 ms | 1.3791 ms | 2.1061 ms |  2.64 |    0.08 |  4714.2857 | 1857.1429 | 571.4286 |  26,412,187 B |
|         'ImageSharp Draw Text Outline - Naive' |            100 | 259.370 ms | 4.9783 ms | 6.9789 ms |  9.85 |    0.34 | 18500.0000 | 3000.0000 | 500.0000 | 115,457,552 B |

JimBobSquarePants avatar Mar 16 '23 14:03 JimBobSquarePants