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

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

Merged
1 commit merged into from
Jul 7, 2021
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion src/renderer/dx/CustomTextLayout.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
lhecker marked this conversation as resolved.
Show resolved Hide resolved
DWRITE_FONT_STYLE style = _fontRenderData->DefaultFontStyle();
const DWRITE_FONT_STRETCH stretch = _fontRenderData->DefaultFontStretch();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link

@TBBle TBBle Jun 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.


if (drawingContext->useBoldFont)
{
// TODO: "relative" bold?
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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?

Copy link

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link

@TBBle TBBle Jun 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

weight = DWRITE_FONT_WEIGHT_BOLD;
Copy link
Member

Choose a reason for hiding this comment

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

You could write something like this:

weight = (weight * DWRITE_FONT_WEIGHT_BOLD) / DWRITE_FONT_WEIGHT_NORMAL;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link

@TBBle TBBle Jun 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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?

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

Copy link

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

}

if (drawingContext->useItalicFont)
{
style = DWRITE_FONT_STYLE_ITALIC;
Expand Down
2 changes: 2 additions & 0 deletions src/renderer/dx/CustomTextRenderer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ namespace Microsoft::Console::Render
renderTarget(renderTarget),
foregroundBrush(foregroundBrush),
backgroundBrush(backgroundBrush),
useBoldFont(false),
useItalicFont(false),
forceGrayscaleAA(forceGrayscaleAA),
dwriteFactory(dwriteFactory),
Expand All @@ -38,6 +39,7 @@ namespace Microsoft::Console::Render
ID2D1RenderTarget* renderTarget;
ID2D1SolidColorBrush* foregroundBrush;
ID2D1SolidColorBrush* backgroundBrush;
bool useBoldFont;
bool useItalicFont;
bool forceGrayscaleAA;
IDWriteFactory* dwriteFactory;
Expand Down
1 change: 1 addition & 0 deletions src/renderer/dx/DxRenderer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1961,6 +1961,7 @@ CATCH_RETURN()
if (_drawingContext)
{
_drawingContext->forceGrayscaleAA = _ShouldForceGrayscaleAA();
_drawingContext->useBoldFont = textAttributes.IsBold();
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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)

Copy link

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@j4james j4james Jun 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

image

I was summoned, I had to

Copy link
Member

Choose a reason for hiding this comment

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

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

_drawingContext->useItalicFont = textAttributes.IsItalic();
}

Expand Down