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.

Sign up for GitHub

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

Jump to bottom

Introduce DxFontRenderData #9096

Merged
16 commits merged into from
Feb 18, 2021
Merged

Introduce DxFontRenderData #9096

16 commits merged into from
Feb 18, 2021

Conversation

skyline75489
Copy link
Collaborator

@skyline75489 skyline75489 commented Feb 10, 2021 •
edited by DHowett
Loading

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

skyline75489 commented Feb 10, 2021 •
edited
Loading

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'

image

@zadjii-msft 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
Copy link
Member

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
miniksa requested changes Feb 10, 2021
View reviewed changes
Copy link
Member

@miniksa miniksa left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
src/renderer/dx/CustomTextLayout.cpp Show resolved Hide resolved
@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 Feb 10, 2021
miniksa
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 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
miniksa
miniksa approved these changes Feb 16, 2021
View reviewed changes
Copy link
Member

@miniksa miniksa left a comment

Choose a reason for hiding this comment

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

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

skyline75489
skyline75489 commented Feb 16, 2021
View reviewed changes
src/renderer/base/lib/base.vcxproj
@@ -6,7 +6,7 @@
<RootNamespace>base</RootNamespace>
<ProjectName>RendererBase</ProjectName>
<TargetName>ConRenderBase</TargetName>
<ConfigurationType>StaticLibrary</ConfigurationType>
<ConfigurationType>StaticLibrary</ConfigurationType>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure. Will do

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

DHowett
DHowett requested changes Feb 17, 2021
View reviewed changes
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.

Only minor things!

src/renderer/dx/DxRenderer.cpp Outdated Show resolved Hide resolved
@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 Feb 17, 2021
skyline75489
skyline75489 commented Feb 18, 2021
View reviewed changes
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
Copy link
Collaborator Author

@skyline75489 skyline75489 Feb 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

😄 thanks

DHowett
DHowett approved these changes Feb 18, 2021
View reviewed changes
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.

Thanks!

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

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

ghost commented Feb 18, 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 eb34993 into microsoft:main Feb 18, 2021
@skyline75489 skyline75489 mentioned this pull request Feb 18, 2021
Merged
@skyline75489 skyline75489 mentioned this pull request May 6, 2021
Closed
@skyline75489 skyline75489 deleted the chesterliu/dev/font-render-data branch May 6, 2021 10:30
ghost pushed a commit that referenced this pull request Jun 22, 2021
@skyline75489
Introduce DxFontInfo (#9201)
85c485e 
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.
@skyline75489 skyline75489 mentioned this pull request Jun 23, 2021
Merged
ghost pushed a commit that referenced this pull request Jul 7, 2021
@skyline75489
Render SGR 1 ("intensity") as bold in the DX engine (#10498)
a507311 
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
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miniksa miniksa miniksa approved these changes

@DHowett DHowett DHowett approved these changes

Assignees

@DHowett DHowett

@miniksa miniksa

Labels
Area-Fonts Related to the font Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues AutoMerge Marked for automatic merge by the bot when requirements are met Product-Terminal The new Windows Terminal.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@skyline75489 @zadjii-msft @DHowett @miniksa