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

Fix vertical alignment of non-vertical glyphs #435

Merged
merged 1 commit into from
Dec 18, 2024

Conversation

JimBobSquarePants
Copy link
Member

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

When I removed the offsetting of vertical glyphs during #432 I forgot about non-vertical glyphs. This PR reintroduces the behavior in a more correct manner.

Note. It's really hard to write unit tests for this stuff. I think we need to introduce some regression tests in the form of image comparison using ImageSharp.Drawing to render the result.

Rendered output

CanDrawTextVertical2_Rgba32_Blank48x935 CanDrawTextVerticalMixed2_Rgba32_Blank48x839

Choose a reason for hiding this comment

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

Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

src/SixLabors.Fonts/TextLayout.cs:541

  • [nitpick] The variable name alignX is ambiguous. It should be renamed to horizontalAlignmentOffset.
float alignX = 0;
@JimBobSquarePants JimBobSquarePants merged commit e8d71d4 into main Dec 18, 2024
15 checks passed
@JimBobSquarePants JimBobSquarePants deleted the js/fix-vertical-layout branch December 18, 2024 01:39
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.

1 participant