-
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
Add experimental retro terminal effects #3468
Conversation
Maybe this kind of "visual effect" could become an extension in the future, so others can "plug-in" their own effects or shaders for Windows Terminal |
Oh my god this is so much fun. I do like the idea that this might be a good type of extension. I'll discuss with the team, but this might be complete enough that we'd want to ship it with the Terminal (at least for 1.0). However, I definitely wouldn't feel comfortable merging this without hiding it behind a setting (probably per-profile) to make it optional. |
OK I love it. I want it in. @zadjii-msft, we should make a list of things that could be pulled into extensions in the future (this, azure cloud shell, etc.) to make them easier to update/swap. But I don't think we should be gating cool and interesting functionality on the lack of an extension framework for v1.0. |
@ironyman, are you in any way related to /u/1r0nyman who "YOLO"d his way through box spreads on /r/wallstreetbets or is this just a coincidence? |
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.
Excellent idea. We're also going to need some configuration plumbed through of course. I left a batch of comments on how it's gone so far.
src/renderer/dx/DxRenderer.cpp
Outdated
} | ||
)"; | ||
|
||
const char _pixelShaderString[] = R"( |
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 haven't done shaders before, in earnest, but isn't there a resource file type that shader code can be placed into and loaded instead of writing it as giant const strings in the source?
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 can move this to an .hlsl file that's built by visual studio. But how do I make sure the output is packaged with application so the application can find the compiled shader output at runtime?
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.
Does it not carry it automatically? I would think that adding a supported compilation thing should be carried through into the binaries and into the package. We might have to page @DHowett-MSFT if it isn't automatic.
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.
Nope
C:\Users\changyl\Documents\GitHub\terminal\src\renderer\dx\DxRenderer.cpp(211)\TerminalControl.dll!00007FF87A823678: (caller: 00007FF87A822E1C) ReturnHr(4) tid(3150) 80070002 The system cannot find the file specified.
[Microsoft::Console::Render::DxEngine::_SetupTerminalEffects(D3DReadFileToBlob(L"ScreenVertexShader.cso", &vertexBlob))]
PS C:\Users\changyl\Documents\GitHub\terminal> gci -Recurse -Filter "ScreenPixel*"
Directory: C:\Users\changyl\Documents\GitHub\terminal\bin\x64\Debug
Mode LastWriteTime Length Name
---- ------------- ------ ----
-a---- 11/8/2019 10:18 PM 30340 ScreenPixelShader.cso
Directory: C:\Users\changyl\Documents\GitHub\terminal\src\renderer\dx\lib
Mode LastWriteTime Length Name
---- ------------- ------ ----
-a---- 11/8/2019 10:10 PM 3563 ScreenPixelShader.hlsl
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.
Dustin said that the only way to package it into the UWP app right now is to put the hlsl files into another project. That's going to be kind of messy so he said he's OK with leaving it in the source code for now and fixing it later. What do you think?
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 think I'm okay with that, but let's compromise by defining the strings in some other header file and including them here. Does that sound reasonable?
God I wish |
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 don't really understand the DX-y bits, but I do understand that this is awesome.
Fundamentally, my only concern with this is that I'd like to have a setting in the terminal that enables/disables this mode. Doesn't need to be anything complicated, probably just a boolean setting in Profile
and IControlSettings
that's something like "retroTerminalEffect"
. Get the profile to deserialize that, pass it to the TerminalSettings, and have TermControl
set some property on the DxEngine to enable this. #3471 was just completed that does something very similar.
src/renderer/dx/DxRenderer.cpp
Outdated
return 0; | ||
} | ||
|
||
Microsoft::WRL::ComPtr<ID3DBlob> |
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.
We should maybe move this to some sort of DxUtils.h/cpp file... @miniksa thoughts?
Also, make sure to run |
a03cf64
to
61d703b
Compare
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.
Largely I think I'm okay with this. I'm not going to sign off quite yet since I left a bunch of nits, but nothing really worth blocking on. Two other people on the team can feel free to override me :)
Let's get @miniksa in here to review this as well.
src/host/exe/Host.EXE.vcxproj
Outdated
@@ -84,8 +84,9 @@ | |||
</ClCompile> | |||
<Link> | |||
<AllowIsolation>true</AllowIsolation> | |||
<AdditionalDependencies>d3dcompiler.lib</AdditionalDependencies> |
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.
There's probably a better place to put this, @miniksa knows where.
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.
(this one is still here too)
{ | ||
Texture2D input = shaderTexture; | ||
|
||
// TODO 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.
I can go through and file follow-up tasks for this
{ | ||
float wave = SquareWave(pos.y); | ||
|
||
// TODO make this configurable. |
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.
(we'll also want a follow-up on this one)
@@ -484,6 +484,8 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation | |||
// Tell the DX Engine to notify us when the swap chain changes. | |||
dxEngine->SetCallback(std::bind(&TermControl::SwapChainChanged, this)); | |||
|
|||
dxEngine->SetRetroTerminalEffects(_settings.RetroTerminalEffect()); |
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.
We probably also want to do this in UpdateSettings
, so that changing the setting will hot-reload the setting
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.
Can we make a task for this? We would need to do setup outside of normal dxEngine setup path to support hot reloading.
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.
Oh, that's a good point. I can make that follow-up
// Prepare shaders. | ||
auto vertexBlob = _CompileShader(screenVertexShaderString, "vs_5_0"); | ||
auto pixelBlob = _CompileShader(screenPixelShaderString, "ps_5_0"); | ||
// TODO: move the shader files to to hlsl files and package their |
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.
we should file a follow-up task for this
Huh, I thought commenting would clear my block. Apparently it doesn't.
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 love this. It is beautifully well-contained, non-intrusive, opt-in, and has good error handling. I love it to bits. Thank you.
One thing I'd like to request in addition to my couple of comments is that we (sorry, I know you had to do this once already) rename the setting.
Eventually, we will replace this with configurable shaders with, perhaps, configurable shader inputs. We're not quite there yet, of course, as you can well see. When we are, though, the option will look quite different from Profile::retroTerminalEffects
.
To clue users into the fact that their cheese might move, and somewhat secure us against their complaints, can we call the JSON key experimental.retroTerminalEffects
? (If @zadjii-msft doesn't like the .
, we can use _
, but this seems nicely namespaced)
<ItemDefinitionGroup> | ||
<ClCompile> | ||
<PrecompiledHeaderFile>pch.h</PrecompiledHeaderFile> | ||
</ClCompile> | ||
<Link> | ||
<AdditionalDependencies>d3dcompiler.lib</AdditionalDependencies> |
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.
nit: You may just want to add this to the common link line in common.pre.props
|
||
if (_retroTerminalEffects) | ||
{ | ||
const HRESULT hr = _SetupTerminalEffects(); |
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.
Does this need an operating system version guard? the WPF terminal control wants to work on Windows 7.
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 don't think there's any dependency on OS version. Just directx which the terminal already depends on.
Wow, beautiful work @ironyman ! |
@@ -1060,6 +1050,16 @@ void DxEngine::_InvalidOr(RECT rc) noexcept | |||
// - S_OK on success, E_PENDING to indicate a retry or a relevant DirectX error | |||
[[nodiscard]] HRESULT DxEngine::Present() noexcept | |||
{ | |||
if (_retroTerminalEffects) |
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.
Awesome! I bet this makes io feel a little faster since we're not doing computation under console-lock.
I tried rebasing but it's giving me a lot of these errors
|
@ironyman woah that's a crazy compilation error. I honestly don't know what's causing that - I'll check out your branch locally and try and merge it up to master and figure it out. |
@ironyman Okay so I just checked this branch out and merged master in, one release at a time. I haven't seen that error message at all unfortunately :/ You can check my version out in dev/migrie/merge-3468-and-master. I bet if you do a clean rebuild, it'll probably go away. I might also try navigating to the |
## Summary of the Pull Request The original PR had a few TODOs in it without issue numbers. IMO, this wasn't important enough to block the PR over. _Also I'm impatient and wanted that setting_. After I merged the PR I created the issues and added the numbers myself. ## References ## PR Checklist * [x] Closes nothing, this just adds a couple TODOs * [x] I work here * [x] this _really_ doesn't need tests * [x] This _is_ a docs update
🎉 Handy links: |
It's more fun than I expected |
Don't understand how you choose the effect; glow, scan lines, cool. Using "experimental.retroTerminalEffect": true seems to set the effect to scan lines. |
You don’t. It’s just one effect that has all three of those things. It glows, has scan lines, and is cool. |
Silly me. Thank you. |
I dont get it. I enabled the feature. See no difference |
@oskaremil If you're having a problem with the Terminal, you should probably search our list of issues to see if anyone else has the same problem as you. If not, could you file a new issue, so we can track and diagnose your issue specifically? |
Can't believe any of the CRTs I worked on ever looked this bad! |
I can't tell if you're being sarcastic or mean, so: if... |
Not mean! Nonono. I would love to use this for some of my long running console jobs, but it is right on the edge of tolerable. Are the effects tweakable at all, or is it all in the shaders? |
Alas! It's all in the shaders, and those aren't yet configurable. |
@oskaremil Make sure to open a new tab after enabling the effect. I don't think hot-reload works with this, probably because of how the shader is loaded. |
If/when the hot-reload works, it would be a hoot to set the property via a keystroke. |
I did. Will open a new issue for this. |
Very cool, loving it! But as @richardthombs said some configurability would be nice as this markedly decreases legibility. Leaving it on anyway! :) |
@oskaremil I also did not see any difference, but after a few minutes I figured out that "experimental.retroTerminalEffect": true has to be placed inside of a profile object in profiles.json. |
So you added a d3d (dx11) dependancy for this feature, that everybody turns off after 10 seconds and never use it again? |
@venimus perhaps there’s a nicer way to get your message across? |
Sorry but this is my nicer way. IMHO this feature is even less useful than that. There are already fonts that include the scanlines effect (although not affecting the background). However being already there, you could improve the nostalgia filter further:
eg: http://www.mclelun.com/2018/10/after-effects-vintage-terminal-console.html |
I turned it on the day it came out and been using it ever since. For hours every day. Apparently, there's no accounting for taste. |
"retroTerminalEffect": { | ||
"description": "When set to true, enable retro terminal effects.", | ||
"type": "boolean" | ||
} |
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.
Should be a comma 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.
Looks like this was already fixed
terminal/doc/cascadia/profiles.schema.json
Lines 535 to 538 in 82f302b
"experimental.retroTerminalEffect": { | |
"description": "When set to true, enable retro terminal effects. This is an experimental feature, and its continued existence is not guaranteed.", | |
"type": "boolean" | |
}, |
@ironyman this is so great! thank you!! I've been using it since i heard about it, with #00FF00 on all colors for the old monochrome green look :) I have the same experience as @pedrolamas though, on a screen with scaling effect it's barely noticeable (i have 300%, so even harder to see). I went down a rabbit-hole of thinking it was the GPU acceleration that was turned off on the laptop monitor but not on the external monitor, but it is indeed the scaling that makes it do the scan lines really tight. |
<!-- Enter a brief description/summary of your PR here. What does it fix/what does it change/how was it tested (even manually, if necessary)? --> ## Summary of the Pull Request - Scale the 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%: ![Scaling scan lines](https://user-images.githubusercontent.com/38924837/75214566-df0f4780-5742-11ea-9bdc-3430eb24ccca.png) Before & after showing artifact removal, with my display scale set to 100%, and image enlarged to 400%: ![Sampling artifacts annotated](https://user-images.githubusercontent.com/38924837/75214618-05cd7e00-5743-11ea-9060-f4eba257ea56.png) <!-- Please review the items on the PR checklist before submitting--> ## PR Checklist * [x] Closes #4362 * [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) 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 <!-- Provide a more detailed description of the PR, other things fixed or any additional comments/features here --> ## 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. 🙂 <!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well --> ## 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.
Thank you for these effects. 👍 They remind me of this other project: Cool Retro Term. |
Summary of the Pull Request
Cool retro terminal effects
References
PR Checklist
Detailed Description of the Pull Request / Additional comments
Validation Steps Performed