Refactor grid line renderers with support for more line types #7107
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ghost
added
Area-Rendering
j4james
force-pushed
the
refactor-gridline-rendering
branch
from
d472d7e
to
89dacc0
Compare
j4james
force-pushed
the
refactor-gridline-rendering
branch
from
89dacc0
to
047022d
Compare
|
For reference, this is what the different line types look like in the GDI renderer:
And this is what they look like in the DirectX renderer:
|
miniksa
reviewed
Jul 30, 2020
miniksa
approved these changes
Jul 30, 2020
DHowett
approved these changes
Jul 30, 2020
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
1 task
ghost
pushed a commit
that referenced
this pull request
Aug 1, 2020
This PR adds support for the ANSI _crossed-out_ graphic rendition attribute, which is enabled by the `SGR 9` escape sequence. * Support for the escape sequences and storage of the attribute was originally added in #2917. * Support for drawing the strikethrough effect in the grid line renderer was added in #7107. Since the majority of the code required for this attribute had already been implemented, it was just a matter of activating the `GridLines::Strikethrough` style in the `Renderer::s_GetGridlines` method when the `CrossedOut` attribute was set. VALIDATION There were already some unit tests in place in `VtRendererTest` and the `ScreenBufferTests`, but I've also now extended the SGR tests in `AdapterTest` to cover this attribute. I've manually confirmed the first test case from #6205 now works as expected in both conhost and Terminal. Closes #6205
ghost
pushed a commit
that referenced
this pull request
Aug 3, 2020
This PR updates the rendering of the _underlined_ graphic rendition attribute, using the style specified in the active font, instead of just reusing the grid line at the bottom of the character cell. * Support for drawing the correct underline effect in the grid line renderer was added in #7107. There was already an `ExtendedAttributes` flag defined for the underlined state, but I needed to update the `SetUnderlined` and `IsUnderlined` methods in the `TextAttribute` class to use that flag now in place of the legacy `LVB_UNDERSCORE` attribute. This enables underlines set via a VT sequence to be tracked separately from `LVB_UNDERSCORE` grid lines set via the console API. I then needed to update the `Renderer::s_GetGridlines` method to activate the `GridLines::Underline` style when the `Underlined` attribute was set. The `GridLines::Bottom` style is still triggered by the `LVB_UNDERSCORE` attribute to produce the bottom grid line effect. Validation ---------- Because this is a change from the existing behaviour, certain unit tests that were expecting the `LVB_UNDERSCORE` to be toggled by `SGR 4` and `SGR 24` have now had to be updated to check the `Underlined` flag instead. There were also some UI Automation tests that were checking for `SGR 4` mapping to `LVB_UNDERSCORE` attribute, which I've now substituted with a test of the `SGR 53` overline attribute mapping to `LVB_GRID_HORIZONTAL`. These tests only work with legacy attributes, so they can't access the extended underline state, and I thought a replacement test that covered similar ground would be better than dropping the tests altogether. As far as the visual rendering is concerned, I've manually confirmed that the VT underline sequences now draw the underline in the correct position and style, while grid lines output via the console API are still displayed in their original form. Closes #2915
|
🎉 Handy links: |
ghost
mentioned this pull request
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a refactoring of the grid line renderers, adjusting the line
widths to scale with the font size, and optimising the implementation to
cut down on the number of draw calls. It also extends the supported grid
line types to include true underlines and strike-through lines in the
style of the active font.
The main gist of the optimisation was to render the horizontal lines
with a single draw call, instead of a loop with lots of little strokes
joined together. In the case of the vertical lines, which still needed
to be handled in a loop, I've tried to move the majority of static
calculations outside the loop, so there is bit of optimisation there
too.
At the same time this code was updated to support a variable stroke
width for the lines, instead of having them hardcoded to 1 pixel. The
width is now calculated as a fraction of the font size (0.025 "em"),
which is still going to be 1 pixel wide in most typical usage, but will
scale up appropriately if you zoom in far enough.
And in preparation for supporting the SGR strike-through attribute, and
true underlines, I've extended the grid line renders with options for
handling those line types as well. The offset and thickness of the lines
is obtained from the font metrics (rounded to a pixel width, with a
minimum of one pixel), so they match the style of the font.
VALIDATION
For now we're still only rendering grid lines, and only the top and
bottom lines in the case of the DirectX renderer in Windows Terminal. So
to test, I hacked in some code to force the renderer to use all the
different options, confirming that they were working in both the GDI and
DirectX renderers.
I've tested the output with a number of different fonts, comparing it
with the same text rendered in WordPad. For the most part they match
exactly, but there can be slight differences when we adjust the font
size for grid alignment. And in the case of the GDI renderer, where
we're working with pixel heights rather than points, it's difficult to
match the sizes exactly.
This is a first step towards supporting the strike-through attribute
(#6205) and true underlines (#2915).
Closes #6911