Skip to content

Conversation

@Nacimota
Copy link
Contributor

@Nacimota Nacimota commented Feb 3, 2021

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

PR Checklist

  • CLA signed. If not, go over here and sign the CLA
  • 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: Fix shader time input #8994

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.

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.

- Make it clear that time is specifically seconds (and not milliseconds, for example)
- Explain that tex ranges from 0 to 1, unlike fragCoord in shadertoy/glsl
@DHowett
Copy link
Member

DHowett commented Feb 3, 2021

@jsoref slightly worried about the spellbot flags on this one!

hlsl and rgb are in the expect document-

(Terminal\dhowett-sl) ~\src\terminal-3.github % rg hlsl #(also rg rgb)
actions\spelling\expect\expect.txt
84:argb
1993:rgb
1994:rgba
1995:rgbi
988:hlsl

Is this a branch merge issue since the PR doesn't have 0.0.17 in?

@Nacimota
Copy link
Contributor Author

Nacimota commented Feb 3, 2021

Oh, yikes. I can't help but notice it's tried to spell check a png file, too. Should I have merged main back in to my branch before posting the PR?

@DHowett
Copy link
Member

DHowett commented Feb 3, 2021

Merging main would definitely help 😄 it should be an easy merge fortunately, and you're welcome to do it even after submitting!

@Nacimota
Copy link
Contributor Author

Nacimota commented Feb 3, 2021

I did consider merging from upstream but it's been a while since I've done it and I figured there were no conflicts with what I was working on and GitHub gave me a big green checkmark and everything. Lesson learned, I guess! 😬

@jsoref
Copy link
Contributor

jsoref commented Feb 3, 2021

I meant to suggest that you're going to want to force everyone with open PRs rebase onto main.

It looks like the bot is happy. And now you even have a shiny ✔️ visible here in the PR.

@jsoref
Copy link
Contributor

jsoref commented Feb 3, 2021

@Nacimota: sorry about that. It's rare that I do something like this. The last time this would have happened here is probably when the tool was first added, and I think it actually handled that case better...

I'll have to think about how to handle this when offering this upgrade to other projects. I'll probably just leave the custom path in place instead.

@Nacimota
Copy link
Contributor Author

Nacimota commented Feb 3, 2021

@jsoref no worries! I think it's mostly just laziness on my part not merging/rebasing as a matter of course.

Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

this is so cool. I'm glad you added this bit of documentation -- it's very well-explained. Very, very well. Thank you.


// Draw the terminal graphics over the background
return lerp(backgroundColor, sample, sample.w);
} No newline at end of file
Copy link
Collaborator

Choose a reason for hiding this comment

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

The code formatter does not seem to be checking for newlines in hlsl files. This is trivial, anyway.

@zadjii-msft zadjii-msft self-assigned this Feb 4, 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.

Okay this is excellent. I feel like this documentation could belong in my CS559 class 😄 Thanks!

@zadjii-msft zadjii-msft merged commit 7c42ed4 into microsoft:main Feb 4, 2021
@Nacimota Nacimota deleted the new-shader-samples branch February 4, 2021 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants