Skip to content

AtlasEngine: Block chars done right-er#14099

Merged
3 commits merged intomainfrom
dev/lhecker/14098-block-drawings
Oct 17, 2022
Merged

AtlasEngine: Block chars done right-er#14099
3 commits merged intomainfrom
dev/lhecker/14098-block-drawings

Conversation

@lhecker
Copy link
Copy Markdown
Member

@lhecker lhecker commented Sep 29, 2022 •
edited
Loading

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
carlos-zamora added a commit that referenced this pull request Oct 8, 2022
@carlos-zamora
PRE-MERGE #14099 AtlasEngine: Block chars done right-er
83ffb40
carlos-zamora added a commit that referenced this pull request Oct 11, 2022
@carlos-zamora
PRE-MERGE #14099 AtlasEngine: Block chars done right-er
e135190
@lhecker
Copy link
Copy Markdown
Member Author

lhecker commented Oct 11, 2022

before:
before

after:
after

DHowett
DHowett approved these changes Oct 11, 2022
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown
Member Author

lhecker commented Oct 11, 2022 •
edited
Loading

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.

@lhecker lhecker mentioned this pull request Oct 12, 2022
Open
@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
@lhecker @DHowett
AtlasEngine: Block chars done right-er (#14099)
cd367e6 
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
Copy Markdown

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:

@ghost ghost mentioned this pull request Dec 14, 2022
Closed
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@DHowett DHowett DHowett approved these changes

@carlos-zamora carlos-zamora carlos-zamora approved these changes

@zadjii-msft zadjii-msft Awaiting requested review from zadjii-msft

@PankajBhojwani PankajBhojwani Awaiting requested review from PankajBhojwani

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Block Elements not rendering correctly with AtlasEngine enabled

3 participants

@lhecker @DHowett @carlos-zamora