Eliminate the IRenderTarget interface #12568

j4james · 2022-02-24T16:00:14Z

Summary of the Pull Request

The purpose of the IRenderTarget interface was to support the concept
of multiple buffers in conhost. When a text buffer needed to trigger a
redraw, the render target implementation would be responsible for
deciding whether to forward that redraw to the renderer, depending on
whether the buffer was active or not.

This PR instead introduces a flag in the TextBuffer class to track
whether it is active or not, and it can then simply check the flag to
decide whether it needs to trigger a redraw or not. That way it can work
with the Renderer directly, and we can avoid a bunch of virtual calls
for the various redraw methods.

PR Checklist

Closes Can we get rid of the IRenderTarget interface? #12551
CLA signed.
Tests added/passed
Documentation updated.
Schema updated.
I've discussed this with core contributors already. Issue number
where discussion took place: Can we get rid of the IRenderTarget interface? #12551

Detailed Description of the Pull Request / Additional comments

Anywhere that had previously been getting an IRenderTarget from the
TextBuffer class in order to trigger a redraw, will now just call the
TriggerRedraw method on the TextBuffer class itself, since that will
handle the active check which used to be the responsibility of the
render target. All the redraw methods that were in IRenderTarget are
now exposed in TextBuffer instead.

For this to work, though, the Renderer needed to be available before
the TextBuffer could be constructed, which required a change in the
conhost initialization order. In the ConsoleInitializeConnectInfo
function, I had to move the Renderer initialization up so it could be
constructed before the call to SetUpConsole (which initializes the
buffer). Once both are ready, the Renderer::EnablePainting method is
called to start the render thread.

The other catch is that the renderer used to setup its initial viewport
in the constructor, but with the new initialization order, the viewport
size would not be known at that point. I've addressed that problem by
moving the viewport initialization into the EnablePainting method,
since that will only be called after the buffer is setup.

Validation Steps Performed

The changes in architecture required a number of tweaks to the unit
tests. Some were using a dummy IRenderTarget implementation which now
needed to be replaced with a full Renderer (albeit with mostly null
parameters). In the case of the scroll test, a mock IRenderTarget was
used to track the scroll delta, which now had to be replaced with a mock
RenderEngine instead.

Some tests previously relied on having just a TextBuffer but without a
Renderer, and now they require both. And tests that were constructing
the TextBuffer and Renderer themselves had to be updated to use the
new initialization order, i.e. with the renderer constructed first.

Semantically, though, the tests still essentially work the same way as
they did before, and they all still pass.

j4james · 2022-02-24T18:19:20Z

Note that this is going to conflict with PR #12561, but it shouldn't be hard to resolve once that's merged.

miniksa

I think this is a fantastic step forward in cutting things down. Let's do this.

miniksa · 2022-03-11T18:49:14Z

src/renderer/base/renderer.cpp

-    _pThread->EnablePainting();
+    // When the renderer is constructed, the initial viewport won't be available yet,
+    // but once EnablePainting is called it should be safe to retrieve.
+    _viewport = _pData->GetViewport();


Should this throw intentionally now if _pData is null given that it's not checked in the constructor anymore? Now it's implicitly crashing if someone did it wrong instead.

I don't think it's worth it. Either way it's not going to run, so it's not like it makes any real difference. With a null pointer dereference it'll stop straight away in the debugger, while a throw will silently shut down and you have to look for a log entry to find out what happened. And there's no chance of shipping with a bug like that, so a cleaner shutdown isn't going to benefit the users.

Also, if we really do want to add a null check, this isn't the only place we'd need to put it, because it's possible to be dereferenced earlier than this. To be really "safe", we'd be putting null checks everywhere, and that just feels like pointless bloat.

miniksa · 2022-03-11T19:07:18Z

src/host/consoleInformation.cpp

@@ -214,6 +214,16 @@ const SCREEN_INFORMATION& CONSOLE_INFORMATION::GetActiveOutputBuffer() const
    return *pCurrentScreenBuffer;
 }

+void CONSOLE_INFORMATION::SetActiveOutputBuffer(SCREEN_INFORMATION& screenBuffer)
+{
+    if (pCurrentScreenBuffer)


Aha I was wondering how you were going to manage that. Makes sense.

miniksa · 2022-03-11T19:09:05Z

src/host/srvinit.cpp

    }

+    NTSTATUS Status = SetUpConsole(&p->ConsoleInfo, p->TitleLength, p->Title, p->CurDir, p->AppName);


Given that you cannot see how g.pRender is a dependency of SetUpConsole now... we should perhaps leave a comment specifying that this has to be the order of them because setting up the console will go look it up and it shouldn't be reordered carelessly.

Yeah, that's a good point. I've added some comments here now.

miniksa · 2022-03-11T19:09:29Z

src/host/srvinit.cpp

+        return Status;
+    }
+
+    // Allow the renderer to paint.


Or clarify further "Allow it to paint after the rest of the console has been hooked up."

DHowett · 2022-03-12T00:00:56Z

I'm sorry, we made IRenderTarget slightly worse in #12358. We're trying to make sure accessibility is buttoned up for the 1.12 inbox release (for Windows 11 x2 (not a marketing name)), and I had to choose to merge that one instead of this one into main. I know that complicates this PR. ☹️

j4james · 2022-03-12T16:18:36Z

I'm sorry, we made IRenderTarget slightly worse in #12358.

No problem. It's wasn't a big deal to merge, and I suspect it could have been a lot worse going in the other direction.

zadjii-msft

On one hand: I'm a bit sad that the nice layering I had done three years ago to have the TextBuffer lib totally independent of the Renderer lib is now gone. On the other hand, I totally get why, and after 2+ years of the Terminal, it's clear why we never needed that in the first place.

Like 90% of this was just test fixes so the review really wasn't so bad.

ghost · 2022-03-14T21:58:50Z

Hello @lhecker!

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.

ghost · 2022-05-24T17:04:10Z

🎉Windows Terminal Preview v1.14.143 has been released which incorporates this pull request.:tada:

Handy links:

ghost added Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Issue-Task It's a feature request, but it doesn't really need a major design. Product-Conhost For issues in the Console codebase Product-Terminal The new Windows Terminal. labels Feb 24, 2022

j4james marked this pull request as ready for review February 24, 2022 16:44

j4james added 6 commits March 10, 2022 15:16

Track active buffer status in TextBuffer.

fd38db6

Get rid of SCREEN_INFORMATION::GetRenderTarget.

beb0963

Avoid using GetRenderTarget when triggering redraws.

167c5f8

Replace IRenderTarget with Renderer in TextBuffer.

d53f509

Get the unit tests working.

a603951

Remove remaining usage of IRenderTarget.

a5cc853

j4james force-pushed the nuke-rendertarget branch from e55a7b1 to a5cc853 Compare March 10, 2022 16:00

miniksa approved these changes Mar 11, 2022

View reviewed changes

miniksa requested a review from lhecker March 11, 2022 19:11

j4james added 2 commits March 12, 2022 02:35

Merge branch 'main' into nuke-rendertarget

b3fb6e8

Add comments clarifying the order of initialization.

76e4487

zadjii-msft approved these changes Mar 14, 2022

View reviewed changes

lhecker approved these changes Mar 14, 2022

View reviewed changes

lhecker added the AutoMerge Marked for automatic merge by the bot when requirements are met label Mar 14, 2022

ghost merged commit ce6ce50 into microsoft:main Mar 14, 2022

j4james deleted the nuke-rendertarget branch March 22, 2022 12:33

ghost mentioned this pull request May 24, 2022

Can we get rid of the IRenderTarget interface? #12551

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Eliminate the IRenderTarget interface #12568

Eliminate the IRenderTarget interface #12568

j4james commented Feb 24, 2022 •

edited

Loading

j4james commented Feb 24, 2022

miniksa left a comment

miniksa Mar 11, 2022

j4james Mar 12, 2022

miniksa Mar 11, 2022

miniksa Mar 11, 2022

j4james Mar 12, 2022

miniksa Mar 11, 2022

DHowett commented Mar 12, 2022

j4james commented Mar 12, 2022

zadjii-msft left a comment

ghost commented Mar 14, 2022

ghost commented May 24, 2022

		}

		NTSTATUS Status = SetUpConsole(&p->ConsoleInfo, p->TitleLength, p->Title, p->CurDir, p->AppName);

Eliminate the IRenderTarget interface #12568

Eliminate the IRenderTarget interface #12568

Conversation

j4james commented Feb 24, 2022 • edited Loading

Summary of the Pull Request

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

j4james commented Feb 24, 2022

miniksa left a comment

Choose a reason for hiding this comment

miniksa Mar 11, 2022

Choose a reason for hiding this comment

j4james Mar 12, 2022

Choose a reason for hiding this comment

miniksa Mar 11, 2022

Choose a reason for hiding this comment

miniksa Mar 11, 2022

Choose a reason for hiding this comment

j4james Mar 12, 2022

Choose a reason for hiding this comment

miniksa Mar 11, 2022

Choose a reason for hiding this comment

DHowett commented Mar 12, 2022

j4james commented Mar 12, 2022

zadjii-msft left a comment

Choose a reason for hiding this comment

ghost commented Mar 14, 2022

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.

ghost commented May 24, 2022

j4james commented Feb 24, 2022 •

edited

Loading

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.