Better previews in shader graph

[iBicha]

Previews in the shader graph nodes are completely broken hacky, dirty and unreliable.

Currently they rely on a global texture _PreviewTexture set by the custom effect.

The shader graph gui needs to be capable of capturing the game view before effects reliably, so the graph is capable of showing the transformation of the original texture into the destination master.

Need ideas on the best way to implement this (the current way can be improved to be honest)

[iBicha]

Lead: https://github.com/Unity-Technologies/UnityCsReference/blob/master/Editor/Mono/GameView/GameView.cs#L70

[iBicha]

Another idea is to use a predefined texture and apply changes to it for the preview (which is not very ideal, but still)

[edwin0cheng]

I am trying to understand this bug (and hopefully fix it), and would you mind to explain where the "_PreviewTexture" is used in current code ? I cannot find any other references which this texture is in used. Or do i miss something ?

[iBicha]

Ok here's the scope of the issue:

Well, the idea is to be able to visualize the camera (any camera in your scene, assuming these is at least one), and see its preview when you sample a Main Texture (Input) node, and see it transformed by all the steps, before achieving the final effect.

We also want a copy of the camera render BEFORE the effect is applied, so we can see the preview in shader graph (because if you take the output of the camera after the effect is applied, and use that for the shader graph previews, the result effect will be applied twice, which is not what we want)

As I did not find a good way to do this, I figured, I would make a copy of the current camera buffer to a render texture right before applying the effect, and set that as a global texture. (which happens right here)

Because the effect is being applied twice (once for the scene view, and once for the game view) that's why we have a context.isSceneView, so we only copy the game view camera. This in theory should work (also should be bad for performance) but since it's only in the editor and not the build, for now it's fine.

It's a very hacky and inelegant solution, which doesn't even work properly. On the other hand, there's a the preview side of the nodes, where we want to tell shader graph to use _PreviewTexture if we are in a preview mode, and _MainTex otherwise.

This should have to do with the methods CollectShaderProperties CollectPreviewMaterialProperties, and GetVariableNameForSlot, and how to implement them for the class MainTextureNode : BaseUnityTextureNode class.

The issue is that GetVariableNameForSlot doesn't have a GenerationMode parameter, so I can do something like this:

            if (generationMode.IsPreview())
            {
                return "_PreviewTexture";
            }
            else
            {
                return "_MainTex";
            }

And overall either I'm missing something important, or simply the shader graph api didn't take into account this use case, and I did not get enough time to dive deeper and find a better method to fix this.

I hope this is enough information, if you have any ideas or other approaches, you're very welcome, also feel free to ask any questions you might have.

[iBicha]

I actually just got inspired, and pushed a fix that is super ugly, but temporarily fixes the problem. Just something to build on top of. I will leave this issue open because this is fix is definitely not good.

[edwin0cheng]

Thanks for the details reply. And i was getting the same conclusion as the main issue are coming from the GetVariableNameForSlot.

On the other hand, here is my hacky approach to fix this issue:

        public override void CollectPreviewMaterialProperties(List<PreviewProperty> properties)
        {
            var inputTextureName = unityInputTexture;

            properties.Add(new PreviewProperty(PropertyType.Texture2D)
            {
                name = inputTextureName,
                textureValue = ImageEffectGraph.PostProcessing.RenderWithMaterialRenderer.PreviewTexture,
            });
        }

And in RenderWithMaterialRenderer :

public class RenderWithMaterialRenderer : PostProcessEffectRenderer<RenderWithMaterial>{
...

        static public RenderTexture PreviewTexture = null;

        public override void Render(PostProcessRenderContext context)
        {
#if UNITY_EDITOR
            // if (!context.isSceneView)
            if(context.camera.cameraType == CameraType.Game)
            {
//                previewCommandBuffer.Clear();
//                Blit(context.command, context.source, rt, null);
                context.command.Blit(context.source, rt);
//                Graphics.ExecuteCommandBuffer(previewCommandBuffer);

//                previewCommandBuffer.Clear();
                context.command.SetGlobalTexture(_PreviewTexture, rt);
//                Graphics.ExecuteCommandBuffer(previewCommandBuffer);                

                PreviewTexture = rt;
            }
...

CollectPreviewMateiralProperiteswill be call in PreviewManager::RenderPreviews, which collect all PreviewProperty into a m_PreviewPropertyBlock, and then draw with it by Graphics.DrawMesh.

[edwin0cheng]

The problem of my fix is I don't know there is no good way to pass the RenderTexture from `RenderWithMaterialRenderer to the BaseUnityTextureNode (I use global static variable right now, which is ugly)

Compare to your fix, i don't know which one is better :)

[iBicha]

This is an excellent find! Passing the textureValue in the preview did not occur to me at all! As per the static Texture, it's not the best, but fortunately textures can be passed globally through Shader.GetGlobalTexture. So a combination of both workarounds is a little less ugly :)

I'm currently working on a fix related to the aspect ratio of the preview (which should be aligned the game view), is it ok if I include your fix in that commit?

[edwin0cheng]

I'm currently working on a fix related to the aspect ratio of the preview (which should be aligned the game view), is it ok if I include your fix in that commit?

Of course.

And i did make some changes which add a new parameter for disable image effect apply on SceneView. May i make a PR for this after your fix pushed ?

[iBicha]

Thank you. And absolutely, that's a good idea! Although it would be better if this was an option in the effect settings, so the user can choose if this is a game view only effect, or both. Having the effect on in the scene view sometimes help with designing things better, instead of moving the camera around or playing the game. How does that sound?

Edit: I misread your comment, that's actually what you did, sounds good to me