Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix conhost UseDx mode #10770

Merged
merged 1 commit into from
Jul 23, 2021
Merged

Fix conhost UseDx mode #10770

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions src/interactivity/win32/window.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,6 @@

#if TIL_FEATURE_CONHOSTDXENGINE_ENABLED
#include "../../renderer/dx/DxRenderer.hpp"
#else
// Forward-declare this so we don't blow up later.
struct DxEngine;
#endif

#include "../inc/ServiceLocator.hpp"
Expand Down Expand Up @@ -214,7 +211,9 @@ void Window::_UpdateSystemMetrics() const

const bool useDx = pSettings->GetUseDx();
GdiEngine* pGdiEngine = nullptr;
#if TIL_FEATURE_CONHOSTDXENGINE_ENABLED
[[maybe_unused]] DxEngine* pDxEngine = nullptr;
#endif
try
{
#if TIL_FEATURE_CONHOSTDXENGINE_ENABLED
Expand Down
24 changes: 10 additions & 14 deletions src/renderer/dx/DxRenderer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,6 @@ using namespace Microsoft::Console::Types;
// TODO GH 2683: The default constructor should not throw.
DxEngine::DxEngine() :
RenderEngineBase(),
_invalidateFullRows{ true },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you pull this? I presume the answer is:

  1. No one ever set this so we'll always just invalidate a full row.
  2. We determined that we likely won't even need to do differential drawing with a better render strategy anyway so there's no reason to try to tighten down the differential drawing any further in the future by doing partial rows.
    If that's the answer... maybe leave some of that in a comment in InvalidateRectangle so we don't forget.

Copy link
Member Author

@lhecker lhecker Jul 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah basically that. I was touching the only function that uses this field and I doubt this field will ever be useful again, as it's slower to figure out what contains ligatures, compared to just redrawing the entire row. The difference between drawing no text and a full row of text is only ~5μs after all. (Also: YAGNI. 😄)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough.

_pool{ til::pmr::get_default_resource() },
_invalidMap{ &_pool },
_invalidScroll{},
Expand Down Expand Up @@ -583,6 +582,9 @@ try

_displaySizePixels = _GetClientSize();

_invalidMap.resize(_displaySizePixels / _fontRenderData->GlyphCell());
RETURN_IF_FAILED(InvalidateAll());

// Get the other device types so we have deeper access to more functionality
// in our pipeline than by just walking straight from the D3D device.

Expand Down Expand Up @@ -963,8 +965,6 @@ CATCH_RETURN()
try
{
_sizeTarget = Pixels;

_invalidMap.resize(_sizeTarget / _fontRenderData->GlyphCell(), true);
return S_OK;
}
CATCH_RETURN();
Expand Down Expand Up @@ -1047,14 +1047,10 @@ HANDLE DxEngine::GetSwapChainHandle()

void DxEngine::_InvalidateRectangle(const til::rectangle& rc)
{
auto invalidate = rc;

if (_invalidateFullRows)
{
invalidate = til::rectangle{ til::point{ static_cast<ptrdiff_t>(0), rc.top() }, til::size{ _invalidMap.size().width(), rc.height() } };
}

_invalidMap.set(invalidate);
const auto size = _invalidMap.size();
const auto topLeft = til::point{ 0, std::min(size.height(), rc.top()) };
const auto bottomRight = til::point{ size.width(), std::min(size.height(), rc.bottom()) };
Comment on lines +1051 to +1052
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sure @miniksa will agree:

i am surprised we do not have til::rectangle::clamp

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean we do technically have an operator&, but I found it cumbersome to first construct a rectangle (which is rather verbose) just to do & rc.

Copy link
Member

@DHowett DHowett Jul 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only somewhat verbose 😄

const auto size = _invalidMap.size();
_invalidMap.set(til::rectangle{ 0, rc.top(), size.width(), rc.bottom() } & til::rectangle{ size });

okay I admit, it does look rather cumbersome.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally prefer Dustin's, but I'll take either. The structures and operators are tools to make life easier... not a strict requirement.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm reading this correctly, there isn't a strong opinion... as I'd like to keep it the way I did it for now, if you don't mind. 😅

_invalidMap.set({ topLeft, bottomRight });
Comment on lines +1050 to +1053
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Due to the missing std::min, but the delayed _invalidMap resize, this was blowing up with exceptions. Now it isn't anymore.

}

bool DxEngine::_IsAllInvalid() const noexcept
Expand Down Expand Up @@ -1273,7 +1269,7 @@ try
// so the entire frame is repainted.
if (_FullRepaintNeeded())
{
_invalidMap.set_all();
RETURN_IF_FAILED(InvalidateAll());
}

if (TraceLoggingProviderEnabled(g_hDxRenderProvider, WINEVENT_LEVEL_VERBOSE, TIL_KEYWORD_TRACE))
Expand Down Expand Up @@ -1322,8 +1318,8 @@ try
// And persist the new size.
_displaySizePixels = clientSize;

// Mark this as the first frame on the new target. We can't use incremental drawing on the first frame.
_firstFrame = true;
_invalidMap.resize(clientSize / _fontRenderData->GlyphCell());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused -- the body of the PR says "responsibility for resizing _invalidMap has been moved entirely into InvalidateAll", but both places we have InvalidateAll calls we resize the map before calling it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woops I mixed things up. I'm fixing my PR body to say that responsibility for full invalidation was moved there.

RETURN_IF_FAILED(InvalidateAll());
}

_d2dDeviceContext->BeginDraw();
Expand Down
1 change: 0 additions & 1 deletion src/renderer/dx/DxRenderer.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,6 @@ namespace Microsoft::Console::Render
uint16_t _hyperlinkHoveredId;

bool _firstFrame;
bool _invalidateFullRows;
std::pmr::unsynchronized_pool_resource _pool;
til::pmr::bitmap _invalidMap;
til::point _invalidScroll;
Expand Down