Fix shader time input#8994
Conversation
zadjii-msft
left a comment
There was a problem hiding this comment.
Seems right to me, thanks!
|
Wow! Do you want to check in a sample shader (that you authored/have the rights to submit) to the samples/PixelShaders folder? That might be helpful for future users! |
Sure. I'm still newish to writing shaders but I've learned a fair bit playing around so far. I imagine there will be other users like me who are new to shaders and could use a little extra help. I'm not suggesting a full-on guide to writing shaders, but I think I could write a couple of simple animated ones and maybe improve the comments on some of the existing ones to clear up some things I initially found confusing. If that sounds good, I'll work on it and submit it in a separate PR for review. |
That would be wonderful in another PR. Thank you in advance and thanks for your contribution here. I'll set it to merge and I'll try to unlock the timer problem for you today. |
|
Hello @miniksa! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
<!-- 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 - Adds two simple animated shaders to the pixel shaders sample folder - Updates the readme in the pixel shaders sample folder to add a section explaining the animated shaders - Modifies some comments in existing shader samples <!-- Please review the items on the PR checklist before submitting--> ## PR Checklist * [ ] Closes #xxx * [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [ ] Tests added/passed * [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx * [ ] Schema updated. * [x] 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: #8994 <!-- 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 The two shaders I wrote are not especially pretty or interesting, but they should hopefully serve as simple examples for anyone looking to do animated effects. One simply draws a line of inverted pixels that scrolls down the screen, and the other fades the background back and forth between two colors. I've added a new section to the readme explaining how the shaders work to achieve animated effects. I've also updated the comments on the existing shaders to clear up a couple of things: 1. Be more explicit that `Time` represents seconds since the shader loaded. Though obvious in hindsight, this was not clear to me when I was first learning/experimenting 2. Explain that `tex` ranges from 0,0 to 1,1. This is important because, when trying to port GLSL shaders from shadertoy, I at first assumed `fragCoord` and `tex` are the same thing but the former actually ranges from 0,0 to the resolution of the canvas, so some of the math doesn't work out if you just substitute it with `tex`. Any and all feedback welcome. <!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well --> ## Validation Steps Performed I ran the shaders manually in a dev build of the Terminal. I auto-spellchecked and manually proofread my additions to the readme and verifed the markdown rendering on github.
Correctly sets the time input on the pixelShaderSettings struct, which was previously hard-coded to `0.0f`. ## PR Checklist * [x] Closes #8935 * [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [x] 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: #8935 ## Detailed Description of the Pull Request / Additional comments I added a private field to `DxEngine` to store the timestamp for when a custom shader is first loaded. The field is initialized in `_SetupTerminalEffects()`, and the calculated time value (seconds since the timestamp) passed to the actual shader is set in `_ComputePixelShaderSettings()`. There remains an issue with with jerky animation due to the renderer not repainting when the window contents are not updated (see discussion in the original issue). This is basically my first time writing C++; constructive review is enthusiastically welcomed 🙂 ## Validation Steps Performed I manually tested using a variety of simple shaders that rely on time input for animation. (cherry picked from commit 9fb4fb2)
Teaches renderer base to keep thread alive if engine requests it. `DxEngine` now requests it if shaders are on. - The render engine interface now has a true/false to return whether the specific renderer wants another frame to immediately follow up. The renderer base will ask for this information as it ends the paint on any particular engine (which is the time where invalid regions are typically cleaned up) and just poke the render thread the same as if an invalidation request came in from outside of render-land. That will trigger the render thread to just keep moving in the same way as any other invalidation. ## Validation Steps Performed - [x] Actually built it - [x] Actually try it I promised this in #8994
Teaches renderer base to keep thread alive if engine requests it. `DxEngine` now requests it if shaders are on. - The render engine interface now has a true/false to return whether the specific renderer wants another frame to immediately follow up. The renderer base will ask for this information as it ends the paint on any particular engine (which is the time where invalid regions are typically cleaned up) and just poke the render thread the same as if an invalidation request came in from outside of render-land. That will trigger the render thread to just keep moving in the same way as any other invalidation. ## Validation Steps Performed - [x] Actually built it - [x] Actually try it I promised this in #8994 (cherry picked from commit 3b24781)
|
🎉 Handy links: |
Correctly sets the time input on the pixelShaderSettings struct, which was previously hard-coded to
0.0f.PR Checklist
Detailed Description of the Pull Request / Additional comments
I added a private field to
DxEngineto store the timestamp for when a custom shader is first loaded. The field is initialized in_SetupTerminalEffects(), and the calculated time value (seconds since the timestamp) passed to the actual shader is set in_ComputePixelShaderSettings().There remains an issue with with jerky animation due to the renderer not repainting when the window contents are not updated (see discussion in the original issue).- Fixed in #9091This is basically my first time writing C++; constructive review is enthusiastically welcomed 🙂
Validation Steps Performed
I manually tested using a variety of simple shaders that rely on time input for animation.