terminal icon indicating copy to clipboard operation
terminal copied to clipboard

Added experimental.pixelShaderImagePath

Open mrange opened this issue 2 years ago β€’ 23 comments

I realize I might be one of the few developers that care about custom shader support in terminal but I thought it's worth proposing it and see what you think.

This is to support custom shaders with custom textures.

I was thinking of exposing the background image to the shader but that felt complicated after looking into it.

I have tested exploratively. I think the texture loader is possible to unit test so that is a possible improvement.

The error reporting (as with other custom pixel shader code) is not very good. That is also an area that I could improve upon.

I do think the risk of adding this is rather low as the new code is only executed when experimental.pixelShaderImagePath is set.

Details

Only added to the Atlas engine. Unsure if needs adding in DXEngine?

Instead I load the texture using WIC into a shader resource view. When binding shader resources I test for presence of custom texture and bind it to register t1.

The image loading code was found in the D3D Texture documentation. It's a mouthful but seems rather robust.

Tested setting: "experimental.pixelShaderImagePath"

  1. Tested not specifying it.
  2. Tested setting it.
  3. Tested changing it (the changes are picked up)
  4. Tested invalid path
  5. Tested a custom shader that made use of the custom texture.

mrange avatar Sep 24 '22 18:09 mrange

image

The blazor logo is a custom shader image.

mrange avatar Sep 24 '22 18:09 mrange

I get a bunch of spell check issues in code I haven't modified. What should I do about them?

mrange avatar Sep 24 '22 18:09 mrange

Finally all checks are green

mrange avatar Sep 25 '22 08:09 mrange

Exciting!

DHowett avatar Sep 27 '22 18:09 DHowett

Thanks for the review. I am a little bit short in time during weekdays, but I think I can address some of your concerns over the weekend.

mrange avatar Sep 27 '22 18:09 mrange

I have to confess as a user, I'm unsure what the difference between this and the already existing shader support is. I don't think we should have 2 settings regarding shaders. That'll be very confusing. If there's some type of support missing from the other shader code, could you simply merge the 2 instead so we only have a single user facing option to specify a shader file?

WSLUser avatar Sep 29 '22 12:09 WSLUser

@WSLUser I don't mind merging the options but this setting is to support loading a shader texture like a PNG that the shader source code can make use of. I am testing with these settings:

        "defaults": 
        {
            "experimental.pixelShaderImagePath": "C:\\temp\\cake.png",
            "experimental.pixelShaderPath": "C:\\code\\github\\windows-terminal-shaders\\cake.hlsl"
        },

So the pixelShaderImagePath points to a PNG loaded as a texture which the shader pointed out by pixelShaderPath uses.

So in my example it looks like this:

image

The background is computed by the shader but cake logo comes from the PNG which the shader overlays over the background.

How would you prefer the configuration option to look like?

mrange avatar Sep 29 '22 12:09 mrange

Ok I think I see what you mean. That makes more sense but this will definitely need some documentation to avoid the confusion I had. In this instance I see that 2 settings will in fact be needed but they should be grouped together as this one requires the other one to be set (so the shader path should be set as a hard dependency before trying to fetch a ShaderImage path). I think you might be able to use the json schema for this, which will need updating regardless.

WSLUser avatar Sep 29 '22 20:09 WSLUser

@lhecker in response to your comment on looking at DirectXTK for texture loading code I made the change that I added that repo as a submodule. I noticed you had submodules already so I figured it wouldn't be too much of a paradigm shift to add another one. Not sure this is a how you like it done but this is my current proposal.

If we are to go with the submodule I would like some advice on best way on how to include the source code from it. The approach taken works it seems but is it a good enough solution?

mrange avatar Oct 02 '22 17:10 mrange

So because of the "controversial" addition of a submodule I consider this PR to be a proposal with need of more testing cleanup to be merged. Seemed to work fine in my smoketests

mrange avatar Oct 02 '22 17:10 mrange

Bump. Still looking for feedback if the latest approach is acceptable (ie bring in DirectXTK as a submodule)

mrange avatar Oct 08 '22 19:10 mrange

@mrange I think it would've been fine if you just copied DirectXTK's source code into this project. But since it's just a submodule, it's easy to add/remove it and so I'm also fine with your new approach. I think this is good to be merged as is. πŸ‘

lhecker avatar Oct 10 '22 14:10 lhecker

Spellchecker still complains about:

#### Unrecognized words (1)

otepad

But didn't find any otepad strings in git repo.

mrange avatar Oct 30 '22 07:10 mrange

(Thanks for being patient with us. I know that it's been some time. I wanted myself assigned so that I could vet the dependency and the risk we'd take on if we further brought it into Windows. We probably wouldn't be taking on any risk, but I just want to make sure.)

DHowett avatar Nov 03 '22 23:11 DHowett

If you are concerned about bringing in the dependency into windows builds I could put some #ifdef in there

mrange avatar Nov 04 '22 05:11 mrange

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 7 days of this comment.

ghost avatar Dec 01 '22 00:12 ghost

It's not exactly fair for me to take a while to respond and then let the bot call you slow, so I'm poking it along to remove the "no recent activity" tag :)

DHowett avatar Dec 01 '22 00:12 DHowett

Hope to get some time coming weekend to look into comments. The weekends before been buzy.

mrange avatar Dec 01 '22 06:12 mrange

Hey we're trying to clean out our PR backlog: I'm gonna tag this up for discussion next team sync and figure out if we're bringing this in or not. The change Dustin called out above was simple enough that we could probably do it on your behalf, but Leonard had some other thoughts on another way we could go about this.... something about comments and shadertoy and I honestly didn't understand all he said 😝

zadjii-msft avatar Feb 07 '24 23:02 zadjii-msft

To add more notes we said after I sent that: Maybe we just pull this in as-is, because it handles a lot of the plumbing for loading images and passing them down. THEN, as a follow up, we remove this setting, and replace it with the ShaderToy thing. That's the great thing with it being an experimental setting - we can just remove it later if we have a better idea πŸ˜‰

