Introduce DxFontRenderData by skyline75489 · Pull Request #9096 · microsoft/terminal

skyline75489 · 2021-02-10T10:01:28Z

This is my attempt to isolate all the dwrite font related thing by
introducing a new layer - DxFontRenderData. This will free
DxRenderer & CustomTextLayout from the burden of handling fonts &
box effects. The logic is more simplified & streamlined.

In short I just moved everything fonts-related into DxFontRenderData
and started from there. There's no modification to code logic. Just pure
structural stuff.

SGR support tracking issue: #6879
Initial Italic support PR: #8580

skyline75489 · 2021-02-10T10:03:32Z

The motivation behind this is to get bold & bold-italic to work. I have a working branch. Will be open new PR after this is merged.

echo -e 'Normal, \x1b[1mbold\x1b[22m, \x1b[3mitalic\x1b[23m, \x1b[1;3mbold italic\x1b[22;23m'

zadjii-msft · 2021-02-10T11:11:28Z

I'm putting Michael and Dustin on this one - Michael since he's the rendering expert, and Dustin because I know he had some other thoughts on how to deal with implementing bold.

miniksa

Very excited by this. There were SO MANY PARAMETERS being passed around for this font information and it just kept getting bigger and bigger.

I just had a few small things to discuss/address before signoff.

Thanks!

src/renderer/dx/DxFontRenderData.cpp

src/renderer/dx/CustomTextLayout.cpp

src/renderer/dx/DxFontRenderData.cpp

src/renderer/dx/DxFontRenderData.h

src/renderer/dx/DxFontRenderData.cpp

src/renderer/dx/DxRenderer.cpp

src/renderer/dx/DxFontRenderData.cpp

src/renderer/dx/CustomTextLayout.cpp

src/renderer/dx/DxFontRenderData.cpp

src/renderer/dx/DxFontRenderData.h

src/renderer/dx/DxRenderer.cpp

src/renderer/dx/DxFontRenderData.cpp

src/renderer/dx/CustomTextLayout.cpp

src/renderer/dx/DxFontRenderData.cpp

src/renderer/dx/DxFontRenderData.h

src/renderer/dx/DxRenderer.cpp

src/renderer/dx/DxFontRenderData.cpp

miniksa

I think I'm good with this now. Thanks!

skyline75489 · 2021-02-16T23:54:07Z

src/renderer/base/lib/base.vcxproj

    <ProjectName>RendererBase</ProjectName>
    <TargetName>ConRenderBase</TargetName>
-    <ConfigurationType>StaticLibrary</ConfigurationType>
+    <ConfigurationType>StaticLibrary</ConfigurationType>


If you’re wondering why this is modified, originally I added an “interface” class here but then found it useless. VS modified the line ending anyway

Since this is the only change in the file, would you mind reverting it?

Sure. Will do

It took a while for me to realize I'm actually changing the LF here to CRLF, to make it 'correct'. Do you still want me to revert this?...

Oh. Since this makes it correct, you can keep it.

src/renderer/dx/lib/dx.vcxproj

DHowett

Only minor things!

src/renderer/dx/DxRenderer.cpp

skyline75489 · 2021-02-18T03:01:06Z

src/renderer/dx/DxFontRenderData.cpp

+// - dpi - The DPI of the screen
+// Return Value:
+// - S_OK or relevant DirectX error
+[[nodiscard]] HRESULT DxFontRenderData::UpdateFont(const FontInfoDesired& desired, FontInfo& actual, const int dpi) noexcept


