Add support for the "italic" graphic rendition attribute #8580

j4james · 2020-12-13T23:53:18Z

This PR adds support for the ANSI italic graphic rendition attribute,
which is enabled by the SGR 3 escape sequence.

For the GDI renderer, I've just created an additional italic variant of
the font, and then the UpdateDrawingBrushes method selects the
appropriate font variant into the device context based on the requested
text attributes.

It's a bit more complicated in the DX renderer, because we need both an
italic variant of the font, and a variant of the text format object. The
CustomTextLayout class also had to be updated to hold the two font and
format instances, and decide which of the variants to use based on a
useItalicFont property in the drawing context, initially set in the
UpdateDrawingBrushes method.

Validation Steps Performed

I've created some test content using a range of different character sets
(e.g. CJK, block characters, emoji, etc.), then applied the italic
attribute mixed with various other SGR attributes to see how they
interact. The output isn't always perfect, but I think it seems
reasonable given the constraints of a cell-based terminal renderer.

Closes #5461

DHowett · 2020-12-14T00:44:21Z

I've had it on my TODO list for a long time to implement "cascading FontInfo", where a Renderer consumer could set up a group of fontinfos and have attribute-specific overrides only to meaningful properties (like, "italic" is "override nothing except this flag in the font lookup"; bold is "set face name to Xxx Bold, keep size and slant"). It's always conceptually been a blocker for me to have actual styling... but look, here it is and working today, and I'm not going to stand in the way of progress and hold out for some "ideal world" where our designs are perfect and community-ready at all times.

That's a lot of words for me to say to explain why I'm 100%, or maybe 150%, okay with this... so that folks on the team who know how I felt about FontInfo shouldn't be worried about how I'll feel. 😀

j4james · 2020-12-14T00:52:35Z

I'm was just going to say this is probably not a great approach, and I don't really know what I'm doing on the DX side of things, but I thought it was at least worth offering as a PR. If nothing else it could serve as a temporary solution until you have something better.

miniksa · 2020-12-14T22:07:47Z

I'm was just going to say this is probably not a great approach, and I don't really know what I'm doing on the DX side of things, but I thought it was at least worth offering as a PR. If nothing else it could serve as a temporary solution until you have something better.

The DX side of things is close with one observation that wouldn't necessarily be immediately apparent: A CustomTextLayout is a rather expensive object to create/maintain with all the vectors that it holds and maintains between calls for caching reasons. Having two of them will consume a lot more memory for a long period of time.

I would greatly prefer if both reg/italic font/formats were pushed into the CustomTextLayout on instantiation. Then at least we're not making all the vector buffers twice.

Whether or not it's italic at the moment could be stored in the DrawingContext like the fg/bg color data and you can flip that from the outside before writing a run to tell the CustomTextLayout as to which font style to use as it has a ref to that DrawingContext. Inside the CustomTextLayout, we can just key on that part of the DrawingContext to choose one or the other during layout and render steps.

j4james · 2020-12-14T22:46:58Z

I would greatly prefer if both reg/italic font/formats were pushed into the CustomTextLayout on instantiation. Then at least we're not making all the vector buffers twice.

OK. That sounds good. I'll give it a try.

j4james · 2020-12-17T19:17:41Z

src/renderer/gdi/state.cpp

-    _hfont((HFONT)INVALID_HANDLE_VALUE)
+    _hfont(nullptr),


Note that this was a deliberate change - the correct error value for a font is null rather than INVALID_HANDLE_VALUE. This was previously generating error logs when the font got deleted.

Woopsie. Thanks.

Great catch!

src/renderer/dx/CustomTextLayout.cpp

j4james · 2020-12-17T20:29:10Z

The more I look at the DX renderer, the less I feel I know what's going on, but I think I've got something working now with a single CustomTextLayout instance.

My one concern is that I'm not sure the block characters are handled correctly when italic. On the plus side, they have less render artifacts than the non-italic variant, but I think they're more likely to have gaps between blocks in some situations. That said, I wouldn't expect people to be using block characters in italics anyway, so I don't think it's a big deal.

But that brings me to another issue: quite a few fonts that I've tested actually render the block characters at an angle when italic, which doesn't really make any sense. The line characters don't connect vertically, and the blocks get sort of chopped off at the edges. Technically I think this is the font's responsibility, but it is probably something we could fix (at least in the DX renderer) if we really wanted to. In practice I think it's unlikely to be a problem though.

j4james · 2020-12-17T20:35:23Z

Btw here's a screenshot of my test pattern using the Cascadia Code font. You can see some of the issues I mentioned in the block character section.

miniksa · 2020-12-17T23:17:02Z

The more I look at the DX renderer, the less I feel I know what's going on, but I think I've got something working now with a single CustomTextLayout instance.

I'll take a look.

My one concern is that I'm not sure the block characters are handled correctly when italic. On the plus side, they have less render artifacts than the non-italic variant, but I think they're more likely to have gaps between blocks in some situations. That said, I wouldn't expect people to be using block characters in italics anyway, so I don't think it's a big deal.

But that brings me to another issue: quite a few fonts that I've tested actually render the block characters at an angle when italic, which doesn't really make any sense. The line characters don't connect vertically, and the blocks get sort of chopped off at the edges. Technically I think this is the font's responsibility, but it is probably something we could fix (at least in the DX renderer) if we really wanted to. In practice I think it's unlikely to be a problem though.

I agree.

Btw here's a screenshot of my test pattern using the Cascadia Code font. You can see some of the issues I mentioned in the block character section.

Thanks. It looks close enough for now. We can revisit block drawing in a different bug, not as a part of implementing italics if it becomes a big enough problem.

miniksa

Yeah that's more or less what I meant by the comment a few days ago. Looks pretty fine to me. Just the one outstanding discussion point on the bare vs. Com pointer. I think @DHowett will have an opinion or an insight that will help clear that one up and then we can approve and get this in.

miniksa · 2020-12-17T23:22:43Z

src/renderer/dx/CustomTextLayout.cpp

