Adjusts High DPI scaling to enable differential rendering #5345
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…X renderer now to know what the signal means and figure it out.
…iewport change. Add comments to the TriggerScroll calls to explain why each was chosen.
…d on as the shader is applied at a late stage in the pipeline and causes an unexpected graphics effect with differential drawing.
… the old Present method.
…ove MUL/DIV ones). Add scale test to point.
…implemented scale on til::size mostly as a copy of til::point's implementation and copied the test.)
…to maintain robustness.
…ure of union rect invalidation meant it was never a problem before.
…estriction area because it isn't populated until the first scroll operation.
…e bottom of characters on every other line when rendering High DPI text.
… render target scaling.
…nder the renderer while it's operating.
This was referenced Apr 20, 2020
* This seems to work, but can it avoid resizing the buffer twice on a DPI change? * Add a doc comment that should have been in the previous commit * Remove some dead code I didn't end up needing * convert TSFInputControl::_RedrawCanvas to use til wherever possible This is just here to help me debug * I think this works fine on high-dpi for latin, cyrillic, emoji, chinese * This _actaully_ works and displays the composition correctly. Needs comments * Clean up this code significantly * This does in fact work * Skip one of the two resizes during a DPI scale change. * This definitely fixes the gutters, but lets try to be more surgical about it * Add some comments * This fixes the 'vim open in an inactive tab' bug I was seeing, but I might be able to do it without reverting the double resize * More good good til helpers * rewrite TermControl::_GetTerminalPosition, this might all be wrong though. * Do pointer event scaling in DIPs * Largely cleanup, but also add a TODO as it's almost 5pm here * Remove unused code, comments about receiving a useless ScaleChanged event * Update the font size in response to a settings reload * Fix blanking vim on a settings update * good bot * Immediately tell the VT Engine about the new viewport size when we resize conpty * fix the bitmap::all() function so we can actually use it (cherry picked from commit ba1a8a34292180cae10c135743ecd94b1da6f286) * maybe I should let it finish building before I commit (cherry picked from commit 98cdbeca349acb55c5b5aef1d96206a8fa7b62e4) * Hey what's this doing here * Account for IME wrapping
| // doesn't reliably happen immediately after a scale change, so we can't | ||
| // depend on it (despite the fact that both the scale and size state is | ||
| // definitely correct in it) | ||
| // - In the 3rd event, we're going to update our font size for te new DPI. |
There was a problem hiding this comment.
Suggested change
| // - In the 3rd event, we're going to update our font size for te new DPI. | |
| // - In the 3rd event, we're going to update our font size for the new DPI. |
let's keep them, they're beautiful at 1dpx |
Yeah you two have the opinion here so I'm going with it. I'll leave it. |
|
This is glorious and robust and I love it. |
DHowett-MSFT
approved these changes
Apr 22, 2020
1 task
carlos-zamora
approved these changes
Apr 22, 2020
| clientCursorPos.Y = ::base::ClampMul(_currentTerminalCursorPos.y(), ::base::ClampedNumeric<ptrdiff_t>(fontHeight)); | ||
| // Convert text buffer cursor position to client coordinate position | ||
| // within the window. This point is in _pixels_ | ||
| const til::point clientCursorPos{ _currentTerminalCursorPos * fontSize }; |
There was a problem hiding this comment.
I've been out of the til::point/size loop for a bit, so correct me if I'm wrong. Can't this throw if we get an overflow?
Comment on lines
+196
to
+198
| // Make sure to unscale the font size to correct for DPI! XAML needs | ||
| // things in DIPs, and the fontSize is in pixels. | ||
| TextBlock().FontSize(fontSizePx / scaleFactor); |
There was a problem hiding this comment.
(unrelated to this PR, more for discussion) Should we eventually do what we did with COORD and create two new til structs to always know when we're in DIPs vs pixels? Or would that be overkill.
This was referenced Apr 29, 2020
|
🎉 Handy links: |
ghost
mentioned this pull request
donno2048
added a commit
to donno2048/terminal
that referenced
this pull request
Sep 28, 2020
## Summary of the Pull Request - Adjusts scaling practices in `DxEngine` (and related scaling practices in `TerminalControl`) for pixel-perfect row baselines and spacing at High DPI such that differential row-by-row rendering can be applied at High DPI. ## References - #5185 ## PR Checklist * [x] Closes #5320, closes #3515, closes #1064 * [x] I work here. * [x] Manually tested. * [x] No doc. * [x] Am core contributor. Also discussed with some of them already via Teams. ## Detailed Description of the Pull Request / Additional comments **WAS:** - We were using implicit DPI scaling on the `ID2D1RenderTarget` and running all of our processing in DIPs (Device-Independent Pixels). That's all well and good for getting things bootstrapped quickly, but it leaves the actual scaling of the draw commands up to the discretion of the rendering target. - When we don't get to explicitly choose exactly how many pixels tall/wide and our X/Y placement perfectly, the nature of floating point multiplication and division required to do the presentation can cause us to drift off slightly out of our control depending on what the final display resolution actually is. - Differential drawing cannot work unless we can know the exact integer pixels that need to be copied/moved/preserved/replaced between frames to give to the `IDXGISwapChain1::Present1` method. If things spill into fractional pixels or the sizes of rows/columns vary as they are rounded up and down implicitly, then we cannot do the differential rendering. **NOW:** - When deciding on a font, the `DxEngine` will take the scale factor into account and adjust the proposed height of the requested font. Then the remainder of the existing code that adjusts the baseline and integer-ifies each character cell will run naturally from there. That code already works correctly to align the height at normal DPI and scale out the font heights and advances to take an exact integer of pixels. - `TermControl` has to use the scale now, in some places, and stop scaling in other places. This has to do with how the target's nature used to be implicit and is now explicit. For instance, determining where the cursor click hits must be scaled now. And determining the pixel size of the display canvas must no longer be scaled. - `DxEngine` will no longer attempt to scale the invalid regions per my attempts in #5185 because the cell size is scaled. So it should work the same as at 96 DPI. - The block is removed from the `DxEngine` that was causing a full invalidate on every frame at High DPI. - A TODO was removed from `TermControl` that was invalidating everything when the DPI changed because the underlying renderer will already do that. ## Validation Steps Performed * [x] Check at 150% DPI. Print text, scroll text down and up, do selection. * [x] Check at 100% DPI. Print text, scroll text down and up, do selection. * [x] Span two different DPI monitors and drag between them. * [x] Giant pile of tests in microsoft/terminal#5345 (comment) Co-authored-by: Dustin Howett <duhowett@microsoft.com> Co-authored-by: Mike Griese <migrie@microsoft.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary of the Pull Request
DxEngine(and related scaling practices inTerminalControl) for pixel-perfect row baselines and spacing at High DPI such that differential row-by-row rendering can be applied at High DPI.References
PR Checklist
Detailed Description of the Pull Request / Additional comments
WAS:
ID2D1RenderTargetand running all of our processing in DIPs (Device-Independent Pixels). That's all well and good for getting things bootstrapped quickly, but it leaves the actual scaling of the draw commands up to the discretion of the rendering target.IDXGISwapChain1::Present1method. If things spill into fractional pixels or the sizes of rows/columns vary as they are rounded up and down implicitly, then we cannot do the differential rendering.NOW:
DxEnginewill take the scale factor into account and adjust the proposed height of the requested font. Then the remainder of the existing code that adjusts the baseline and integer-ifies each character cell will run naturally from there. That code already works correctly to align the height at normal DPI and scale out the font heights and advances to take an exact integer of pixels.TermControlhas to use the scale now, in some places, and stop scaling in other places. This has to do with how the target's nature used to be implicit and is now explicit. For instance, determining where the cursor click hits must be scaled now. And determining the pixel size of the display canvas must no longer be scaled.DxEnginewill no longer attempt to scale the invalid regions per my attempts in Render row-by-row instead of invalidating entire screen #5185 because the cell size is scaled. So it should work the same as at 96 DPI.DxEnginethat was causing a full invalidate on every frame at High DPI.TermControlthat was invalidating everything when the DPI changed because the underlying renderer will already do that.Validation Steps Performed