(there's also probably some work that needs doing to update this for the latest version of the Atlas Engine (which went through an entire re-write since this PR was opened)

@mrange you are the Terminal shader master though, so let us know what you're thinking. Sorry for letting this stagnate so long! Let's get this in for 1.21 ☺️

zadjii-msft avatar Feb 08 '24 11:02 zadjii-msft

Hi sorry for not answering. I have been travelling and doing shader presentations for .NET developers. Anyway, I will try to have an answer after work.

mrange avatar Feb 14 '24 12:02 mrange

So I obviously like to see this merged and I think it will enable some cool things for us. The obvious thing is ofc an image like a logo but it will also enable importing noise images which are very useful things as it can be costly to do a compute for.

When I wrote it after I got the tip about using the external library I felt pretty confident that this was a rather safe change as it simplified the code quite much.

What is this shadertoy thing you are discussing?

mrange avatar Feb 18 '24 09:02 mrange

Re-reading the PR. Most changes are just forwarding data and some spelling errors. The actual works is mostly handled by DirectXTk or rather simple (like checking if we have an extra texture and in that case include).

I think personally it's rather low risk especially since this is new experimental setting but obviously you might disagree

mrange avatar Feb 18 '24 09:02 mrange

What is this shadertoy thing you are discussing?

I honestly don't know. @lhecker mentioned something about being able to pass a texture to the shader using a comment in the body of the shader itself, that shadertoy would then load as a texture for you? That was at the end of the day though and I probably misunderstood πŸ˜…

I can handle updating to main, and pulling in DirectXTK as a raw include if you'd like. That'll get this in and get us moving ☺️

EDIT: I may have spoke too quickly on the "I can merge it" thing. I forgot that Atlas engine was basically entirely re-written since this was opened. This is more than a trivial merge, but I'll give it a shot.

zadjii-msft avatar Feb 22 '24 12:02 zadjii-msft

shadertoy has a very constrained template of things you can do in a shader: https://www.shadertoy.com/howto ...but it allows you to do some super impressive stuff: https://www.shadertoy.com/view/ssjyWc

I figured we could - over time - offer an API that is inspired by shadertoy's, but with the web interface configuration for the 4 channels and the buffer stages replaced with comments or similar that can be parsed during shader loading, or by having an additional .json file that contains such metadata.

I know that FX11 would fit quite well here, but it's not actively maintained, nor are there plans to port it to D3D12.

lhecker avatar Feb 22 '24 13:02 lhecker

@check-spelling-bot Report

:red_circle: Please review

See the :open_file_folder: files view, the :scroll:action log, or :memo: job summary for details.

Unrecognized words (1)

BGRA

Previously acknowledged words that are now absent AAAa ABORTIFHUNG Batang bgra chcbpat CLUSTERMAP COLORMATRIX COLR CUsers DBlob dcompile DESTINATIONNAME dxguid FFrom Geddy GETKEYSTATE GFEh hicon inputrc IWIC kcub kcud kcuf kcuu khome MAPVIRTUALKEY Mbxy Mip NOTIMEOUTIFNOTHUNG nto QUESTIONMARK reallocs reamapping Resequence RTFTo SFolder srv subresource tracelog TRIANGLESTRIP Uniscribe VKKEYSCAN waitable wic wincodec WScript XColors xff XMFLOAT πŸ«₯
To accept these unrecognized words as correct and remove the previously acknowledged and now absent words, you could run the following commands

... in a clone of the [email protected]:mrange/terminal.git repository on the mrange/shader_bitmap branch (:information_source: how do I use this?):

curl -s -S -L 'https://raw.githubusercontent.com/check-spelling/check-spelling/v0.0.22/apply.pl' |
perl - 'https://github.com/microsoft/terminal/actions/runs/8008136479/attempts/1'
Available :books: dictionaries could cover words (expected and unrecognized) not in the :blue_book: dictionary

This includes both expected items (2246) from .github/actions/spelling/expect/04cdb9b77d6827c0202f51acd4205b017015bfff.txt .github/actions/spelling/expect/alphabet.txt .github/actions/spelling/expect/expect.txt .github/actions/spelling/expect/web.txt and unrecognized words (1)

Dictionary Entries Covers Uniquely
cspell:swift/src/swift.txt 53 1 1
cspell:gaming-terms/dict/gaming-terms.txt 59 1 1
cspell:monkeyc/src/monkeyc_keywords.txt 123 1 1
cspell:cryptocurrencies/cryptocurrencies.txt 125 1 1
cspell:scala/dict/scala.txt 153 1 1

Consider adding them (in .github/workflows/spelling2.yml) for uses: check-spelling/[email protected] in its with:

      with:
        extra_dictionaries:
          cspell:swift/src/swift.txt
          cspell:gaming-terms/dict/gaming-terms.txt
          cspell:monkeyc/src/monkeyc_keywords.txt
          cspell:cryptocurrencies/cryptocurrencies.txt
          cspell:scala/dict/scala.txt

To stop checking additional dictionaries, add (in .github/workflows/spelling2.yml) for uses: check-spelling/[email protected] in its with:

check_extra_dictionaries: ''
Errors (1)

See the :open_file_folder: files view, the :scroll:action log, or :memo: job summary for details.

:x: Errors Count
:x: ignored-expect-variant 4

See :x: Event descriptions for more information.

:pencil2: Contributor please read this

By default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.

If the listed items are:

  • ... misspelled, then please correct them instead of using the command.
  • ... names, please add them to .github/actions/spelling/allow/names.txt.
  • ... APIs, you can add them to a file in .github/actions/spelling/allow/.
  • ... just things you're using, please add them to an appropriate file in .github/actions/spelling/expect/.
  • ... tokens you only need in one place and shouldn't generally be used, you can add an item in an appropriate file in .github/actions/spelling/patterns/.

See the README.md in each directory for more information.

:microscope: You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. :wink:

If the flagged items are :exploding_head: false positives

If items relate to a ...

  • binary file (or some other file you wouldn't want to check at all).

    Please add a file path to the excludes.txt file matching the containing file.

    File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.

    ^ refers to the file's path from the root of the repository, so ^README\.md$ would exclude README.md (on whichever branch you're using).

  • well-formed pattern.

    If you can write a pattern that would match it, try adding it to the patterns.txt file.

    Patterns are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your lines.

    Note that patterns can't match multiline strings.

github-actions[bot] avatar Feb 22 '24 16:02 github-actions[bot]

image

zadjii-msft avatar Feb 22 '24 17:02 zadjii-msft

shadertoy has a very constrained template of things you can do in a shader: https://www.shadertoy.com/howto ...but it allows you to do some super impressive stuff: https://www.shadertoy.com/view/ssjyWc

I figured we could - over time - offer an API that is inspired by shadertoy's, but with the web interface configuration for the 4 channels and the buffer stages replaced with comments or similar that can be parsed during shader loading, or by having an additional .json file that contains such metadata.

I know that FX11 would fit quite well here, but it's not actively maintained, nor are there plans to port it to D3D12.

I have used shadertoy quite much :) (322 public shaders). The comment made me curious ofc.

Not surprisingly I would like to see more shader capabilities. While multipasses (and retaining the previous frame pass) is great I personally would think I would love some kind of live editing before to simplify development.

Imagine for example that windows terminal reloads the shader code automatically when changed. If something goes wrong catch the error and show it as the shader texture.

That way I could sit and develop shaders real-time in VIM. That would be a very cool demo! And also helps productivity.

Also would like to control the pixelshader version.

Then ofc capturing the windows sound and provide it as a texture with both the signal and the FFT opens up for cool stuff.

And so on :)

mrange avatar Feb 22 '24 19:02 mrange

As for deploying shaders what about a folder with all the content in (also zip)?

In the folder images, Common.fx, BufferA.fx, BufferB.fx, BufferC.fx and Main.fx (only Main.fx required). Some kind of way to configure the input textures (not sure how).

mrange avatar Feb 22 '24 19:02 mrange

Perhaps look at https://github.com/Gargaj/Bonzomatic for how they store "projects". Seems like pretty smart and supports DX. Bonzomatic is the goto tool for shader competitions and jams.

mrange avatar Feb 22 '24 19:02 mrange