This monstrous method will be refactored in my following PRs, which will introduce DxFontInfo and clear the logic in DxFontRenderData. @DHowett Probably not what you initially expect ("cascading FontInfo", See #8580 (comment) ) but I'll try my best.

😄 thanks

DHowett

Thanks!

note: i brought up the sources thing because technically the dx renderer still works in conhost ... sometimes. 😄

ghost · 2021-02-18T06:11:35Z

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 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.

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

skyline75489 added 3 commits February 10, 2021 10:02

Initialize IFontRenderData & DxFontRenderData

bc6ab94

vcxprojc

1440ae7

Refactor

ba43e8c

skyline75489 requested a review from miniksa February 10, 2021 10:04

zadjii-msft requested a review from DHowett February 10, 2021 11:10

zadjii-msft added Area-Fonts Related to the font Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Product-Terminal The new Windows Terminal. labels Feb 10, 2021

zadjii-msft assigned DHowett and miniksa Feb 10, 2021

miniksa requested changes Feb 10, 2021

View reviewed changes

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

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

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 Feb 10, 2021

Feedback

9a7413e

skyline75489 commented Feb 11, 2021

View reviewed changes

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

skyline75489 commented Feb 11, 2021

View reviewed changes

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

skyline75489 commented Feb 11, 2021

View reviewed changes

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

Format

5b65094

skyline75489 commented Feb 11, 2021

View reviewed changes

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

skyline75489 commented Feb 11, 2021

View reviewed changes

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

skyline75489 added 2 commits February 11, 2021 19:20

Unused

c9a43be

Feedback

36e7463

skyline75489 commented Feb 12, 2021

View reviewed changes

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

skyline75489 commented Feb 12, 2021

View reviewed changes

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

skyline75489 commented Feb 12, 2021

View reviewed changes

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

skyline75489 commented Feb 12, 2021

View reviewed changes

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

skyline75489 commented Feb 12, 2021

View reviewed changes

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

miniksa requested changes Feb 12, 2021

View reviewed changes

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

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

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

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

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 Feb 12, 2021

skyline75489 added 3 commits February 13, 2021 08:29

Merge branch 'main' into chesterliu/dev/font-render-data

d651886

Fix locale name

3a44c44

Hide UserLocaleName

149951b

skyline75489 commented Feb 13, 2021

View reviewed changes

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

skyline75489 added 2 commits February 13, 2021 08:54

Clean

11ecfa6

Format

d414f1e

miniksa approved these changes Feb 16, 2021

View reviewed changes

skyline75489 commented Feb 16, 2021

View reviewed changes

DHowett reviewed Feb 17, 2021

View reviewed changes

src/renderer/dx/lib/dx.vcxproj Show resolved Hide resolved

DHowett requested changes Feb 17, 2021

View reviewed changes

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

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 Feb 17, 2021

skyline75489 added 3 commits February 18, 2021 10:53

Feedback

6e2debb

Merge branch 'main' into chesterliu/dev/font-render-data

e7b7550

Naming

8521023

skyline75489 commented Feb 18, 2021

View reviewed changes

Damn, what is it..

afba98d

DHowett approved these changes Feb 18, 2021

View reviewed changes

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

ghost merged commit eb34993 into microsoft:main Feb 18, 2021

skyline75489 mentioned this pull request Feb 18, 2021

Introduce DxFontInfo #9201

Merged

skyline75489 mentioned this pull request May 6, 2021

Enable Bold Text in Windows Terminal #109

Closed

skyline75489 deleted the chesterliu/dev/font-render-data branch May 6, 2021 10:30

skyline75489 mentioned this pull request Jun 23, 2021

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

Merged

This pull request was closed.

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

zadjii-msft commented Feb 10, 2021

Uh oh!

miniksa left a comment

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

miniksa left a comment

Choose a reason for hiding this comment

Uh oh!

skyline75489 Feb 16, 2021

Choose a reason for hiding this comment

Uh oh!

DHowett Feb 17, 2021

Choose a reason for hiding this comment

Uh oh!

skyline75489 Feb 17, 2021

Choose a reason for hiding this comment

Uh oh!

skyline75489 Feb 18, 2021

Choose a reason for hiding this comment

Uh oh!

DHowett Feb 18, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DHowett left a comment

Choose a reason for hiding this comment

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!

DHowett Feb 18, 2021

Choose a reason for hiding this comment

Uh oh!

DHowett left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Feb 18, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

skyline75489 commented Feb 10, 2021 •

edited by DHowett

Loading

skyline75489 commented Feb 10, 2021 •

edited

Loading

skyline75489 Feb 18, 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.