[Mårten Rånge (mrange)]

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.

[Mårten Rånge (mrange)]

The blazor logo is a custom shader image.

[Mårten Rånge (mrange)]

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

[Mårten Rånge (mrange)]

Finally all checks are green

[Leonard Hecker (lhecker)]

I'm generally fine with this, because while we might remove this option in the future to replace it with something more powerful I believe that texture loading like this will at least remain.

I just have a few concerns...

[Dustin L. Howett (DHowett)]

Exciting!

[Mårten Rånge (mrange)]

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.

[WSLUser]

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?

[Mårten Rånge (mrange)]

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:

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?

[WSLUser]

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.

[Mårten Rånge (mrange)]

Leonard Hecker (@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?

[Mårten Rånge (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

[Mårten Rånge (mrange)]

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

[Leonard Hecker (lhecker)]

Mårten Rånge (@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. 👍

[Mårten Rånge (mrange)]

Required by TextureLoader.

[Mike Griese (zadjii-msft)]

[Mårten Rånge (mrange)]

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 :)

[Mårten Rånge (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).

[Mårten Rånge (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.

[Mike Griese (zadjii-msft)]

Yea I mean, I read most of this as I merged the code, and I'm cool with it.

I'd suggest that Leonard Hecker (@lhecker) be the second, since atlas is his baby.

[Mårten Rånge (mrange)]

I read through the latest changes and at least to me they looked ok.

[Leonard Hecker (lhecker)]

Yeah, this PR looks good to me.

However, I'll try and see if I can write my own WIC loader, because I'd personally prefer if we minimized our external dependencies for now. I think it's unlikely we'll depend on other DirectXTK functionality in the future and so I'm not entirely sure if the ~40k line dependency is worth it, if I'm entirely honest. Reading through WICTextureLoader.cpp I get the feeling that we can write a significantly reduced version of it by relying on the fact that we only support recent Windows versions. If my own "WICTextureLoader" sucks, I'll approve the PR. 😅

[Mårten Rånge (mrange)]

Leonard Hecker (@lhecker) I will just mention that IIRC my first PR had my own texture loader but I was then pointed DirectXTK :)

[Leonard Hecker (lhecker)]

I will just mention that IIRC my first PR had my own texture loader but I was then pointed DirectXTK :)

I believe my intention at the time was to just say "Hey you can copy this other MS code if you'd like! It's free real estate!". When you added it as a submodule I figured I could just fix it up later, because I didn't want to bother you too much. But in the end, merging this PR took ages due to change in plans, etc., and... I'm sorry. 😣

[Mårten Rånge (mrange)]

Leonard Hecker (@lhecker) it's all good. I am just happy this gets in eventually :) I am planning to do some noise texturing stuff.

[Leonard Hecker (lhecker)]

After some thinking, I realized we don't need anything but GUID_WICPixelFormat32bppPBGRA. BGRA is the most widely supported native decoder format on Windows and is sufficient for our swap-chain (which is not sRGB and doesn't support HDR). I also felt like we want it to be Premultiplied so you don't have to do that yourself in the shader first. This makes all the lookup tables unnecessary and allowed me to shrink the entire WIC code down to <60 LOC.

I tested it with a HDR HEIF image, a 400MP JPEG, and a grayscale GIF, as well as a semi-transparent PNG and they all worked correctly. (Side note: The HEIF integration throws ~50 exceptions, calls the app store service via RPC, creates 76 threads, and stamps out an entire separate WARP D3D device per load on my machine. kwalidy™)

[Dustin L. Howett (DHowett)]

Let's DO this

[Dustin L. Howett (DHowett)]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[Mårten Rånge (mrange)]

Cool when do you think this is available in a preview I can install?

[Dustin L. Howett (DHowett)]

Mårten Rånge (@mrange) In about 9 hours! If you're a canary user, that is :D