Bug Graph in half-precision overrides any precision of subgraphs

EDIT: We're using Shader Graph 7.4.1 but same bug is all version up to latest 7.5.3
Hello,

We've encountered a bug, where using half-precision setting in master graph overrides any precision settings of nodes in any subgraphs used in that master graph. It's a subtle bug, but it can cause quite a bit of pain when used with time based shaders (or anything that uses "big numbers").

Let me illustrate the bug with this simple setup:


On left, you have the master graph in half-precision mode. It only contains reference to a sub-graph.

The sub-graph on the right has few nodes (Time, Sine and Add), and has half-precision mode and overridden precision for node Sine and Time.

Now, if I generate the shader for the master graph, it gives me following:

Code (CSharp):

1.   // Graph Functions
2.          
3.             void Unity_Sine_float(half In, out half Out) // WRONG! half parameters
4.             {
5.                 Out = sin(In);
6.             }
7.          
8.             void Unity_Add_half(half A, half B, out half Out)
9.             {
10.                 Out = A + B;
11.             }
12.          
13.             struct Bindings_HalfPrecisionTestSub_49e552f1530d6d94fbf198c8493e41e0
14.             {
15.                 float3 TimeParameters;
16.             };
17.          
18.             void SG_HalfPrecisionTestSub_49e552f1530d6d94fbf198c8493e41e0(Bindings_HalfPrecisionTestSub_49e552f1530d6d94fbf198c8493e41e0 IN, out half OutVector1_1)
19.             {
20.                 float _Sine_B500F2D8_Out_1;
21.                 Unity_Sine_float(IN.TimeParameters.x, _Sine_B500F2D8_Out_1);
22.                 half _Add_7841D88C_Out_2;
23.                 Unity_Add_half(_Sine_B500F2D8_Out_1, 0.001, _Add_7841D88C_Out_2);
24.                 OutVector1_1 = _Add_7841D88C_Out_2;
25.             }

As you see, the Unity_Sine_float has actually half parameters despite the precision set (which is also reflected in the name). Otherwise the precision is ok. TimeParameters are actually ignoring the precision altogether.

It took me a bit of investigation (and learning how Shader Graph works), and I finally found out the issue.

In short, the issue is how subgraphs are imported as assets. In ShaderSubGraphImporter.cs there is this piece of code in method ProcessSubGraph:

Code (CSharp):

1.  foreach (var node in nodes)
2.             {
3.                 if (node is IGeneratesFunction generatesFunction)
4.                 {
5.                     registry.builder.currentNode = node;
6.                     generatesFunction.GenerateNodeFunction(registry, GenerationMode.ForReals);                  
7.                     registry.builder.ReplaceInCurrentMapping(PrecisionUtil.Token, node.concretePrecision.ToShaderString());
8.                 }
9.  

GenerateNodeFunction is responsible for the actual code generation for each node, and then registry.builder.ReplaceInCurrentMapping is supposed to replace "$precision" (PrecisionUtil.Token) tag in the resulting function with whatever precision the node had (in our case Sine has float precision).

The issue is that while registry.builder contains the generated function, the actual code for the function is already copied elsewhere (registry.sources dictionary). Those copied sources actually contain "$precision" tag in them, and they're saved into the imported asset:



Then, when the master graph is generated, it grabs the code for the functions of the sub-graph(s), and inserts into the shader. Later on, the actual code generation for that master graph replaces $precision tokens with the precision of the master graph, thus overriding all the types of the functions in the sub-graph.

So there. I fixed the issue with this really ugly piece of code, but until there's an official fix, we need to roll with this:

Code (CSharp):

1. if (node is IGeneratesFunction generatesFunction)
2.                 {
3.                     registry.builder.currentNode = node;
4.                     generatesFunction.GenerateNodeFunction(registry, GenerationMode.ForReals);
5.                  
6.                     // this call is unnecessary, the meat is in values of registry.sources
7.                     // registry.builder.ReplaceInCurrentMapping(PrecisionUtil.Token, node.concretePrecision.ToShaderString());
8.                  
9.                     // ugly hack to fix precision of subgraph functions
10.                     var keys = registry.sources.Keys.ToList();
11.                     foreach (var key in keys)
12.                     {
13.                         var value = registry.sources[key];
14.                         value.code = value.code.Replace(PrecisionUtil.Token, node.concretePrecision.ToShaderString());
15.                         registry.sources[key] = value;
16.                     }
17.                 }

Maybe GenerateNodeFunction should do the token replacement, but I did not know how to fix it "properly" so I'll leave it to the pros.

After re-importing the sub-graphs, and re-saving the master graph, the issue was gone and Unity_Sine_float had correct parameter types and our time based shaders were again jitter free.

 

Hi,

We have met similar issues. This workaround also works for us. Thanks a lot for the post.
I would like to know how Unity thinks about it, and if there could be any potential issues.