Strawman: Dumb implementation of text D2D effects. #817

simonbuchan · 2019-05-15T09:40:09Z

More for feedback and ideas after I was screwing around with the code,
not really for merging, though this is probably not that far off.

Edit

Updated with settings parsing (though pretty hacky), and applies to all rendering output:

Note the offset x,y settings are not hooked up, but they are other settings you'd probably want. I'm thinking all those should be extracted to a shadow setting block and a shadow renderer effect taking it that can plug in, design wise.

Original

After building this, I realized you probably actually want to do it on
the whole buffer, not just the text layer, which would make things way
easier, and correctly handle cases like the vim bottom separator.

Here's the current shadow effect:

And without acrylic:

The super-dumb displacement effect, also showing a weird limit on the affected output size:

After building this, I realized you probably actually want to do it on the whole buffer, not just the text layer, which would make things way easier.

msftclas · 2019-05-15T09:40:23Z

All CLA requirements met.

simonbuchan · 2019-05-15T09:44:47Z

Also currently doesn't implement letting you set the shadow color/alpha, size, or offset for the shadow.

The effect being on the text means that it's also doesn't affect the cursor or underlines, etc...

Note that if you're going to restore invalidation (#778), that effects, including shadow, increase the invalidated area.

Suggestions welcome on what the config for this should look like, if any of the other built-in d2d effects should be supported.

miniksa

I like the idea in practice, but you're right.... this probably needs to somehow apply at the entire display/render target surface, not just the text itself.

I'd rather have an effect apply to the entire composition so I don't have to be concerned with how each layer is drawn/composed.

If we do that, it might also work properly with partial invalidation... depending on where we can apply the effect at a higher level.

However.... do note that while this is cool playing around, I probably can't accept this right away or I'll have to accept it as nerfed out additional functions that aren't actually usable code yet (like _DrawGlowTextRun which isn't really hooked up yet). It's way more important to us to get the base layers of rendering up to speed and the differential drawing up and then focus on the fun effects a little later.

I'm also going to have to personally cross the bridge of figuring out how to carry settings down from the TerminalControl interface into the renderer itself to enable other such preferences like which cleartype/antialiasing the user would like. And I was planning on working it through with @zadjii-msft. So if you wait a little while until I get to put more effort into this, some of those things will be solved for you.

miniksa · 2019-05-16T03:43:10Z

src/cascadia/TerminalControl/TerminalControl.vcxproj.filters

@@ -11,20 +11,15 @@
  </ItemGroup>
  <ItemGroup>
    <ClCompile Include="pch.cpp" />
-    <ClCompile Include="KeyChord.cpp" />


What's going on here? Why are these getting removed? (and below)

It doesn't look like you otherwise manipulated this module. Were these moved out by someone else and the filters wasn't updated?

I have no idea! I guess so, as the .vcxproj wasn't changed.

miniksa · 2019-05-16T03:45:12Z

src/renderer/dx/CustomTextRenderer.h

