Scale retro terminal scan lines by dotpaul · Pull Request #4716 · microsoft/terminal

dotpaul · 2020-02-25T04:38:18Z

Summary of the Pull Request

Scale the retro terminal effects (Add experimental retro terminal effects #3468) scan lines with the screen's DPI.
Remove artifacts from sampling wrap around.

Before & after, with my display scale set to 350%:

Before & after showing artifact removal, with my display scale set to 100%, and image enlarged to 400%:

PR Checklist

Closes Retro terminal effect prodces artifacts on the right screen edge #4362
CLA signed. If not, go over here and sign the CLA
Tests added/passed
Requires documentation to be updated
I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Detailed Description of the Pull Request / Additional comments

Adds a constant buffer, which could be used for other settings for the retro terminal pixel shader.

I haven't touched C++ in over a decade before this change, and this is the first time I've played with DirectX, so please assume my code isn't exactly best practice. 🙂

Validation Steps Performed

Changed display scale with experimental.retroTerminalEffect enabled, enjoyed scan lines on high resolution monitors.
Enabled experimental.retroTerminalEffect, turned the setting off, changed display scale. Retro tabs still scale scan lines.

DHowett-MSFT · 2020-02-25T04:50:39Z

YES THANK YOU

DHowett-MSFT · 2020-02-25T04:51:34Z

You can add Closes #4362 for the screen edge issue

dotpaul · 2020-02-25T05:23:05Z

You're welcome! Tagged the screen edge issue.

zadjii-msft

Yea I'm cool with this, even if I don't totally understand what that sizeof math is doing.

zadjii-msft · 2020-02-25T13:32:00Z

src/renderer/dx/DxRenderer.cpp


+    D3D11_BUFFER_DESC pixelShaderSettingsBufferDesc{};
+    pixelShaderSettingsBufferDesc.Usage = D3D11_USAGE_DEFAULT;
+    pixelShaderSettingsBufferDesc.ByteWidth = sizeof(_pixelShaderSettings) + (16 - sizeof(_pixelShaderSettings) % 16) % 16;


Okay I don't know DX that well but what the heck is going on here

I believe this is "nearest larger multiple of 16"

libra  ~  %  $n=9 libra  ~  %  $n + (16 - $n % 16) % 16 16 libra  ~  %  $n=17 libra  ~  %  $n + (16 - $n % 16) % 16 32

(that "multiples of 16" thing is documented in the docs for CreateBuffer for when you pass D3D11_BIND_CONSTANT_BUFFER)

Right. Thinking about this more, I want to pad the struct instead. As it is now, I would assume DirectX is reading beyond the struct's memory when initializing / updating the constant buffer, which would make me feel bad.

Thanks for pointing that out, @zadjii-msft !

zadjii-msft · 2020-02-25T13:32:24Z

src/renderer/dx/DxRenderer.cpp

-    samplerDesc.AddressU = D3D11_TEXTURE_ADDRESS_WRAP;
-    samplerDesc.AddressV = D3D11_TEXTURE_ADDRESS_WRAP;
-    samplerDesc.AddressW = D3D11_TEXTURE_ADDRESS_WRAP;
+    samplerDesc.AddressU = D3D11_TEXTURE_ADDRESS_CLAMP;


zadjii-msft · 2020-02-25T13:34:00Z

@msftbot make sure @miniksa signs off on this

ghost · 2020-02-25T13:34:05Z

Hello @zadjii-msft!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

I'll only merge this pull request if it's approved by @miniksa

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

DHowett-MSFT · 2020-02-25T18:04:02Z

src/renderer/dx/DxRenderer.cpp


+    D3D11_BUFFER_DESC pixelShaderSettingsBufferDesc{};
+    pixelShaderSettingsBufferDesc.Usage = D3D11_USAGE_DEFAULT;
+    pixelShaderSettingsBufferDesc.ByteWidth = sizeof(_pixelShaderSettings) + (16 - sizeof(_pixelShaderSettings) % 16) % 16;


(that "multiples of 16" thing is documented in the docs for CreateBuffer for when you pass D3D11_BIND_CONSTANT_BUFFER)

src/renderer/dx/DxRenderer.cpp

DHowett-MSFT · 2020-02-25T20:32:50Z

@msftbot make sure @miniksa signs off

ghost · 2020-02-25T20:32:54Z

Hello @DHowett-MSFT!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

I'll only merge this pull request if it's approved by @miniksa

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

DHowett-MSFT

I'm flipping the sense on my approval bit because I learned that the HRESULT thing isn't actually true.

## Summary of the Pull Request I needed to do something to keep sane so today I day of learned about antialiasing. This PR adds the ability to specify the `"antialiasingMode"` as a setting. * "antialiasingMode": "grayscale": the current behavior, `D2D1_TEXT_ANTIALIAS_MODE_GRAYSCALE` * "antialiasingMode": "cleartype": use `D2D1_TEXT_ANTIALIAS_MODE_CLEARTYPE` instead ## PR Checklist * [x] Closes microsoft#1298 * [x] I work here * [ ] I didn't add tests * [x] Requires documentation to be updated ## Detailed Description of the Pull Request / Additional comments Grayscale: ![image](https://user-images.githubusercontent.com/18356694/75173847-2373f680-56f5-11ea-8896-c1cf04c61d41.png) Cleartype: ![image](https://user-images.githubusercontent.com/18356694/75173854-25d65080-56f5-11ea-9de1-e2d1c343cae5.png) Side-by-side (can you tell which is which?)  ![image](https://user-images.githubusercontent.com/18356694/75173864-28d14100-56f5-11ea-8bdd-d47a60fbbe4d.png)

miniksa · 2020-02-25T23:34:10Z

src/renderer/dx/ScreenPixelShader.h

@@ -74,7 +78,7 @@ float4 main(float4 pos : SV_POSITION, float2 tex : TEXCOORD) : SV_TARGET

    // TODO:GH#3930 Make these configurable in some way.


Well this TODO is going to be a lot easier now that you piped through the settings buffer.

miniksa

Wonderful, thank you.

…into dotpaul-scaleScanLines

DHowett-MSFT · 2020-02-25T23:52:34Z

I pushed a merge to your branch so we could land it 😄

dotpaul · 2020-02-25T23:54:53Z

I pushed a merge to your branch so we could land it 😄

Ah, thank you @DHowett-MSFT !

dotpaul added 3 commits February 23, 2020 05:01

Scale retro scan lines initial

7b604bd

Just put the pixel shader settings into its own member struct

6708b77

Undo unintended formatting changes

de21767

Trying to fix static analysis warnings

2c66231

dotpaul added 2 commits February 24, 2020 21:32

Oops, indenting

a887ca6

Invoke-CodeFormat'd

21bc523

zadjii-msft approved these changes Feb 25, 2020

View reviewed changes

ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Feb 25, 2020

zadjii-msft added Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Product-Terminal The new Windows Terminal. labels Feb 25, 2020

DHowett-MSFT suggested changes Feb 25, 2020

View reviewed changes

ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Feb 25, 2020

Pad _pixelShaderSettings

d57ceab

ghost removed the AutoMerge Marked for automatic merge by the bot when requirements are met label Feb 25, 2020

ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Feb 25, 2020

DHowett-MSFT approved these changes Feb 25, 2020

View reviewed changes

miniksa reviewed Feb 25, 2020

View reviewed changes

miniksa approved these changes Feb 25, 2020

View reviewed changes

Merge branch 'scaleScanLines' of https://github.com/dotpaul/terminal …

494682c

…into dotpaul-scaleScanLines

ghost merged commit de5e72f into microsoft:master Feb 26, 2020

dotpaul deleted the scaleScanLines branch February 26, 2020 19:50

zadjii-msft mentioned this pull request Mar 9, 2020

CRT effect bleeds from right side when terminal window padded #4844

Closed

This pull request was closed.

		@@ -74,7 +78,7 @@ float4 main(float4 pos : SV_POSITION, float2 tex : TEXCOORD) : SV_TARGET

		// TODO:GH#3930 Make these configurable in some way.

Conversation

dotpaul commented Feb 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of the Pull Request

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Uh oh!

DHowett-MSFT commented Feb 25, 2020

Uh oh!

DHowett-MSFT commented Feb 25, 2020

Uh oh!

dotpaul commented Feb 25, 2020

Uh oh!

zadjii-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zadjii-msft commented Feb 25, 2020

Uh oh!

ghost commented Feb 25, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DHowett-MSFT commented Feb 25, 2020

Uh oh!

ghost commented Feb 25, 2020

Uh oh!

DHowett-MSFT left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miniksa left a comment

Choose a reason for hiding this comment

Uh oh!

DHowett-MSFT commented Feb 25, 2020

Uh oh!

dotpaul commented Feb 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dotpaul commented Feb 25, 2020 •

edited

Loading