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 UiFlags::KerningFitSpacing #7705

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

kphoenix137
Copy link
Collaborator

When using UiFlags::KerningFitSpacing, the text alignment was computed using unadjusted spacing. Later, during drawing, the spacing was reduced to "squish" the text so it fits inside its rect. This mismatch caused the final drawn text to be narrower than expected, resulting in left-shifted text that would have been word-wrapped without the use of UiFlags::KerningFitSpacing.

We now compute the adjusted spacing before calculating the starting X coordinate for text. The changes are as follows:

  • First, measure the text width using the default spacing.
  • Use AdjustSpacingToFitHorizontally to determine the new spacing that ensures the text fits within the available width.
  • Recalculate the line width using the adjusted spacing and compute the starting X position (with GetLineStartX) based on this new width.
  • Update the text render options with the adjusted spacing before passing them to the drawing routine, ensuring that both the alignment calculation and the drawing use the same spacing.

@glebm
Copy link
Collaborator

glebm commented Feb 3, 2025

In DoDrawString, we recalculate the kerning for every new line:

  1. Does that work correctly with AdjustSpacingToFitHorizontally?
  2. Can the new logic be moved there? We do most of the layout in that function.

Also, DrawStringWithColors has its own implementation. Does it need to be adjusted as well?

@kphoenix137
Copy link
Collaborator Author

kphoenix137 commented Feb 3, 2025

In DoDrawString, we recalculate the kerning for every new line:

1. Does that work correctly with `AdjustSpacingToFitHorizontally`?

2. Can the new logic be moved there? We do most of the layout in that function.

Also, DrawStringWithColors has its own implementation. Does it need to be adjusted as well?

I pushed a commit to move the fix directly into DoDrawString. This should work now without modifying DrawString or DrawStringWithColors. I'm not sure if there are redundancies or not within DrawStringWithColors due to the usage of AdjustSpacingToFitHorizontally being present. I attempted to remove the usage there, but ended up breaking things so I reverted to just the added logic within DoDrawString. With the current iteration, I viewed examples of all three instances of DrawStringWithColors being used, and they all appear to display correctly.

? AdjustSpacingToFitHorizontally(lineWidth, opts.spacing, charactersInLine, rect.size.width)
: opts.spacing;
int curSpacing = opts.spacing;
if (HasAnyOf(opts.flags, UiFlags::KerningFitSpacing)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is very similar code in DrawStringWithColors that also needs to be adjusted in the same way (search the file for int curSpacing = HasAnyOf)

Copy link
Collaborator

@glebm glebm Feb 5, 2025

Choose a reason for hiding this comment

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

Here (see bottom of the screenshot):
image

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.

2 participants