Add support for double-width/double-height lines in conhost #8664
Conversation
ghost
added
Area-VT
|
I've tried to keep this as simple and efficient as I could, but it touches on almost every part of the system, so it seems inevitable that it's going to have some impact on performance. And given that there isn't much demand for this functionality, I'd completely understand if you decide not to merge this. That said, here are a couple of screenshots so you can see how well it handles VT Test and the VT100 Torture Test.
|
|
wow |
James... with high quality contributions like this, it would be a crime for us to not merge it at some point. I'll even catch your back with the performance implications and writing out the DX version of the rendering if I must. It just might take us a little bit of time to digest the whole thing before we can take it. Once again, thank you so much. I hope to read through it soon. |
j4james
force-pushed
the
feature-line-attributes
branch
from
3e3ba06
to
525ebd4
Compare
ghost
added
the
Needs-Author-Feedback
|
This is the only thing I noticed in writing
It looks like we're calling
It does look like its a very small amount in CPU time, but there's probably some amount of thunk/kernel switching that isn't being accounted for here too that we could avoid. |
ghost
removed
the
Needs-Author-Feedback
I'm really glad you spotted that. I actually had checks in place to avoid setting and resetting the transform unnecessarily in the And I just realised there's also the special case handling for the inverted cursor, which has to "uninvert" itself when scrolling, and that requires setting a transform first. So that's also going to need a check to avoid the transform when it's just the identity matrix. |
|
OK. I've added the requested comments, made the |
j4james
force-pushed
the
feature-line-attributes
branch
from
7a0dbd4
to
88f22c7
Compare
ghost
added
the
Needs-Author-Feedback
|
@hafixo Please stop commenting on unrelated PRs without actionable feedback. |
ghost
added
the
No-Recent-Activity
|
This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 7 days of this comment. |
|
Sorry, @j4james, this is all on me. I've been travelling and not had a chance to look over this in-depth. I'll clear the stale state and get it in this week. 😄 Thanks again for excellent work. |
| // Reset the line rendition for all of these rows. | ||
| success = success && _pConApi->PrivateResetLineRenditionRange(csbiex.srWindow.Top, csbiex.srWindow.Bottom); |
There was a problem hiding this comment.
Huh, we fill after we reset the rendition? I don't totally understand why, but I trust your homework here.
There was a problem hiding this comment.
I don't think it matters. The fill operation doesn't care about line renditions, so I suspect it would work in either order. I do know that this sequence works correctly as is - it's one of my manual test cases.
There was a problem hiding this comment.
I've just double checked this and the order doesn't make any difference. In both case the lines are reset to single width and the screen is completely filled with Es.
j4james
force-pushed
the
feature-line-attributes
branch
from
88702d4
to
3f72563
Compare
|
FYI, I'm going to bed now, so if there is many more merging or changes required it'll have to wait until tomorrow. |
DHowett
added
the
AutoMerge
|
Hello @DHowett! Because this pull request has the 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 (
|
ghost
merged commit
|
Beautiful work with great attention to detail, as always. Thanks again @j4james! |
|
What the... I am very glad to see you having implemented this. Good work! |
|
This is available in conhost as of Windows Insider Preview build 21337. Congrats! 😄 |
This PR adds support for the VT line rendition attributes in the DirectX renderer, which allows for double-width and double-height line renditions. Line renditions were first implemented in conhost (with the GDI renderer) in PR #8664. Supporting them in the DX renderer now is a small step towards #11595. The DX implementation is very similar to the GDI one. When a particular line rendition is requested, we create a transform that is applied to the render target. And in the case of double-height renditions, we also initialize some clipping offsets to allow for the fact that we only render half of a line at a time. One additional complication exists when drawing the cursor, which requires a two part process where it first renders to a command list, and then draw the command list in a second step. We need to temporarily reset the transform in that first stage otherwise it ends up being applied twice. I've manually tested the renderer in conhost by setting the `UseDx` registry entry and confirmed that it passes the _Vttest_ double-size tests as well as several of my own tests. I've also checked that the renderer can now handle horizontal scrolling, which is a feature we get for free with the transforms.
This PR introduces a mechanism for passing through line rendition attributes to the conpty client, so we can support double-width and double-height text in Windows Terminal. Line renditions were first implemented in conhost (with the GDI renderer) in PR #8664, and were implemented in the DX renderer in PR #13102. By default this won't add any additional overhead to the conpty output, but as soon as there is any usage of double-size text, we switch to a mode in which every line output will be prefixed with a line rendition sequence. This is to ensure that the line attributes in the client terminal are always in sync with the host. Since this does add some overhead to the conpty output, we'd prefer not to remain in this mode longer than necessary. So whenever there is a full repaint of the entire viewport, we check to see if all of the lines are single-width. If that is the case, we can then safely skip the line rendition sequences in future updates. One other small optimization is when the conpty update is only writing out a single character (this is something we already check for). When that is the case, we can safely skip the line rendition prefix, because a single character update should never include a change of the line rendition. ## Validation Steps Performed I've manually tested that Windows Terminal now passes the double-size tests in _Vttest_, and also confirmed various edge cases are working correctly in my own double-size tests. Closes #11595
This PR adds support for the VT line rendition attributes, which allow
for double-width and double-height line renditions. These renditions are
enabled with the
DECDWL(double-width line) andDECDHL(double-height line) escape sequences. Both reset to the default
rendition with the
DECSWL(single-width line) escape sequence. For nowthis functionality is only supported by the GDI renderer in conhost.
There are a lot of changes, so this is just a general overview of the
main areas affected.
Previously it was safe to assume that the screen had a fixed width, at
least for a given point in time. But now we need to deal with the
possibility of different lines have different widths, so all the
functions that are constrained by the right border (text wrapping,
cursor movement operations, and sequences like
ELandICH) now needto lookup the width of the active line in order to behave correctly.
Similarly it used to be safe to assume that buffer and screen
coordinates were the same thing, but that is no longer true. Lots of
places now need to translate back and forth between coordinate systems
dependent on the line rendition. This includes clipboard handling, the
conhost color selection and search, accessibility location tracking and
screen reading, IME editor positioning, "snapping" the viewport, and of
course all the rendering calculations.
For the rendering itself, I've had to introduce a new
PrepareLineTransformmethod that the render engines can use to setupthe necessary transform matrix for a given line rendition. This is also
now used to handle the horizontal viewport offset, since that could no
longer be achieved just by changing the target coordinates (on a double
width line, the viewport offset may be halfway through a character).
I've also had to change the renderer's existing
InvalidateCursormethod to take a
SMALL_RECTrather than aCOORD, to allow for thecursor being a variable width. Technically this was already a problem,
because the cursor could occupy two screen cells when over a
double-width character, but now it can be anything between one and four
screen cells (e.g. a double-width character on the double-width line).
In terms of architectural changes, there is now a new
lineRenditionfield in the
ROWclass that keeps track of the line rendition for eachrow, and several new methods in the
ROWandTextBufferclasses formanipulating that state. This includes a few helper methods for handling
the various issues discussed above, e.g. position clamping and
translating between coordinate systems.
Validation Steps Performed
I've manually confirmed all the double-width and double-height tests in
Vttest are now working as expected, and the VT100 Torture Test now
renders correctly (at least the line rendition aspects). I've also got
my own test scripts that check many of the line rendition boundary cases
and have confirmed that those are now passing.
I've manually tested as many areas of the conhost UI that I could think
of, that might be affected by line rendition, including things like
searching, selection, copying, and color highlighting. For
accessibility, I've confirmed that the Magnifier and Narrator
correctly handle double-width lines. And I've also tested the Japanese
IME, which while not perfect, is at least useable.
Closes #7865