-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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.
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
Scale retro terminal scan lines #4716
Conversation
YES THANK YOU |
You can add |
You're welcome! Tagged the screen edge issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea I'm cool with this, even if I don't totally understand what that sizeof
math is doing.
src/renderer/dx/DxRenderer.cpp
Outdated
@@ -285,12 +285,24 @@ HRESULT DxEngine::_SetupTerminalEffects() | |||
|
|||
RETURN_IF_FAILED(_d3dDevice->CreateBuffer(&bd, &InitData, &_screenQuadVertexBuffer)); | |||
|
|||
D3D11_BUFFER_DESC pixelShaderSettingsBufferDesc{}; | |||
pixelShaderSettingsBufferDesc.Usage = D3D11_USAGE_DEFAULT; | |||
pixelShaderSettingsBufferDesc.ByteWidth = sizeof(_pixelShaderSettings) + (16 - sizeof(_pixelShaderSettings) % 16) % 16; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay I don't know DX that well but what the heck is going on here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(that "multiples of 16" thing is documented in the docs for CreateBuffer
for when you pass D3D11_BIND_CONSTANT_BUFFER
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing that out, @zadjii-msft !
samplerDesc.AddressU = D3D11_TEXTURE_ADDRESS_WRAP; | ||
samplerDesc.AddressV = D3D11_TEXTURE_ADDRESS_WRAP; | ||
samplerDesc.AddressW = D3D11_TEXTURE_ADDRESS_WRAP; | ||
samplerDesc.AddressU = D3D11_TEXTURE_ADDRESS_CLAMP; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
THANK YOU
@msftbot make sure @miniksa signs off on this |
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:
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". |
src/renderer/dx/DxRenderer.cpp
Outdated
@@ -285,12 +285,24 @@ HRESULT DxEngine::_SetupTerminalEffects() | |||
|
|||
RETURN_IF_FAILED(_d3dDevice->CreateBuffer(&bd, &InitData, &_screenQuadVertexBuffer)); | |||
|
|||
D3D11_BUFFER_DESC pixelShaderSettingsBufferDesc{}; | |||
pixelShaderSettingsBufferDesc.Usage = D3D11_USAGE_DEFAULT; | |||
pixelShaderSettingsBufferDesc.ByteWidth = sizeof(_pixelShaderSettings) + (16 - sizeof(_pixelShaderSettings) % 16) % 16; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(that "multiples of 16" thing is documented in the docs for CreateBuffer
for when you pass D3D11_BIND_CONSTANT_BUFFER
)
@msftbot make sure @miniksa signs off |
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:
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". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?) <!-- grayscale, cleartype --> ![image](https://user-images.githubusercontent.com/18356694/75173864-28d14100-56f5-11ea-8bdd-d47a60fbbe4d.png)
@@ -74,7 +78,7 @@ float4 main(float4 pos : SV_POSITION, float2 tex : TEXCOORD) : SV_TARGET | |||
|
|||
// TODO:GH#3930 Make these configurable in some way. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well this TODO is going to be a lot easier now that you piped through the settings buffer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wonderful, thank you.
…into dotpaul-scaleScanLines
I pushed a merge to your branch so we could land it 😄 |
Ah, thank you @DHowett-MSFT ! |
Summary of the Pull Request
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
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