@@ -1226,7 +1242,7 @@ CATCH_RETURN();
    {
        // Get the font fallback first
        ::Microsoft::WRL::ComPtr<IDWriteTextFormat1> format1;
-        if (FAILED(_format.As(&format1)))
+        if (FAILED(::Microsoft::WRL::ComPtr<IDWriteTextFormat>(_formatInUse).As(&format1)))


I thought this strange, then I realized you're storing the InUse ones as a bare pointer, instead of as a ComPtr. Given the ComPtr is just a refcount anyway... should we maybe just store it that way so there are 2 refs outstanding for holding it twice in our class? I don't think that should be THAT harmful to performance overall.

Yeah I went back and forth with this a few times, but settled on the bare pointer in the end because it felt a bit wasteful having that extra ref counting for something that's entirely internal to the class. I wouldn't object to going back to a ComPtr if that's what you prefer though. Just let me know.

The lifetime is held by other member variables. I am comfortable with a bare pointer here.

Anyway, it will be instantly promoted to an owning reference (of TextFormat1) here as needed.

If you want to make this a little "clearer" (clearer to a COM person; instead of using a constructor call with _formatInUse), you can do this:

if (FAILED(_formatInUse->QueryInterface(IID_PPV_ARGS(&format1))))

IID_PPV_ARGS is a macro that takes a pointer to a COM thing and transforms it into an interface ID for that pointed-to type + the type as an argument pack.

the & operator on ComPtr acts roughly as expected of COM things.

QueryInterface returns a positive reference counted object into the pointed-to location so it's consistent with what WRL needs.

OK I'm good with what Dustin says then. Bare pointer is fine with me and you can optionally make it "clearer to a COM person" per his suggestion.

miniksa · 2020-12-17T23:22:43Z

src/renderer/gdi/state.cpp

-    _hfont((HFONT)INVALID_HANDLE_VALUE)
+    _hfont(nullptr),


Woopsie. Thanks.

DHowett

Totally comfortable with this. Minor readability nit (though whether you consider IID_PPV_ARGS more or less readable is rather up to you!)

DHowett · 2020-12-18T02:22:01Z

src/renderer/dx/CustomTextLayout.cpp

@@ -1226,7 +1242,7 @@ CATCH_RETURN();
    {
        // Get the font fallback first
        ::Microsoft::WRL::ComPtr<IDWriteTextFormat1> format1;
-        if (FAILED(_format.As(&format1)))
+        if (FAILED(::Microsoft::WRL::ComPtr<IDWriteTextFormat>(_formatInUse).As(&format1)))


If you want to make this a little "clearer" (clearer to a COM person; instead of using a constructor call with _formatInUse), you can do this:

if (FAILED(_formatInUse->QueryInterface(IID_PPV_ARGS(&format1))))

IID_PPV_ARGS is a macro that takes a pointer to a COM thing and transforms it into an interface ID for that pointed-to type + the type as an argument pack.

the & operator on ComPtr acts roughly as expected of COM things.

QueryInterface returns a positive reference counted object into the pointed-to location so it's consistent with what WRL needs.

DHowett · 2020-12-18T02:22:01Z

src/renderer/gdi/state.cpp

-    _hfont((HFONT)INVALID_HANDLE_VALUE)
+    _hfont(nullptr),


Great catch!

DHowett · 2020-12-18T02:22:01Z

src/renderer/gdi/state.cpp

@@ -300,6 +322,16 @@ GdiEngine::~GdiEngine()
    // Save the font.
    _hfont = hFont.release();


it is hilarious that we used the smart pointer types for all of the 8 seconds required to ... decapsulate them and store them as raw handles in our class. LOL.

Yeah, I mean. I'm sure I had good intentions like "it'll be released properly if one of the interim steps fails and the RETURN_IF* macro fires. But it is pretty silly. The class should probably have some smart pointers too... future fix.

miniksa

I'm good. James, if you want to fix the readability nit that Dustin called out, go for it. I'll wait a bit to merge this in case you want that chance.

j4james · 2020-12-18T18:32:34Z

I was planning to make the change, but I'm still at work so won't be able to do anything until a little later tonight. I don't mind if you want to merge though.

mdtauk · 2020-12-18T18:34:44Z

Will there be a Faux Italic as well as a Faux Bold option when the chosen font does not include those varients?

DHowett · 2020-12-18T18:37:35Z

Will there be a Faux Italic as well as a Faux Bold option when the chosen font does not include those varients?

@mdtauk This is not intended to be configurable right now. If DirectWrite or GDI chooses to make a faux-italic variant of a font, that is its right.

Cascadia doesn't have an italic variant, so the one pictured above is faux-italic.

DHowett · 2020-12-18T18:38:10Z

@j4james if that's the only change, I'll make it on your branch & merge. 😄 Thanks

mdtauk · 2020-12-18T18:40:08Z

Will there be a Faux Italic as well as a Faux Bold option when the chosen font does not include those varients?

@mdtauk This is not intended to be configurable right now. If DirectWrite or GDI chooses to make a faux-italic variant of a font, that is its right.

Cascadia doesn't have an italic variant, so the one pictured above is faux-italic.

So this is something handled by DirectWrite then, fair enough.

msftbot · 2020-12-18T18:45:28Z

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.

DHowett · 2020-12-18T19:23:38Z

Hmm. Our agents are on the floor.

) This PR adds support for the ANSI _italic_ graphic rendition attribute, which is enabled by the `SGR 3` escape sequence. For the GDI renderer, I've just created an additional italic variant of the font, and then the `UpdateDrawingBrushes` method selects the appropriate font variant into the device context based on the requested text attributes. It's a bit more complicated in the DX renderer, because we need both an italic variant of the font, and a variant of the text format object. The `CustomTextLayout` class also had to be updated to hold the two font and format instances, and decide which of the variants to use based on a `useItalicFont` property in the drawing context, initially set in the `UpdateDrawingBrushes` method. ## Validation Steps Performed I've created some test content using a range of different character sets (e.g. CJK, block characters, emoji, etc.), then applied the italic attribute mixed with various other SGR attributes to see how they interact. The output isn't always perfect, but I think it seems reasonable given the constraints of a cell-based terminal renderer. Closes microsoft#5461

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

Add support for italics in the GDI renderer.

660b32f

msftbot bot added Area-Rendering Issue-Task Product-Terminal labels Dec 13, 2020

Add support for italics in the DX renderer.

Loading status checks…

966f748

j4james force-pushed the j4james:feature-sgr-italic branch from d5e2966 to 966f748 Dec 14, 2020

Reimplement DX renderer with a single text layout.

Loading status checks…

13f0da5

j4james reviewed Dec 17, 2020

View changes

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

j4james marked this pull request as ready for review Dec 17, 2020

miniksa requested changes Dec 17, 2020

View changes

msftbot bot added Needs-Author-Feedback and removed Needs-Author-Feedback labels Dec 17, 2020

DHowett approved these changes Dec 18, 2020

View changes

miniksa approved these changes Dec 18, 2020

View changes

Avoid a constructor call on ComPtr

Loading status checks…

1642c4c

DHowett added the AutoMerge label Dec 18, 2020

j4james deleted the j4james:feature-sgr-italic branch Dec 19, 2020

skyline75489 mentioned this pull request Feb 10, 2021

Introduce DxFontRenderData #9096

Merged

microsoft / terminal

Add support for the "italic" graphic rendition attribute #8580

Add support for the "italic" graphic rendition attribute #8580

j4james commented Dec 13, 2020 •

edited by DHowett

DHowett commented Dec 14, 2020 •

edited

j4james commented Dec 14, 2020

miniksa commented Dec 14, 2020

j4james commented Dec 14, 2020

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

j4james commented Dec 17, 2020

j4james commented Dec 17, 2020

miniksa commented Dec 17, 2020

miniksa left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

DHowett left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

miniksa left a comment

j4james commented Dec 18, 2020

mdtauk commented Dec 18, 2020

DHowett commented Dec 18, 2020 •

edited

DHowett commented Dec 18, 2020

mdtauk commented Dec 18, 2020

msftbot bot commented Dec 18, 2020

DHowett commented Dec 18, 2020

		@@ -300,6 +322,16 @@ GdiEngine::~GdiEngine()
		// Save the font.
		_hfont = hFont.release();

Feb	MAR	Apr
	28
2020	2021	2022

microsoft / terminal

Add support for the "italic" graphic rendition attribute #8580

Add support for the "italic" graphic rendition attribute #8580

Conversation

j4james commented Dec 13, 2020 • edited by DHowett

Validation Steps Performed

DHowett commented Dec 14, 2020 • edited

j4james commented Dec 14, 2020

miniksa commented Dec 14, 2020

j4james commented Dec 14, 2020

This comment has been minimized.

j4james Dec 17, 2020 Author Contributor

This comment has been minimized.

miniksa Dec 17, 2020 Member

This comment has been minimized.

DHowett Dec 18, 2020 Member

j4james commented Dec 17, 2020

j4james commented Dec 17, 2020

miniksa commented Dec 17, 2020

miniksa left a comment

This comment has been minimized.

miniksa Dec 17, 2020 Member

This comment has been minimized.

j4james Dec 17, 2020 Author Contributor

This comment has been minimized.

DHowett Dec 18, 2020 Member

This comment has been minimized.

DHowett Dec 18, 2020 Member

This comment has been minimized.

miniksa Dec 18, 2020 Member

This comment has been minimized.

miniksa Dec 17, 2020 Member

DHowett left a comment

This comment has been minimized.

DHowett Dec 18, 2020 Member

This comment has been minimized.

DHowett Dec 18, 2020 Member

This comment has been minimized.

DHowett Dec 18, 2020 Member

This comment has been minimized.

miniksa Dec 18, 2020 • edited Member

miniksa left a comment

j4james commented Dec 18, 2020

mdtauk commented Dec 18, 2020

DHowett commented Dec 18, 2020 • edited

DHowett commented Dec 18, 2020

mdtauk commented Dec 18, 2020

msftbot bot commented Dec 18, 2020

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.

DHowett commented Dec 18, 2020

j4james commented Dec 13, 2020 •

edited by DHowett

DHowett commented Dec 14, 2020 •

edited

j4james Dec 17, 2020
Author Contributor

miniksa Dec 17, 2020
Member

DHowett Dec 18, 2020
Member

miniksa Dec 17, 2020
Member

j4james Dec 17, 2020
Author Contributor

DHowett Dec 18, 2020
Member

DHowett Dec 18, 2020
Member

miniksa Dec 18, 2020
Member

miniksa Dec 17, 2020
Member

DHowett Dec 18, 2020
Member

DHowett Dec 18, 2020
Member

DHowett Dec 18, 2020
Member

miniksa Dec 18, 2020 •

edited

Member

DHowett commented Dec 18, 2020 •

edited

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.