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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support Vertical Mixed Text #271

Merged
merged 7 commits into from
May 5, 2023
Merged

Support Vertical Mixed Text #271

merged 7 commits into from
May 5, 2023

Conversation

JimBobSquarePants
Copy link
Member

@JimBobSquarePants JimBobSquarePants commented May 4, 2023

Prerequisites

  • I have written a descriptive pull-request title
  • 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 馃懏.
  • I have provided test coverage for my change (where applicable)

Description

Updates the glyph renderers to handle support for Unicode Standard Annex #50 Unicode Vertical Text Layout

Note: I've discovered an issue when rendering the decorations over certain CFF fonts in that holes are left, however I'm unable to determine what the cause could be. The paths themselves look ok.

This only affects filling a glyph collection.

@codecov
Copy link

codecov bot commented May 4, 2023

Codecov Report

Merging #271 (dbb1ef6) into main (f72bd7a) will increase coverage by 0%.
The diff coverage is 90%.

@@         Coverage Diff         @@
##           main   #271   +/-   ##
===================================
  Coverage    78%    79%           
===================================
  Files        97     97           
  Lines      5065   5088   +23     
  Branches    932    945   +13     
===================================
+ Hits       3983   4025   +42     
+ Misses      898    875   -23     
- Partials    184    188    +4     
Flag Coverage 螖
unittests 79% <90%> (+<1%) 猬嗭笍

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

Impacted Files Coverage 螖
...ImageSharp.Drawing/Shapes/Text/BaseGlyphBuilder.cs 88% <77%> (+31%) 猬嗭笍
...rocessing/Processors/Text/RichTextGlyphRenderer.cs 95% <100%> (+<1%) 猬嗭笍

... and 1 file with indirect coverage changes

馃摚 We鈥檙e building smart automated test selection to slash your CI/CD build times. Learn more


// Drawing is always centered around the point so we need to offset by half.
Vector2 offset = Vector2.Zero;
if (textDecorations == TextDecorations.Overline)
{
// CSS overline is drawn above the position, so we need to move it up.
offset = new(0, -(thickness * .5F));
offset = rotated ? new(thickness * .5F, 0) : new(0, -(thickness * .5F));
Copy link
Member

Choose a reason for hiding this comment

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

do we still need these offsets?, now we seem to also be applying them above?

Reason I ask is looking at the onion skin of the changed test images shows the decorations have shifted and are less sharp (i.e. they look like they are .5 of a pixel off and thus aliased edges are happening)

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'm rereading the code and it does appear that we need this. Overlines and underlines are drawn above and below the point while strike-through is centered on the point.

The comment in the method is actually incorrect which I'll update to .

// Expand the points to create a rectangle centered around the line.

The onion skin aliasing you are seeing is actually for the output of the RichTextGlyphRenderer. I removed the pixel clamping in when rendering decorations as I was seeing issues.

Take for example a horizontal 1-pixel thick line. For the rasterizer to draw that with a crisp output it would have to be positioned at exactly halfway vertically within a pixel.

For a 2-pixel thick line you have to be positioned at whole pixel bounds.

If we limit decoration thicknesses to whole pixels then we could work out the correct offset but if we don't then it gets really tricky.

I thought the simplest approach was to simply avoid clamping in favour of accuracy and let the rasterizer do the work.

Copy link
Member

@tocsoft tocsoft May 5, 2023

Choose a reason for hiding this comment

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

fair enough...just figured i'd check. I'm sure we can circle back round to the minor visual differences, and as you say try and tackle that at the rasterizer level, which will improve things across the board not just for test rendering.

if its deemed worth the effort.

tocsoft
tocsoft previously approved these changes May 5, 2023

// Drawing is always centered around the point so we need to offset by half.
Vector2 offset = Vector2.Zero;
if (textDecorations == TextDecorations.Overline)
{
// CSS overline is drawn above the position, so we need to move it up.
offset = new(0, -(thickness * .5F));
offset = rotated ? new(thickness * .5F, 0) : new(0, -(thickness * .5F));
Copy link
Member

@tocsoft tocsoft May 5, 2023

Choose a reason for hiding this comment

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

fair enough...just figured i'd check. I'm sure we can circle back round to the minor visual differences, and as you say try and tackle that at the rasterizer level, which will improve things across the board not just for test rendering.

if its deemed worth the effort.

@JimBobSquarePants
Copy link
Member Author

@tocsoft I couldn't help myself and implemented the smart pixel clamping I was on about in a manner that allowed preserving defined pen thickness. The results look great 馃榿

Copy link
Member

@tocsoft tocsoft left a comment

Choose a reason for hiding this comment

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

looks way better all around 馃憤

@JimBobSquarePants JimBobSquarePants merged commit 7b19f39 into main May 5, 2023
9 checks passed
@JimBobSquarePants JimBobSquarePants deleted the js/vertical-mixed branch May 5, 2023 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants