Introduce DxFontInfo by skyline75489 · Pull Request #9201 · microsoft/terminal

skyline75489 · 2021-02-18T10:17:39Z

This PR Introduces DxFontInfo to simplify the logic in
DxFontRenderData.

DxFontInfo aims to be the DWrite equivalent of FontInfo &
FontInfoBase in GDI. It encapsulates the needed information to
represent a displayable font face. It also provides the ability to
resolve a font face based on the available fonts on the system.

References

This is a follow-up of #9096.
Initial Italic support was introduced by #8580.

The motivation behind this is to support bold & bold-italic text in
Windows Terminal.

src/renderer/dx/DxFontRenderData.cpp

src/renderer/dx/CustomTextLayout.cpp

src/renderer/dx/DxFontInfo.h

skyline75489 · 2021-02-18T10:24:32Z

src/renderer/dx/DxFontInfo.h

+    public:
+        DxFontInfo() noexcept;
+
+        DxFontInfo(std::wstring_view familyName,


I'm not sure whether this should be familyName or faceName. Seems to me there are familyName & faceName in the GDI font info, but only familyName in DWrite?

There are face names on the IDWriteFont::GetFaceNames method... the problem tends to be that there are multiple of them depending on the locale or language or variations and GDI just sort of glosses over that. I think we should just continue to focus on the family name until proven otherwise.

src/renderer/dx/DxFontRenderData.cpp

src/renderer/dx/DxFontInfo.cpp

miniksa · 2021-04-14T17:52:06Z

src/renderer/dx/DxFontInfo.h

+    public:
+        DxFontInfo() noexcept;
+
+        DxFontInfo(std::wstring_view familyName,


There are face names on the IDWriteFont::GetFaceNames method... the problem tends to be that there are multiple of them depending on the locale or language or variations and GDI just sort of glosses over that. I think we should just continue to focus on the family name until proven otherwise.

src/renderer/dx/DxFontInfo.h

src/renderer/dx/DxFontRenderData.cpp

src/renderer/dx/CustomTextLayout.cpp

src/renderer/dx/DxFontInfo.cpp

src/renderer/dx/CustomTextLayout.cpp

src/renderer/dx/DxFontRenderData.cpp

skyline75489 · 2021-04-16T06:58:24Z

Huh...the trick [skip ci] does not seem to work with Azure Pipelines.

See microsoft/azure-pipelines-agent#1270 (comment) . I don't honestly know what she meant by saying "Azure DevOps honors the [skip ci] commands on CI builds as the change being built has already been merged"

PankajBhojwani

Some of the unit tests seem to be failing, other than that this looks good to me!

skyline75489 · 2021-06-15T04:14:19Z

OK I have no idea how & why there tests has anything to do with my code:

2021-06-11T16:59:23.2143730Z Summary of Non-passing Tests:
2021-06-11T16:59:23.2144447Z     ControlUnitTests::ControlCoreTests::TestInitialize [Failed]
2021-06-11T16:59:23.2145027Z     ControlUnitTests::ControlCoreTests::TestAdjustAcrylic [Failed]
2021-06-11T16:59:23.2145574Z     ControlUnitTests::ControlInteractivityTests::TestScrollWithMouse [Failed]
2021-06-11T16:59:23.2146125Z     ControlUnitTests::ControlInteractivityTests::CreateSubsequentSelectionWithDragging [Failed]
2021-06-11T16:59:23.2146693Z     ControlUnitTests::ControlInteractivityTests::ScrollWithSelection [Failed]
2021-06-11T16:59:23.2147256Z     ControlUnitTests::ControlInteractivityTests::TestScrollWithTrackpad [Failed]
2021-06-11T16:59:23.2147786Z     ControlUnitTests::ControlInteractivityTests::TestQuickDragOnSelect [Failed]

miniksa · 2021-06-15T18:43:50Z

OK I have no idea how & why there tests has anything to do with my code:

2021-06-11T16:59:23.2143730Z Summary of Non-passing Tests:
2021-06-11T16:59:23.2144447Z     ControlUnitTests::ControlCoreTests::TestInitialize [Failed]
2021-06-11T16:59:23.2145027Z     ControlUnitTests::ControlCoreTests::TestAdjustAcrylic [Failed]
2021-06-11T16:59:23.2145574Z     ControlUnitTests::ControlInteractivityTests::TestScrollWithMouse [Failed]
2021-06-11T16:59:23.2146125Z     ControlUnitTests::ControlInteractivityTests::CreateSubsequentSelectionWithDragging [Failed]
2021-06-11T16:59:23.2146693Z     ControlUnitTests::ControlInteractivityTests::ScrollWithSelection [Failed]
2021-06-11T16:59:23.2147256Z     ControlUnitTests::ControlInteractivityTests::TestScrollWithTrackpad [Failed]
2021-06-11T16:59:23.2147786Z     ControlUnitTests::ControlInteractivityTests::TestQuickDragOnSelect [Failed]

The failed tests all look like

2021-06-15T11:27:11.3398459Z TAEF: A crash with exception code 0xC0000094 occurred in module "Control.Unit.Tests.dll" in process "te.processhost.exe" (pid:6860).

Where 0xC0000094 is a thrown divide by zero exception. So I'm guessing something about the setup part of those tests is initializing something that is now having a divide by zero with your change. :(

TBBle · 2021-06-16T14:01:18Z

The common thing in the unit tests looks like a call to ControlCore::Initialize, which calls ControlCore::_updateFont and hence Renderer::TriggerFontChange, which leads to DxEngine::UpdateFont, and hence DxFontRenderData::UpdateFont. This calls DxFontRenderData::_BuildFontRenderData, which is introduced in this patch.

DxFontRenderData::_BuildFontRenderData has lots of division calls in it as well, although at least some of them were already in the code-base, so this might be a red herring.

So at a guess, there's some code-path where some initialisation never happens and hence some value is 0.f. It's being hit by the unit tests' very minimal initialisation, that you aren't seeing in actual testing when you run WT.

skyline75489 · 2021-06-17T02:38:02Z

wow thanks @TBBle for the investigation. I'll get to this PR later and found out why. I promise.

src/cascadia/UnitTests_Control/MockControlSettings.h

PankajBhojwani

Thanks!

DHowett

blocking while reviewing since this has 2 s/o

DHowett · 2021-06-21T18:25:58Z

src/renderer/dx/DxFontInfo.h

+        bool _didFallback;
+    };
+
+    struct DxFontInfoHash


My personal preference is that we properly extend std::hash; it means that we can use the STL containers with less customization, and perhaps that leads to a higher likelihood that they will be deduplicated as "identical code" 😄

ghost · 2021-06-22T19:31:24Z

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.

This commit adds support for bold text in DxRenderer. For now, bold fonts are always rendered using DWRITE_FONT_WEIGHT_BOLD regardless of the base weight. As yet, this behavior is unconfigurable. References Previous refactoring PRs: #9096 (DxFontRenderData) #9201 (DxFontInfo) SGR support tracking issue: #6879 Closes #109

Fixes the performance regression caused by DxFontInfo. DxFontInfo introduced in #9201

ghost · 2021-07-14T17:08:11Z

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

Handy links:

Introduce DxFontInfo

eaf86b0

skyline75489 commented Feb 18, 2021

View reviewed changes

Format

b1fc5be

zadjii-msft assigned miniksa Feb 18, 2021

zadjii-msft added the Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues label Feb 18, 2021

skyline75489 requested a review from DHowett February 19, 2021 03:25

skyline75489 added 2 commits March 2, 2021 13:11

Merge branch 'main' into chesterliu/dev/init-dxfontinfo

6b748f8

Merge branch 'main' into chesterliu/dev/init-dxfontinfo

bf1da37

skyline75489 mentioned this pull request Apr 8, 2021

Reduce instances of font fallback dialog #9734

Merged

5 tasks

skyline75489 added 3 commits April 8, 2021 15:28

Merge branch 'main' into chesterliu/dev/init-dxfontinfo

b2b548b

Merge branch 'main' into chesterliu/dev/init-dxfontinfo

a877bac

clean

8456d81

miniksa requested changes Apr 14, 2021

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Apr 14, 2021

Merge branch 'main' into chesterliu/dev/init-dxfontinfo

6a61d45

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Apr 15, 2021

skyline75489 commented Apr 15, 2021

View reviewed changes

src/renderer/dx/DxFontInfo.cpp Show resolved Hide resolved

skyline75489 commented Apr 15, 2021

View reviewed changes

src/renderer/dx/DxFontInfo.cpp Show resolved Hide resolved

Feedback

4f5e4fc

skyline75489 commented Apr 15, 2021

View reviewed changes

src/renderer/dx/CustomTextLayout.cpp Outdated Show resolved Hide resolved

skyline75489 commented Apr 15, 2021

View reviewed changes

src/renderer/dx/DxFontRenderData.cpp Show resolved Hide resolved

miniksa approved these changes Apr 15, 2021

View reviewed changes

[skip ci] private method naming

04bbcda

skyline75489 added the Product-Terminal The new Windows Terminal. label Apr 16, 2021

miniksa added the Needs-Second It's a PR that needs another sign-off label Apr 16, 2021

ghost requested review from PankajBhojwani, carlos-zamora, leonMSFT and zadjii-msft April 16, 2021 15:51

skyline75489 added 2 commits June 1, 2021 17:58

Merge branch 'main' into chesterliu/dev/init-dxfontinfo

789d33c

Fix merge

c4ab9b5

PankajBhojwani requested changes Jun 11, 2021

View reviewed changes

ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Jun 11, 2021

Merge branch 'main' into chesterliu/dev/init-dxfontinfo

03e56df

skyline75489 added 2 commits June 18, 2021 19:01

Fix ControlUnitTests

26a489b

Merge branch 'main' into chesterliu/dev/init-dxfontinfo

2f6c091

TBBle reviewed Jun 18, 2021

View reviewed changes

src/cascadia/UnitTests_Control/MockControlSettings.h Outdated Show resolved Hide resolved

Handle first font resolve attempt correctly

a59757a

PankajBhojwani approved these changes Jun 21, 2021

View reviewed changes

DHowett requested changes Jun 21, 2021

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jun 21, 2021

Feedback

806eef4

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jun 22, 2021

DHowett approved these changes Jun 22, 2021

View reviewed changes

DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jun 22, 2021

ghost merged commit 85c485e into microsoft:main Jun 22, 2021

This was referenced Jun 23, 2021

Render SGR 1 ("intensity") as bold in the DX engine #10498

Merged

Add fast lookup path for DxFontInfo #10521

Merged

skyline75489 deleted the chesterliu/dev/init-dxfontinfo branch July 3, 2021 02:47

ghost pushed a commit that referenced this pull request Jul 9, 2021

Add fast lookup path for DxFontInfo (#10521)

f339705

Fixes the performance regression caused by DxFontInfo. DxFontInfo introduced in #9201

hltdev8642 mentioned this pull request Jul 16, 2021

windows terminal preview bold / cut-off fonts #10678

Closed

This pull request was closed.

Conversation

skyline75489 commented Feb 18, 2021 • edited by DHowett Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

References

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

skyline75489 Feb 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miniksa Apr 14, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

miniksa Apr 14, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

skyline75489 commented Apr 16, 2021

Uh oh!

PankajBhojwani left a comment

Choose a reason for hiding this comment

Uh oh!

skyline75489 commented Jun 15, 2021

Uh oh!

miniksa commented Jun 15, 2021

Uh oh!

TBBle commented Jun 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skyline75489 commented Jun 17, 2021

Uh oh!

Uh oh!

PankajBhojwani left a comment

Choose a reason for hiding this comment

Uh oh!

DHowett left a comment

Choose a reason for hiding this comment

Uh oh!

DHowett Jun 21, 2021

Choose a reason for hiding this comment

Uh oh!

ghost commented Jun 22, 2021

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.

Uh oh!

ghost commented Jul 14, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

skyline75489 commented Feb 18, 2021 •

edited by DHowett

Loading

skyline75489 Feb 18, 2021 •

edited

Loading

TBBle commented Jun 16, 2021 •

edited

Loading

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.