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

skyline75489 · 2021-06-23T00:52:44Z

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 · 2021-06-23T00:54:01Z

After first two gigantic refactoring PRs, this is it, ladies & gentlemen. It's finally happening:

skyline75489 · 2021-06-23T00:58:03Z

src/renderer/dx/CustomTextLayout.cpp

    DWRITE_FONT_STYLE style = _fontRenderData->DefaultFontStyle();
    const DWRITE_FONT_STRETCH stretch = _fontRenderData->DefaultFontStretch();

+    if (drawingContext->useBoldFont)
+    {
+        // TODO: "relative" bold?


Do we need to do this? I don't quite know. This was brought up in other discussions that I can't find at the moment. probably somewhere in #109. Basically what we gonna do when use choose "semi-bold" or "bold" as their default font? Do we choose a relative "bolder" font here?

#109 (comment) is probably the comment you're looking for?

Yea I believe so. Thanks.

Here's a joke that I just thought of:

When they go normal, we go bold.
When they go bold, we go extra-bold.

Michelle "Terminal" Obama

Just multiply by 1.75, cap it at 1000, and call it a day, perhaps. I run font weight 450 locally ("Retina" weight) rather than 400, so this would come out closer to Extra Bold than Bold, which seems reasonable to me. If the font doesn't support variable-width, let DirectWrite work out which weight instance to pick.

If it's helpful, CSS defines its own transformation for bold, but that doesn't allow for variable-weight fonts, it relies on their font rounding calculation to pick a "named" weight first, then enbolden it. I would expect DirectWrite to do that for us.

This is useful information. Thanks. And I believe DirectWrite can work out which weight instance to pick, even under those seemingly invalid situations.

I'm not sure what you mean by invalid. From what I can see, the API will reject invalid weights (<1, >999), and any other weight is valid, although if you're not using a variable-width font, DWrite must pick the nearest matching font variant.

Suggested change

// TODO: "relative" bold?

// TODO:GH#10577 "relative" bold?

skyline75489 · 2021-06-23T00:58:47Z

src/renderer/dx/CustomTextLayout.cpp

@@ -137,10 +137,16 @@ try
 {
    const auto drawingContext = static_cast<const DrawingContext*>(clientDrawingContext);

-    const DWRITE_FONT_WEIGHT weight = _fontRenderData->DefaultFontWeight();
+    DWRITE_FONT_WEIGHT weight = _fontRenderData->DefaultFontWeight();
    DWRITE_FONT_STYLE style = _fontRenderData->DefaultFontStyle();
    const DWRITE_FONT_STRETCH stretch = _fontRenderData->DefaultFontStretch();


Is there anything that can change the stretch property? Is there a SGR sequence for this? Or this is just gonna sit here not being used.

tl;dr: There's character spacing control in the Teletext and ECMA-48 specs, but to implement it usefully, we'd have to make this code depend on DirectWrite from Windows 10 builds 16299 or later.

Poking around the various standards...

T.416 and ECMA-48 document some CSI sequences for character spacing and character separation, but that's not really the same thing, as I suspect they expected extra space in the layout, not stretching the characters.

Select Character Spacing - SHS (CSI Pn <space>K) is probably the closest conceptually, since it lets you pick from a width enum 0..6, per ECMA-48. T.416 only includes 0..4, and measures them in BMU instead of millimeters, but with the same relative meanings. T.61 uses the same definition as ECMA-48, but notes that 3 is only for Chinese ideogram and Japanese Kanji terminals, and 5 and 6 are only for Chinese ideogram terminals, suggesting that they relate to things like the "full-width" latin character layout from Japanese fonts, i.e. a latin character stretched to take the same width as a kanji character and whatever the similar concept is in Chinese typography.

0: 10 characters per 25,4 mm DWRITE_FONT_STRETCH_NORMAL

1: 12 characters per 25,4 mm DWRITE_FONT_STRETCH_SEMI_CONDENSED

2: 15 characters per 25,4 mm DWRITE_FONT_STRETCH_EXTRA_CONDENSED

3: 6 characters per 25,4 mm DWRITE_FONT_STRETCH_EXTRA_EXPANDED

4: 3 characters per 25,4 mm DWRITE_FONT_STRETCH_ULTRA_EXPANDED

5: 9 characters per 50,8 mm DWRITE_FONT_STRETCH_ULTRA_EXPANDED

6: 4 characters per 25,4 mm DWRITE_FONT_STRETCH_ULTRA_EXPANDED

Anyway, I doubt it's ever used, and if it is, presumably that use-case will also be interested in Set Character Spacing - SCS which lets you specify the value rather than pick from an enum, and unlike width, we can't pass random values outside the DWRITE_FONT_STRETCH enum to DirectWrite here, so it'd have to be done with something like DWRITE_FONT_AXIS_VALUE's "Width" support.

Assuming 100 means the same as enum value 0 above, that might actually be feasible, but it's a newer version of DWrite (dwrite_3.h) than we're apparently using here, using IDWriteFontFamily2::GetMatchingFonts instead of IDWriteFontFamily::GetFirstMatchingFont, which needs Build 16299: Windows 10 Fall Creators Update.

But it does suggest that somewhere there's a mapping from the DWRITE_FONT_STRETCH values to the DWRITE_FONT_AXIS_VALUE range, if we do want to try and implement SHS at some far-far-far-far-future point.

Edit: There it is, in the OpenType spec. And the width tag docs say how to interpolate between enum and numeric width value, which is probably what font designers do anyway.

I updated the table from ECMA-48 with the stretch value, but it's not a wonderful result, if something relies on SHS for layout, particularly if they're expecting precise cells to line up, i.e. 2 characters at SHS 0 and 3 characters at SHS2 should be precisely the same width, that won't work using the enum matches above.

skyline75489 · 2021-06-23T01:00:33Z

CC @DHowett @miniksa @j4james

src/renderer/dx/CustomTextLayout.cpp

lhecker · 2021-06-23T23:36:56Z

src/renderer/dx/CustomTextLayout.cpp

+    if (drawingContext->useBoldFont)
+    {
+        // TODO: "relative" bold?
+        weight = DWRITE_FONT_WEIGHT_BOLD;


You could write something like this:

weight = (weight * DWRITE_FONT_WEIGHT_BOLD) / DWRITE_FONT_WEIGHT_NORMAL;

Yeah I could. Just I'm not sure if it's correct. Or is there a "correct" way to do this.

That's equivalent to what I suggested above (that's there the 1.75 came from), but today I suspect that font weight might not be perceptually linear. It'd work as a first pass, and clearly getting the rest of the framework in-place is important.

It also needs to cap at 999, so it's really

min(999, (weight * DWRITE_FONT_WEIGHT_BOLD) / DWRITE_FONT_WEIGHT_NORMAL)

If there's any background on the CSS font-boldening algorithm (i.e. how they determined it), it might give a better approach that works for variable-weight fonts as well. They might however have just been "given these four weights, just move up one to make it bold", which is probably not a good result for variable-width fonts.

A totally alternative approach is to let the user specify through config, and default to DWRITE_FONT_WEIGHT_BOLD, so if the user overrides the normal font weight, they either also override the bold font weight, or accept that they might have made their default font bolder than their bold font.

Actually, that seems like the best long-term result, and needs less magic, but can we live with hard-coded "bold is always 700" in the meantime?

It isn't unreasonable to "let the user specify through config", but it's certainly harder than what I intended for this PR. That would also require modification in the Settings UI. Maybe we can consider it in the long-term plan.

can we live with hard-coded "bold is always 700" in the meantime

I'm perfectly OK with it, TBH. Just like I'm OK with Quake mode without animations. But I'm worried that some other folks in the community might disagree.

I agree that "exposing the user config" shouldn't block this PR. We'll how those who must suffer the slings and arrows of outraged users feel about it. ^_^

I'm gonna say lets file this as a follow up discussion, and ship as-is for now.

I'm cool with shipping this as-is right now. Bold is bold. Makes sense. We can discuss more in #10577.

DHowett

Temporarily blocking so msftbot makes sure I sign it :)

skyline75489 · 2021-06-25T06:18:59Z

The msftbot can also merge draft PRs? Can anyone really merge draft PR? I don't honestly know...

Anyway, Hope we can come up with a good-enough solution and ship this in the 1.10 release 😄

DHowett · 2021-06-28T16:06:54Z

src/renderer/dx/DxRenderer.cpp

@@ -1961,6 +1961,7 @@ CATCH_RETURN()
    if (_drawingContext)
    {
        _drawingContext->forceGrayscaleAA = _ShouldForceGrayscaleAA();
+        _drawingContext->useBoldFont = textAttributes.IsBold();


This is going to be the most controversial change we ever make. 😄

I sincerely hope the controversy will not be a blocker for this PR, because, well, we want BOLD 😃

(some meme here, if I were Mike)

"To boldly go, where no font has bolded before" perhaps?

I don't think this will be a problem as long as we're planning to add an option for it. Not necessarily in this PR, but it's probably a good idea to have the option in place before this reaches a release build.

@j4james by “option” do you mean the “choose which font weight is bold” one, or “choose if you want bold as bright” one?

I mean choose if you want SGR 1 to select a bold font face or not (basically a way to disable this PR). I suppose you could potentially implement that by allowing users to choose which font weight is bold, and expect them to select the normal font weight, but that seems a bit obscure. For example, in XTerm you can just uncheck the "Bold Fonts" menu, and in Konsole you uncheck the option "Draw intense colors in bold font".

There are a whole bunch of other variations people may want to configure, e.g. disabling bold-as-bright, or only enabling a bold font when used with the extended colors (i.e. the aix bright colors and the ITU 256/RGB colors). Personally I think the latter is the best default config, since it gives you the standard behaviour for the original 8 SGR colors, while still allowing bold fonts when used with extended colors, but I'd be happy with a simple option to turn this off.

Thanks James for the explanation. I never really thought that people would not want bold font. Just like bracketed paste. I think seldom do people want to disable bracketed paste. But you are right. Bold seems like a more intrusive feature and some folks might not fancy it.

_{I was summoned, I had to}

#10576 filed to track adding this setting alongside this release.

zadjii-msft

frankly, the code for this is ridiculously simple.

zadjii-msft · 2021-07-07T16:04:34Z

src/renderer/dx/DxRenderer.cpp

@@ -1961,6 +1961,7 @@ CATCH_RETURN()
    if (_drawingContext)
    {
        _drawingContext->forceGrayscaleAA = _ShouldForceGrayscaleAA();
+        _drawingContext->useBoldFont = textAttributes.IsBold();


_{I was summoned, I had to}

zadjii-msft · 2021-07-07T16:06:58Z

src/renderer/dx/CustomTextLayout.cpp

+    if (drawingContext->useBoldFont)
+    {
+        // TODO: "relative" bold?
+        weight = DWRITE_FONT_WEIGHT_BOLD;


I'm gonna say lets file this as a follow up discussion, and ship as-is for now.

zadjii-msft · 2021-07-07T16:21:28Z

src/renderer/dx/CustomTextLayout.cpp

+    if (drawingContext->useBoldFont)
+    {
+        // TODO: "relative" bold?
+        weight = DWRITE_FONT_WEIGHT_BOLD;


I'm cool with shipping this as-is right now. Bold is bold. Makes sense. We can discuss more in #10577.

zadjii-msft · 2021-07-07T16:21:52Z

src/renderer/dx/CustomTextLayout.cpp

    DWRITE_FONT_STYLE style = _fontRenderData->DefaultFontStyle();
    const DWRITE_FONT_STRETCH stretch = _fontRenderData->DefaultFontStretch();

+    if (drawingContext->useBoldFont)
+    {
+        // TODO: "relative" bold?


Suggested change

// TODO: "relative" bold?

// TODO:GH#10577 "relative" bold?

zadjii-msft · 2021-07-07T16:22:31Z

src/renderer/dx/DxRenderer.cpp

@@ -1961,6 +1961,7 @@ CATCH_RETURN()
    if (_drawingContext)
    {
        _drawingContext->forceGrayscaleAA = _ShouldForceGrayscaleAA();
+        _drawingContext->useBoldFont = textAttributes.IsBold();


#10576 filed to track adding this setting alongside this release.

mdtauk · 2021-07-07T16:34:35Z

Can I just ask, how does this work when someone makes a choice for font-weight that is not Regular/Normal?

Does it always use the font's bold weight, or does it have a more fine grained approach so if it is on Light, bold uses Semi Light, or if the user has Bold as the default weight, does it move to Black?

DHowett · 2021-07-07T16:35:48Z

@mdtauk you will be very interested in discussion thread #10498 (comment) and the followup issue #10577

mdtauk · 2021-07-07T16:46:12Z

@mdtauk you will be very interested in discussion thread #10498 (comment) and the followup issue #10577

Ah good good, glad the thought has already occured. I posted some thoughts
#10577 (comment)

ghost · 2021-07-07T21:07:31Z

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 · 2021-07-14T17:08:14Z

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

Handy links:

sarim · 2021-07-18T16:40:07Z

Is there a setting to disable this behavior? It looks like it renders font as bold if its in certain color. I don't like this.

Edit: sorry for commenting without deep searching :( I see there's already work being done to create a setting.

Initial implementation for bold support in DxRenderer

265bdc7

skyline75489 commented Jun 23, 2021

View reviewed changes

skyline75489 mentioned this pull request Jun 23, 2021

Enable Bold Text in Windows Terminal #109

Closed

lhecker approved these changes Jun 23, 2021

View reviewed changes

DHowett requested changes Jun 23, 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 23, 2021

DHowett reviewed Jun 28, 2021

View reviewed changes

DHowett added a commit that referenced this pull request Jul 1, 2021

Pre-merge #10498 (bold)

b68f203

This was referenced Jul 7, 2021

Add a setting for disabling "intense is bold" #10576

Closed

Relatively "more bold" discussion #10577

Open

zadjii-msft approved these changes Jul 7, 2021

View reviewed changes

DHowett marked this pull request as ready for review July 7, 2021 20:41

DHowett changed the title ~~Initial implementation for bold support in DxRenderer~~ Render SGR 1 ("intensity") as bold in the DX engine Jul 7, 2021

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

DHowett approved these changes Jul 7, 2021

View reviewed changes

ghost merged commit a507311 into microsoft:main Jul 7, 2021

hltdev8642 mentioned this pull request Jul 16, 2021

windows terminal preview bold / cut-off fonts #10678

Closed

skyline75489 deleted the chesterliu/dev/bold-2021-edition branch July 19, 2021 01:34

This was referenced Jul 19, 2021

Wrong font for bold text in WSL #10709

Closed

Can I get bright without bold (as in a console)? #10758

Closed

This pull request was closed.

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

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

Conversation

skyline75489 commented Jun 23, 2021 • edited by DHowett Loading

skyline75489 commented Jun 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TBBle Jun 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TBBle Jun 23, 2021 • edited Loading

Choose a reason for hiding this comment

skyline75489 commented Jun 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TBBle Jun 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett left a comment

Choose a reason for hiding this comment

skyline75489 commented Jun 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

j4james Jun 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zadjii-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdtauk commented Jul 7, 2021

DHowett commented Jul 7, 2021 • edited Loading

mdtauk commented Jul 7, 2021

ghost commented Jul 7, 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.

ghost commented Jul 14, 2021

sarim commented Jul 18, 2021 • edited Loading

skyline75489 commented Jun 23, 2021 •

edited by DHowett

Loading

skyline75489 commented Jun 23, 2021 •

edited

Loading

TBBle Jun 23, 2021 •

edited

Loading

TBBle Jun 23, 2021 •

edited

Loading

TBBle Jun 24, 2021 •

edited

Loading

skyline75489 commented Jun 25, 2021 •

edited

Loading

j4james Jun 29, 2021 •

edited

Loading

DHowett commented Jul 7, 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.

sarim commented Jul 18, 2021 •

edited

Loading