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

Introduce DxFontInfo #9201

Merged
21 commits merged into from
Jun 22, 2021
Merged

Conversation

skyline75489
Copy link
Collaborator

@skyline75489 skyline75489 commented Feb 18, 2021

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 Show resolved Hide resolved
src/renderer/dx/DxFontRenderData.cpp Outdated Show resolved Hide resolved
src/renderer/dx/CustomTextLayout.cpp Show resolved Hide resolved
src/renderer/dx/CustomTextLayout.cpp Outdated Show resolved Hide resolved
src/renderer/dx/DxFontInfo.h Show resolved Hide resolved
public:
DxFontInfo() noexcept;

DxFontInfo(std::wstring_view familyName,
Copy link
Collaborator Author

@skyline75489 skyline75489 Feb 18, 2021

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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 Show resolved Hide resolved
@zadjii-msft zadjii-msft added the Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues label Feb 18, 2021
@skyline75489 skyline75489 requested a review from DHowett February 19, 2021 03:25
src/renderer/dx/DxFontInfo.cpp Outdated Show resolved Hide resolved
src/renderer/dx/DxFontInfo.cpp Outdated Show resolved Hide resolved
src/renderer/dx/DxFontInfo.cpp Outdated Show resolved Hide resolved
public:
DxFontInfo() noexcept;

DxFontInfo(std::wstring_view familyName,
Copy link
Member

Choose a reason for hiding this comment

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

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 Show resolved Hide resolved
src/renderer/dx/DxFontRenderData.cpp Show resolved Hide resolved
src/renderer/dx/DxFontRenderData.cpp Outdated Show resolved Hide resolved
src/renderer/dx/DxFontRenderData.cpp Show resolved Hide resolved
src/renderer/dx/CustomTextLayout.cpp Show resolved Hide resolved
@ghost 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
@ghost 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
Copy link
Collaborator Author

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"

@skyline75489 skyline75489 added the Product-Terminal The new Windows Terminal. label Apr 16, 2021
@miniksa miniksa added the Needs-Second It's a PR that needs another sign-off label Apr 16, 2021
Copy link
Contributor

@PankajBhojwani PankajBhojwani left a comment

Choose a reason for hiding this comment

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

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

@ghost 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
@skyline75489
Copy link
Collaborator Author

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
Copy link
Member

miniksa commented Jun 15, 2021

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
Copy link

TBBle commented Jun 16, 2021

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
Copy link
Collaborator Author

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

Copy link
Contributor

@PankajBhojwani PankajBhojwani left a comment

Choose a reason for hiding this comment

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

Thanks!

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.

blocking while reviewing since this has 2 s/o

bool _didFallback;
};

struct DxFontInfoHash
Copy link
Member

Choose a reason for hiding this comment

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

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 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
@ghost 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 DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jun 22, 2021
@ghost
Copy link

ghost commented Jun 22, 2021

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.

@ghost ghost merged commit 85c485e into microsoft:main Jun 22, 2021
@skyline75489 skyline75489 deleted the chesterliu/dev/init-dxfontinfo branch July 3, 2021 02:47
ghost pushed a commit that referenced this pull request Jul 7, 2021
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
ghost pushed a commit that referenced this pull request Jul 9, 2021
Fixes the performance regression caused by DxFontInfo.

DxFontInfo introduced in #9201
@ghost
Copy link

ghost commented Jul 14, 2021

🎉Windows Terminal Preview v1.10.1933.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-Rendering Text rendering, emoji, complex glyph & font-fallback issues AutoMerge Marked for automatic merge by the bot when requirements are met Needs-Second It's a PR that needs another sign-off Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants