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

AtlasEngine: Block chars done right-er #14099

Merged
3 commits merged into from
Oct 17, 2022
Merged

Conversation

lhecker
Copy link
Member

@lhecker lhecker commented Sep 29, 2022

This commit makes the following improvements:

  • Only adjust block characters that come from fallback fonts. This ensures
    that the glyphs of the chosen font all look exactly as they were designed.
  • When adjusting the size, use the fallback font's full block glyph U+2588
    to determine the size that the given glyph should have.

Closes #14098

Validation Steps Performed

  • Print UTF-8-demo.txt in Consolas.
  • All block glyphs look uniform. ✅

@ghost ghost added Area-AtlasEngine Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal. labels Sep 29, 2022
@lhecker lhecker marked this pull request as draft September 29, 2022 01:34
@lhecker lhecker marked this pull request as ready for review October 5, 2022 12:10
@lhecker
Copy link
Member Author

lhecker commented Oct 11, 2022

before:
before

after:
after

Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

I once again cannot profess to understand the math changes without scrutinizing them, but I will "go with it"

@DHowett DHowett added AutoMerge Marked for automatic merge by the bot when requirements are met Needs-Second It's a PR that needs another sign-off labels Oct 11, 2022
@ghost
Copy link

ghost commented Oct 11, 2022

Hello @DHowett!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@lhecker
Copy link
Member Author

lhecker commented Oct 11, 2022

I once again cannot profess to understand the math changes without scrutinizing them, but I will "go with it"

The DWRITE_OVERHANG_METRICS math is difficult to understand. I wrote it by picking random fonts and sizes and glyphs and painstakingly comparing my own calculations against what GetOverhangMetrics returns until it all matched up perfectly up to the last float digit. To be honest, I don't think there's any comment that can sufficiently explain that code - it simply is confusing, because it is confusing.

If some other code is difficult to understand let me know and I'll try to add some better comments!
BTW the _r.fontFaces matching is extremely bodgy and depends on the existence of the so called ActiveFontFace class (you can look it up internally). I asked the DWrite folks if we can depend on it, but I haven't heard back yet. Basically, my argument for depending on bodgy behavior is because the new-new AtlasEngine needs to hash font-face/glyph-index tuples and you can't do that if you can't hash font faces. Windows 11 22H1 now got a IDWriteFontFace5::Equals method, but as I put it: "you can't hash an equals() method". ActiveFaceCache on the other hand has continuously existed since 2011.

@ghost ghost merged commit 97abc3d into main Oct 17, 2022
@ghost ghost deleted the dev/lhecker/14098-block-drawings branch October 17, 2022 17:25
DHowett pushed a commit that referenced this pull request Dec 1, 2022
This commit makes the following improvements:
* Only adjust block characters that come from fallback fonts. This ensures
  that the glyphs of the chosen font all look exactly as they were designed.
* When adjusting the size, use the fallback font's full block glyph U+2588
  to determine the size that the given glyph should have.

Closes #14098

## Validation Steps Performed
* Print `UTF-8-demo.txt` in Consolas.
* All block glyphs look uniform. ✅

(cherry picked from commit 97abc3d)
Service-Card-Id: 86159056
Service-Version: 1.16
@ghost
Copy link

ghost commented Dec 14, 2022

🎉Windows Terminal Preview v1.16.3463.0 and v1.16.3464.0 has been released which incorporates this pull request.:tada:

Handy links:

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-AtlasEngine AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Second It's a PR that needs another sign-off Priority-3 A description (P3) Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Block Elements not rendering correctly with AtlasEngine enabled
3 participants