AtlasEngine: Implement remaining underlines and builtin glyphs for D2D #17278

lhecker · 2024-05-16T13:53:33Z

This implements builtin glyphs for our Direct2D renderer, as well as
dashed and curly underlines. With this in place the only two features
it doesn't support are inverted cursors and VT soft fonts.
This allows us to remove the _hack* members introduced in a6a0e44.

The implementation of dashed underlines is trivial, while curly
underlines use quadratic bezier curves. Caching the curve as a sprite
is possible, however I feel like that can be done in the future.

Builtin glyphs on the other hand require a cache, because otherwise
filling the entire viewport with shaded glyphs would result in poor
performance. This is why it's built on top of ID2D1SpriteBatch.
Unfortunately the API causes an eager flush of other pending graphics
instructions, which is why there's still a decent perf hit.

Finally, as a little extra, this fixes the rounded powerline glyph
shapes being slightly cut off. The fix is to simply don't round the
position and radius of the ellipsis/semi-circle.

Closes #17224

Validation Steps Performed

RenderingTests.exe updated ✅
All supported builtin glyphs look sorta right at different sizes ✅

DHowett · 2024-05-16T14:13:20Z

src/renderer/atlas/BuiltinGlyphs.cpp

@@ -1183,13 +1177,13 @@ void BuiltinGlyphs::DrawBuiltinGlyph(ID2D1Factory* factory, ID2D1DeviceContext*
        }
        case Shape_FilledEllipsis:
        {
-            const D2D1_ELLIPSE e{ { begXabs, begYabs }, endX, endY };
+            const D2D1_ELLIPSE e{ { rectX + begX, rectY + begY }, endX, endY };


why this change? it doesn't take into account the line offsets any longer

Filled shapes don't have any line width, so this will work just fine.
This change ensures that the rounded powerline glyphs aren't slightly cut off anymore.

okay but the empty glyphs DO have a line width - how does this apply on line 1186R?

(looks fine on my screen)

Ah right. More generally speaking, the lineOffsetX/Y code is only for snapping the coordinates of lines. In case of the "arc" shape glyph, this probably doesn't matter much because the entire glyph uses a lot of anti-aliasing anyway.

DHowett · 2024-05-16T14:14:26Z

src/renderer/atlas/BackendD3D.cpp


        _curlyLineHalfHeight = height * 0.5f;
-        _curlyUnderline.position = gsl::narrow_cast<u16>(lrintf(top));
-        _curlyUnderline.height = gsl::narrow_cast<u16>(lrintf(height));
+        _curlyUnderline.position = gsl::narrow_cast<u16>(position);


no more rounding - all this math now uses truncation. is that OK?

(I don't 100% know why the changes to the D3D backend were needed -- can you explain? Did you test them again at different font sizes and scalings?)

I did this because I copied the code over to D2D. While doing so I noticed that this code doesn't actually do any floating point math whatsoever. As you can see I don't need to do any explicit integer/float casts anywhere in the preceding lines (which would normally be required during AuditMode). I changed the D3D part so that it's mostly identical to the D2D part.

src/renderer/atlas/AtlasEngine.cpp

DHowett

tested, can't turn em off!

DHowett · 2024-05-16T21:46:33Z

src/renderer/atlas/BuiltinGlyphs.cpp

@@ -1183,13 +1177,13 @@ void BuiltinGlyphs::DrawBuiltinGlyph(ID2D1Factory* factory, ID2D1DeviceContext*
        }
        case Shape_FilledEllipsis:
        {
-            const D2D1_ELLIPSE e{ { begXabs, begYabs }, endX, endY };
+            const D2D1_ELLIPSE e{ { rectX + begX, rectY + begY }, endX, endY };


okay but the empty glyphs DO have a line width - how does this apply on line 1186R?

DHowett · 2024-05-16T21:53:10Z

src/renderer/atlas/BuiltinGlyphs.cpp

@@ -1183,13 +1177,13 @@ void BuiltinGlyphs::DrawBuiltinGlyph(ID2D1Factory* factory, ID2D1DeviceContext*
        }
        case Shape_FilledEllipsis:
        {
-            const D2D1_ELLIPSE e{ { begXabs, begYabs }, endX, endY };
+            const D2D1_ELLIPSE e{ { rectX + begX, rectY + begY }, endX, endY };


(looks fine on my screen)

carlos-zamora · 2024-05-16T22:24:41Z

src/renderer/atlas/BackendD2D.cpp

+
+void BackendD2D::_prepareBuiltinGlyphRenderTarget(const RenderingPayload& p)
+{
+    if (!_builtinGlyphBatch || _builtinGlyphsRenderTarget)


Suggested change

if (!_builtinGlyphBatch || _builtinGlyphsRenderTarget)

if (!_builtinGlyphBatch || !_builtinGlyphsRenderTarget)

(I'm only like half-understanding what's going on so correct me if I'm wrong) Wouldn't we want to exit early if _buildinGlyphsRenderTarget is not defined? I see we overwrite it a few lines later. Curious.

Ah, yeah that condition sucks. Let me split it apart and add comments!

Should be much better now!

DHowett

these comments are amazing

#17278) This implements builtin glyphs for our Direct2D renderer, as well as dashed and curly underlines. With this in place the only two features it doesn't support are inverted cursors and VT soft fonts. This allows us to remove the `_hack*` members introduced in a6a0e44. The implementation of dashed underlines is trivial, while curly underlines use quadratic bezier curves. Caching the curve as a sprite is possible, however I feel like that can be done in the future. Builtin glyphs on the other hand require a cache, because otherwise filling the entire viewport with shaded glyphs would result in poor performance. This is why it's built on top of `ID2D1SpriteBatch`. Unfortunately the API causes an eager flush of other pending graphics instructions, which is why there's still a decent perf hit. Finally, as a little extra, this fixes the rounded powerline glyph shapes being slightly cut off. The fix is to simply don't round the position and radius of the ellipsis/semi-circle. Closes #17224 ## Validation Steps Performed * RenderingTests.exe updated ✅ * All supported builtin glyphs look sorta right at different sizes ✅ (cherry picked from commit 3486111) Service-Card-Id: 92572076 Service-Version: 1.21

AtlasEngine: Implement builtin glyphs for D2D

acb3e29

lhecker changed the title ~~AtlasEngine: Implement builtin glyphs for D2D~~ AtlasEngine: Implement remaining underlines and builtin glyphs for D2D May 16, 2024

DHowett reviewed May 16, 2024

View reviewed changes

Fix constness

e8f0e91

DHowett reviewed May 16, 2024

View reviewed changes

src/renderer/atlas/AtlasEngine.cpp Show resolved Hide resolved

DHowett requested changes May 16, 2024

View reviewed changes

microsoft-github-policy-service bot added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label May 16, 2024

Fix the builtinGlyph setting

b5c79ab

microsoft-github-policy-service bot removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label May 16, 2024

lhecker added 2 commits May 16, 2024 23:38

Address feedback

26dabaf

Forgot to stage this

313be14

DHowett approved these changes May 16, 2024

View reviewed changes

carlos-zamora approved these changes May 16, 2024

View reviewed changes

Add comments

e015a0c

DHowett approved these changes May 16, 2024

View reviewed changes

DHowett enabled auto-merge May 16, 2024 23:30

DHowett added this pull request to the merge queue May 16, 2024

Merged via the queue into main with commit 3486111 May 17, 2024
20 checks passed

DHowett deleted the dev/lhecker/atlas-engine-d2d-improvements branch May 17, 2024 00:11

lhecker added Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. Area-Settings UI Anything specific to the SUI labels Nov 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AtlasEngine: Implement remaining underlines and builtin glyphs for D2D #17278

AtlasEngine: Implement remaining underlines and builtin glyphs for D2D #17278

lhecker commented May 16, 2024 •

edited

Loading

DHowett May 16, 2024

lhecker May 16, 2024

DHowett May 16, 2024

DHowett May 16, 2024

lhecker May 16, 2024

DHowett May 16, 2024

DHowett May 16, 2024

lhecker May 16, 2024

DHowett left a comment

DHowett May 16, 2024

DHowett May 16, 2024

carlos-zamora May 16, 2024

lhecker May 16, 2024

lhecker May 16, 2024

DHowett left a comment

	if (!_builtinGlyphBatch \|\| _builtinGlyphsRenderTarget)
	if (!_builtinGlyphBatch \|\| !_builtinGlyphsRenderTarget)

AtlasEngine: Implement remaining underlines and builtin glyphs for D2D #17278

AtlasEngine: Implement remaining underlines and builtin glyphs for D2D #17278

Conversation

lhecker commented May 16, 2024 • edited Loading

Validation Steps Performed

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

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

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

lhecker commented May 16, 2024 •

edited

Loading