Fix conhost UseDx mode #10770
Fix conhost UseDx mode #10770
Conversation
ghost
added
Area-Rendering
| 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()) }; | ||
| _invalidMap.set({ topLeft, bottomRight }); |
There was a problem hiding this comment.
Due to the missing std::min, but the delayed _invalidMap resize, this was blowing up with exceptions. Now it isn't anymore.
| @@ -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()); | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Woops I mixed things up. I'm fixing my PR body to say that responsibility for full invalidation was moved there.
| 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()) }; |
There was a problem hiding this comment.
I'm sure @miniksa will agree:
i am surprised we do not have til::rectangle::clamp
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I personally prefer Dustin's, but I'll take either. The structures and operators are tools to make life easier... not a strict requirement.
There was a problem hiding this comment.
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. 😅
ghost
added
Needs-Author-Feedback
|
Make sure you test unusual scenarios too, like doing a lot of resizing in WT, with multiple panes, and moving between displays of different DPIs (or changing your active DPI) |
I can't spot anything unusual. 👍 |
| @@ -61,7 +61,6 @@ using namespace Microsoft::Console::Types; | |||
| // TODO GH 2683: The default constructor should not throw. | |||
| DxEngine::DxEngine() : | |||
| RenderEngineBase(), | |||
| _invalidateFullRows{ true }, | |||
There was a problem hiding this comment.
Why did you pull this? I presume the answer is:
- No one ever set this so we'll always just invalidate a full row.
- 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 inInvalidateRectangleso we don't forget.
There was a problem hiding this comment.
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. 😄)
This commit fixes the UseDx mode for conhost.
In order to add support for UseDx without calling
SetWindowSize,responsibility for resizing
_invalidMaphas been moved to occuronly when the renderer itself recognizes a new size. Furthermore
InvalidateAllis now the central point to invalidate_invalidMap.Validation Steps Performed
UseDxenables the DxEngine for conhost ✔️Closes #5455