@@ -18,6 +22,8 @@ namespace Microsoft::Console::Render
                       const D2D1_DRAW_TEXT_OPTIONS options = D2D1_DRAW_TEXT_OPTIONS_NONE)
        {
            this->renderTarget = renderTarget;
+			this->textRenderContext = textRenderContext;


you're using tabs. we use 4 spaces.

Oops, missed one!

miniksa · 2019-05-16T03:45:22Z

src/renderer/dx/CustomTextRenderer.h

@@ -3,13 +3,17 @@

 #pragma once

+#include <d2d1_3.h>


this probably belongs in a precomp

Weirdly, it is in the precomp.h for all the projects I found that should be using it, but without it the build failed, but I didn't look too much into it. Definitely a FIXME.

Correction: It is explicitly #included in the precomp.h for RendererDx, but this file ends up included in a whole bunch of other projects. I couldn't find an appropriate file to add it, e.g. one with other #include <d2d1*.h> lines.

Why does this end up included in other projects? Shouldn't it just be scoped inside DxRenderer?

It's included in DxRenderer.hpp rather than forwarded, and that is included in InteractivityWin32's Window.cpp and TerminalControls TermControl.{h,cpp}. Seems like the right place to put it going by current style would be DxRenderer.hpp then, but better for those to have a pimpl pattern or get them onto IRenderEngine.

simonbuchan · 2019-05-16T06:05:26Z

I probably can't accept this right away

Definitely: this should have a bunch more iteration, just wanted to get some feedback early.

If we do that, it might also work properly with partial invalidation... depending on where we can apply the effect at a higher level.

I'm thinking that if the custom text renderer stuff is reverted but for using the provided bitmap render target that invalidation at the text run level should work fine, For applying the effect either:

Limit to effects with known invalidation impact: eg. CLSID_D2D1Shadow increases the invalidated area by 3 * blur radius (in DIPs).
Assume that text rendering is expensive and effects are (relatively) cheap, and only use invalidation for rendering to the bitmap render target, and always run the whole effect. This seems plausible to me?

focus on the fun effects a little later.

While true, a text shadow does make very low acrylic opacities usable, so it still is a functionality improvement... for a fun effect 😉

I'm also going to have to personally cross the bridge of figuring out how to carry settings down from the TerminalControl interface into the renderer itself

TerminalApp is already calling TerminalControl::UpdateSettings(), and TerminalControl has a DxEngine in _renderEngine, it doesn't look too complicated? If I can figure out the IDL stuff going on with the settings I might take a stab?

simonbuchan · 2019-05-16T09:33:40Z

@miniksa, I added some placeholder parsing, I'm assuming you would want a general IRendererSettings that the renderer could keep?

I also... simplified? the rendering, switching between using the bitmap render target and the dxgi render target for all draw calls. Since a device context is also a render target, that means we end up with basically the same contents in DrawingContext, except that the ID2D1DeviceContext4 doesn't have to be re-queried for each text run.

Side note, while poking around, I'm curious about DxRenderer::_CopyFrontToBack(): from my understanding, reading the front buffer is extremely expensive? One of the sequential swap effects seem like they should have the same effect, but maybe there's a UWP restriction or something. In this case, it may be worth keeping the bitmap render target even when not effect rendering just to preserve the last frame.

I also noticed what looks like some low hanging fruit for caching SolidColorBrushes, etc... in a few places, but I'm not sure what the cost is to create those.

miniksa · 2019-05-21T16:51:51Z

Sorry @simonbuchan, you have way more time to think about this right now than I do. I just know that I want to get back in here as soon as I can.

I haven't thought through the right way to do settings yet. IRendererSettings sounds plausible though.

Simplifying the rendering/switching between two of the interfaces to the one that doesn't have to be re-queried is goodness that I would want to get to.

The _CopyFrontToBack function is also something that just needs investigation. I don't like that either because it's expensive, but I couldn't get the swap chain to draw correctly otherwise. I stared down the documentation for a while and gave up temporarily to revisit that later. If you can figure it out, go for it. I would much prefer to never have to do a copy and use the FLIP_SEQUENTIAL chain mode.

Creating brushes is expensive-ish. That's why to some degree I cached the foreground/background ones and just keep adjusting their colors/opacities as text is drawn. Then I came through and added a bunch of other complexity to the renderer that didn't use those two brushes. If we can just get away with one SolidColorBrush that we change the colors/opacity of... great. If we can cache the little ancillary ones otherwise... also good. The fewer brushes we need, the better, I think.

simonbuchan · 2019-05-22T09:28:38Z

Sorry @simonbuchan, you have way more time to think about this right now than I do. I just know that I want to get back in here as soon as I can.

No problem, this is mostly just me getting my feet wet in the codebase and seeing how stuff fits together.

The _CopyFrontToBack function is also something that just needs investigation. I don't like that either because it's expensive, but I couldn't get the swap chain to draw correctly otherwise. I stared down the documentation for a while and gave up temporarily to revisit that later. If you can figure it out, go for it. I would much prefer to never have to do a copy and use the FLIP_SEQUENTIAL chain mode.

Hmm, even after commenting out the _CopyBackToFront() call, FLIP_DISCARD and FLIP_SEQUENTIAL both seemed to work fine for both conhost.exe and TerminalApp.exe? I did a quick check (running without debugging, which seems to make a huge difference) with time ls -R node_modules in different variants and didn't see anything statistically significant, and vim with :set relativenumber cursorcolumn cursorline on a page of highlighted text, which could easily have +1s update times on the GDI renderer are still as fast to respond as in the command line (that is, a teeny bit laggy) with and without it, and I didn't notice any glitches.

Creating brushes is expensive-ish. That's why to some degree I cached the foreground/background ones and just keep adjusting their colors/opacities as text is drawn. Then I came through and added a bunch of other complexity to the renderer that didn't use those two brushes. If we can just get away with one SolidColorBrush that we change the colors/opacity of... great. If we can cache the little ancillary ones otherwise... also good. The fewer brushes we need, the better, I think.

It was pretty easy to do all this for the CustomTextRenderer, but it doesn't really move the performance needle, since it's dominated by text layout before hand. Probably not worth a PR at this point.

miniksa · 2019-05-22T17:05:23Z

The _CopyFrontToBack function is also something that just needs investigation. I don't like that either because it's expensive, but I couldn't get the swap chain to draw correctly otherwise. I stared down the documentation for a while and gave up temporarily to revisit that later. If you can figure it out, go for it. I would much prefer to never have to do a copy and use the FLIP_SEQUENTIAL chain mode.

Hmm, even after commenting out the _CopyBackToFront() call, FLIP_DISCARD and FLIP_SEQUENTIAL both seemed to work fine for both conhost.exe and TerminalApp.exe? I did a quick check (running without debugging, which seems to make a huge difference) with time ls -R node_modules in different variants and didn't see anything statistically significant, and vim with :set relativenumber cursorcolumn cursorline on a page of highlighted text, which could easily have +1s update times on the GDI renderer are still as fast to respond as in the command line (that is, a teeny bit laggy) with and without it, and I didn't notice any glitches.

When testing conhost.exe, make sure that the UseDx flag in the registry is set to 1 or it will use the GDI one. I make that mistake all the time, even though it's my code. The trick to checking which one you have quickly in conhost.exe is selecting the screen. GDI inverts. DX does a translucent gray.

I'm not sure why it's working correctly without _CopyBackToFront() then.

I usually use a full WPR trace though to compare performance characteristics, not something like a timed command. Sometimes when you make one part of the code faster, a different part decides to then waste the time and they actually both need to be fixed for an overall improvement. That's hard to see from a simple wall-clock analysis and easier with a hot-stack-trace.

Creating brushes is expensive-ish. That's why to some degree I cached the foreground/background ones and just keep adjusting their colors/opacities as text is drawn. Then I came through and added a bunch of other complexity to the renderer that didn't use those two brushes. If we can just get away with one SolidColorBrush that we change the colors/opacity of... great. If we can cache the little ancillary ones otherwise... also good. The fewer brushes we need, the better, I think.

It was pretty easy to do all this for the CustomTextRenderer, but it doesn't really move the performance needle, since it's dominated by text layout before hand. Probably not worth a PR at this point.

Alright, that's a fine conclusion. I don't mind either way. We have a lot of minor PRs for people getting their feet wet, but not bothering until something substantial is fine too.

simonbuchan · 2019-05-23T06:06:02Z

When testing conhost.exe, make sure that the UseDx flag in the registry is set to 1 or it will use the GDI one. I make that mistake all the time, even though it's my code. The trick to checking which one you have quickly in conhost.exe is selecting the screen. GDI inverts. DX does a translucent gray.

Ah, you're right - good to know!

I usually use a full WPR trace though to compare performance characteristics, not something like a timed command. Sometimes when you make one part of the code faster, a different part decides to then waste the time and they actually both need to be fixed for an overall improvement. That's hard to see from a simple wall-clock analysis and easier with a hot-stack-trace.

Ah, yes I was a bit misleading there. I actually did profile first and saw the majority of the time spent on text layout and basically none spent on the actual rendering commands or the present. I did a timed command and compared against the same redirected to /dev/null to verify that it wasn't the CPU sampler ignoring the CPU being stalled on the GPU for most of the runtime or something.

DHowett-MSFT · 2019-05-23T22:23:02Z

@mdtauk That's cool, but it doesn't seem material to the rendering discussion in this thread?

mdtauk · 2019-05-23T22:28:57Z

@mdtauk That's cool, but it doesn't seem material to the rendering discussion in this thread?

Got me, I was just commenting on that statusbar in the images above. Comment removed.

If these kinds of effects were to become part of the app - and if a statusbar becomes part of the console area (rather than in the window frame below) should these effects affect the status area?

simonbuchan · 2019-05-24T05:21:45Z

Got me, I was just commenting on that statusbar in the images above. Comment removed.

It's just the existing setting experimental_showTabsInTitleBar. It's a little annoying as the area between the tabs and the new tab button isn't draggable.

If these kinds of effects were to become part of the app - and if a statusbar becomes part of the console area (rather than in the window frame below) should these effects affect the status area?

Not exactly sure what you mean by a status bar or console area, but I would guess by default these settings would be purely for the terminal control itself. If the suggestion of extending the background up into the selected tab is done, it might make sense then?

mdtauk · 2019-05-24T06:54:16Z

@simonbuchan In the trailer video for the new Terminal, it shows a statusbar at the bottom of the window

In the various mock ups I have done to illustrate ideas I have submitted issues for our responded to, I've included the statusbar into the console area itself, not below it on the window acrylic itself.

So if these fun text effects got added officially, and if the statusbar was to be part of the console view itself, should it be affected by the effect, or should it be immune?

WSLUser · 2019-12-09T19:29:46Z

Once #3468 is merged, can we use that as a base to update this PR to bring in more cool visual D2D features?

zadjii-msft · 2019-12-09T20:02:04Z

Honestly, I'm not sure we're ever going to really ship this in Windows Terminal 1.0 built in. To me, it makes a lot more sense as an extension. #3468 is pretty similar, but I think it's a polished enough appearance that we could accept it as an experimental feature for now, and move it to an extension in the future.

@simonbuchan Do you mind if we close this PR for now? We'll certainly use learnings from it to influence future extensions planning.

simonbuchan · 2019-12-09T22:02:46Z

Of course! It was always meant as a strawman, to discuss the value of this and see the code impact, and to be re-written when the code base was more settled (see "Dumb implementation" in the title)

That said, implementation aside, I feel if you're going to natively support only one effect, a drop shadow to increase contrast against low-opacity backgrounds would be the one I would pick (which is why I did, of course).

I put this down when it reached the basic level needed for the above, and the code base started shifting pretty quickly under it (particularly in settings). I'm happy to try this out again when you start looking at extensions, if you're looking at something to exercise them, maybe.

In particular, it seems like packaging effects like #3468 as Direct2D custom effects, so they can be more easily mix-and-matched together with D2D built-in effects by configuration might be a good way to go? In the extreme, you could probably define #3468 in terms of the built in effects (tiled bitmap source for the scanlines, blur and multiply the source for the glow, add the blur and source, multiple that with the scanlines), but that's probably worse performance.

Dumb implementation of text D2D effects.

711c6f5

After building this, I realized you probably actually want to do it on the whole buffer, not just the text layer, which would make things way easier.

miniksa reviewed May 16, 2019

View reviewed changes

miniksa added the Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues label May 16, 2019

Apply shadow effect to all control rendering, add settings.

1610371

DHowett-MSFT mentioned this pull request May 23, 2019

[Settings] An option to disable colour font emoji #956

Closed

zadjii-msft added Area-Extensibility A feature that would ideally be fulfilled by us having an extension model. Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something Product-Terminal The new Windows Terminal. labels Dec 9, 2019

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Dec 9, 2019

simonbuchan closed this Dec 9, 2019

WSLUser mentioned this pull request Feb 11, 2020

Scenario: Add support for 3rd-party extensions #4000

Open

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Strawman: Dumb implementation of text D2D effects. #817

Strawman: Dumb implementation of text D2D effects. #817

simonbuchan commented May 15, 2019 •

edited

Loading

msftclas commented May 15, 2019 •

edited

Loading

simonbuchan commented May 15, 2019 •

edited

Loading

miniksa left a comment

miniksa May 16, 2019

simonbuchan May 16, 2019

miniksa May 16, 2019

simonbuchan May 16, 2019

miniksa May 16, 2019

simonbuchan May 16, 2019

simonbuchan May 16, 2019 •

edited

Loading

miniksa May 21, 2019

simonbuchan May 22, 2019

simonbuchan commented May 16, 2019

simonbuchan commented May 16, 2019

miniksa commented May 21, 2019

simonbuchan commented May 22, 2019

miniksa commented May 22, 2019

simonbuchan commented May 23, 2019

DHowett-MSFT commented May 23, 2019

mdtauk commented May 23, 2019

simonbuchan commented May 24, 2019

mdtauk commented May 24, 2019

WSLUser commented Dec 9, 2019

zadjii-msft commented Dec 9, 2019

simonbuchan commented Dec 9, 2019

Strawman: Dumb implementation of text D2D effects. #817

Strawman: Dumb implementation of text D2D effects. #817

Conversation

simonbuchan commented May 15, 2019 • edited Loading

Edit

Original

msftclas commented May 15, 2019 • edited Loading

simonbuchan commented May 15, 2019 • edited Loading

miniksa 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

Choose a reason for hiding this comment

simonbuchan May 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonbuchan commented May 16, 2019

simonbuchan commented May 16, 2019

miniksa commented May 21, 2019

simonbuchan commented May 22, 2019

miniksa commented May 22, 2019

simonbuchan commented May 23, 2019

DHowett-MSFT commented May 23, 2019

mdtauk commented May 23, 2019

simonbuchan commented May 24, 2019

mdtauk commented May 24, 2019

WSLUser commented Dec 9, 2019

zadjii-msft commented Dec 9, 2019

simonbuchan commented Dec 9, 2019

simonbuchan commented May 15, 2019 •

edited

Loading

msftclas commented May 15, 2019 •

edited

Loading

simonbuchan commented May 15, 2019 •

edited

Loading

simonbuchan May 16, 2019 •

edited

Loading