Skip to content

Fix shader time input#8994

Merged
1 commit merged intomicrosoft:mainfrom
Nacimota:8935-fix-shader-time
Feb 2, 2021
Merged

Fix shader time input#8994
1 commit merged intomicrosoft:mainfrom
Nacimota:8935-fix-shader-time

Conversation

@Nacimota
Copy link
Contributor

@Nacimota Nacimota commented Feb 1, 2021

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 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). - Fixed in #9091

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.

@ghost ghost added Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. labels Feb 1, 2021
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems right to me, thanks!

@DHowett
Copy link
Member

DHowett commented Feb 1, 2021

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!

@Nacimota
Copy link
Contributor Author

Nacimota commented Feb 2, 2021

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.

@miniksa
Copy link
Member

miniksa commented Feb 2, 2021

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.

@miniksa miniksa added the AutoMerge Marked for automatic merge by the bot when requirements are met label Feb 2, 2021
@ghost
Copy link

ghost commented Feb 2, 2021

Hello @miniksa!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

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 (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit 9fb4fb2 into microsoft:main Feb 2, 2021
@Nacimota Nacimota deleted the 8935-fix-shader-time branch February 3, 2021 01:02
@Nacimota Nacimota mentioned this pull request Feb 3, 2021
2 tasks
zadjii-msft pushed a commit that referenced this pull request Feb 4, 2021
<!-- 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.
DHowett pushed a commit that referenced this pull request Feb 5, 2021
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)
DHowett pushed a commit that referenced this pull request Feb 10, 2021
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
DHowett pushed a commit that referenced this pull request Feb 10, 2021
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)
@ghost
Copy link

ghost commented Feb 11, 2021

🎉Windows Terminal Preview v1.6.10412.0 has been released which incorporates this pull request.:tada:

Handy links:

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Experimental HLSL shaders - cbuffer Time not bound

4 participants