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

Remove implicit scaling to improve performance and crispness of High DPI text #5320

Closed
miniksa opened this issue Apr 10, 2020 · 1 comment · Fixed by #5345
Closed

Remove implicit scaling to improve performance and crispness of High DPI text #5320

miniksa opened this issue Apr 10, 2020 · 1 comment · Fixed by #5345
Assignees
@miniksa
Labels
Area-Performance Performance-related issue Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone
Terminal v1.x

Comments

@miniksa
Copy link
Member

miniksa commented Apr 10, 2020

In high DPI, we have to disable differential drawing.
This is because the text is drawn at fractional pixel heights and baselines automatically as we're scaling the entire d2d render target. The fractional drawing is also making it look a little bit off for High DPI customers.

So doing the work of removing the implicit scaling and installing our own transforms on the origins and sizes of the five things we draw should resolve this issue:

  1. Clear background rectangles
  2. The cursor
  3. Selection rectangles
  4. The background color behind text
  5. Text runs
@miniksa miniksa added Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Area-Performance Performance-related issue Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. labels Apr 10, 2020
@ghost ghost added the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Apr 10, 2020
@miniksa miniksa mentioned this issue Apr 10, 2020
Merged
5 tasks
@DHowett-MSFT DHowett-MSFT removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Apr 10, 2020
@DHowett-MSFT DHowett-MSFT added this to the Terminal v1.0 milestone Apr 10, 2020
@miniksa miniksa mentioned this issue Apr 13, 2020
Merged
9 tasks
@ghost ghost added the In-PR This issue has a related PR label Apr 13, 2020
@miniksa miniksa removed the In-PR This issue has a related PR label Apr 15, 2020
@miniksa miniksa modified the milestones: Terminal v1.0, Terminal v1.x Apr 15, 2020
@ghost ghost added Needs-Tag-Fix Doesn't match tag requirements Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. labels Apr 22, 2020
miniksa added a commit that referenced this issue Apr 22, 2020
@miniksa @DHowett @zadjii-msft
Adjusts High DPI scaling to enable differential rendering (#5345)
8ea9b32 
## 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 #5345 (comment)

Co-authored-by: Dustin Howett <duhowett@microsoft.com>
Co-authored-by: Mike Griese <migrie@microsoft.com>
@zadjii-msft zadjii-msft mentioned this issue Apr 24, 2020
Closed
@ghost
Copy link

ghost commented May 5, 2020

🎉This issue was addressed in #5345, which has now been successfully released as Windows Terminal Release Candidate v0.11.1251.0 (1.0rc1).:tada:

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@miniksa miniksa

Labels
Area-Performance Performance-related issue Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Milestone
Terminal v1.x
Development

Successfully merging a pull request may close this issue.

Adjusts High DPI scaling to enable differential rendering microsoft/terminal
2 participants
@DHowett-MSFT @miniksa