Skip to content
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

Experimental HLSL shaders - cbuffer Time not bound #8935

Closed
softmonkey opened this issue Jan 29, 2021 · 7 comments · Fixed by #8994
Closed

Experimental HLSL shaders - cbuffer Time not bound #8935

softmonkey opened this issue Jan 29, 2021 · 7 comments · Fixed by #8994
Labels
Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues good first issue This is a fix that might be easier for someone to do as a first contribution Help Wanted We encourage anyone to jump in on these. Issue-Task It's a feature request, but it doesn't really need a major design. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone

Comments

@softmonkey
Copy link

Environment

Platform ServicePack Version VersionString
Win32NT 10.0.19042.0 Microsoft Windows NT 10.0.19042.0

Steps to reproduce

  1. Write a simple HLSL shader that uses Time such as:
  // The original retro pixel shader
  Texture2D shaderTexture;
  SamplerState samplerState;
  
  cbuffer PixelShaderSettings {
    float  Time;
    float  Scale;
    float2 Resolution;
    float4 Background;
  };
  
  float4 main(float4 pos : SV_POSITION, float2 tex : TEXCOORD) : SV_TARGET
  {
    // Read the color value at the current texture coordinate (tex)
    //  float4 is tuple of 4 floats, rgba
    float4 color = shaderTexture.Sample(samplerState, tex);
    
    // Vertical scroll
    tex.y+= Time;
    color.rg = tex * 0.5f;
    
    // Return the final color
    return color;
  }

2, Add it to your profile config

Expected behavior

I am expecting the example shader to scroll vertically. Time is useful for a number of effects.

Actual behavior

Shader is compiling fine, so suspect an error in the binding.
See TODO terminal/blob/main/src/renderer/dx/DxRenderer.cpp, DxEngine::_ComputePixelShaderSettings()

@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Jan 29, 2021
@zadjii-msft
Copy link
Member

Oh yea, yikes. That TODO definitely should be tracked somewhere, thanks! We probably should have made sure to do that before we shipped the shader out, clearly I forgot 😅

@zadjii-msft zadjii-msft added Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues good first issue This is a fix that might be easier for someone to do as a first contribution Help Wanted We encourage anyone to jump in on these. Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. labels Jan 29, 2021
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Jan 29, 2021
@zadjii-msft zadjii-msft added this to the Terminal v1.7 milestone Jan 29, 2021
@Nacimota
Copy link
Contributor

Nacimota commented Feb 1, 2021

This feature got me excited enough to actually learn HLSL and have a go at doing some fancy shader effects for the terminal but I was quite disappointed to see the time input wasn't working because it's essential to basically any passive animated effect.

C++ is almost greek to me, but I had a go at fixing it myself and it seems like passing the time in _ComputePixelShaderSettings() where that TODO is doesn't entirely solve the problem because the screen only seems to update when the text changes:

2021-02-01.11-32-23.mp4

I can't figure out if it's that _ComputePixelShaderSettings() needs to be called somewhere else, or if the renderer just doesn't bother redrawing the frame when no updates have been made, or something else entirely, but oh boy is it pretty! I can't wait to see this fixed (by someone that knows what they're doing).

@zadjii-msft
Copy link
Member

Let me just start with a WOW 👀! When we first added these shaders, this is exactly the kind of cool thing I was expecting people to be able to do with them. That looks awesome!

You're right in finding that our renderer only paints a frame when the contents of the window have changed. So yea, right now you need to be constantly emitting text or blinking the cursor or whatever to have the effect be "smooth". This is something we did a long time ago to try and save CPU cycles. Obviously though, that optimization doesn't make any sense if we want to let people make smooth graphical effects.

If you make the PR to bind Time to something, then we can on our side hook up the DxEngine to the RenderThread to tell it to manually paint a frame every 1/60th of a second (when the pixel shader effects are enabled). Sound like a plan?

/cc @miniksa (our renderer expert)

@Nacimota
Copy link
Contributor

Nacimota commented Feb 1, 2021

That sounds good to me. As I said, I'm not really a C++ guy so I'm not sure I'm getting/setting the time in the most sensible way, but if you folks are willing to entertain my lack of experience with some constructive review I'll happily take a stab at it.

@zadjii-msft
Copy link
Member

I'm always happy to help! I'm not sure anyone can confidently call themselves a C++ guy, no matter how much experience they have with the language 😜

@miniksa
Copy link
Member

miniksa commented Feb 1, 2021

If you make the PR to bind Time to something, then we can on our side hook up the DxEngine to the RenderThread to tell it to manually paint a frame every 1/60th of a second (when the pixel shader effects are enabled). Sound like a plan?

/cc @miniksa (our renderer expert)

Yeah no problem. I can do that after the Time is piped through... force the ticks to happen anyway when shaders are enabled.

Do note that this will only make the performance characteristics of this even heavier (more CPU, more battery/power, etc.) but I think we're well past caring about that point when using shaders anyway especially given that shaders enabled right now already forces full-screen repaint instead of differential drawing.

@Nacimota Nacimota mentioned this issue Feb 1, 2021
3 tasks
@ghost ghost added the In-PR This issue has a related PR label Feb 1, 2021
@ghost ghost closed this as completed in #8994 Feb 2, 2021
ghost pushed a commit that referenced this issue Feb 2, 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.
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Feb 2, 2021
DHowett pushed a commit that referenced this issue 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)
@ghost
Copy link

ghost commented Feb 11, 2021

🎉This issue was addressed in #8994, which has now been successfully released as Windows Terminal Preview v1.6.10412.0.:tada:

Handy links:

This issue 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 good first issue This is a fix that might be easier for someone to do as a first contribution Help Wanted We encourage anyone to jump in on these. Issue-Task It's a feature request, but it doesn't really need a major design. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants