Visual shader editor node preview shader compilation fails with instance uniforms

[lostminds]

Godot version

4.0b17

System information

macOS 13.2

Issue description

If you make a visual spatial shader and that uses an instance uniform and you have some nodes with previews (the little graphic view) for example like this:

You will get a strange error in the output whenever you load the shader like this:

--Main Shader--
    1 | shader_type canvas_item;
    2 | 
E   3-> instance uniform float InstanceParameter : hint_range(0, 1) = 1;
    4 | 
    5 | 
    6 | 
    7 | void fragment() {
    8 | 
    9 |  float n_out21p0 = InstanceParameter;
   10 | 
   11 | 
   12 | 
   13 |  float n_out14p0 = 1.0 - n_out21p0;
   14 | 
   15 | 
   16 |  COLOR.rgb = vec3(n_out14p0);
   17 | }
   18 | 
  :3 - Uniform instances are not yet implemented for 'canvas_item' shaders.
  Shader compilation failed.
  ./servers/rendering/renderer_rd/shader_rd.h:140 - Condition "!version" is true. Returning: RID()

After a lot of experimenting the reason for this seems to be that the visual shader editor is actually also generating canvas_item shaders to render the little node previews (an elegant solution!). And for this shader code it's using the uniforms the nodes are connected to. However, this doesn't seem to account for the fact that we now have instance uniforms that are supported in spatial visual shaders, but not in canvas_item shaders. So it's failing to compile the shaders for the previews and outputs this error in the process. The failures have probably been happening since instance uniforms were added, but the error reporting has been added more recently so I started seeing these in one of the later beta versions.

This doesn't seem to affect the generated shader performance, but it's outputting some confusing errors (took me a good while to figure out) and the previews for the nodes just show all white so you could say it affects the usability of the shader editor.

Steps to reproduce

• Create a spatial visual shader
• Add an input parameter and set the qualifier to be "Instance"
• Link the parameter value to some node with a preview
• Link that node to the output
• Observe output for errors
• Open/close the file in the editor or open/close the preview on the node to see errors again.

Minimal reproduction project

The shader from the example: VisualShaderInstanceUniform.zip

[clayjohn]

There is a function that decides whether previews are allowed to be shown. It should be able to detect if an instant uniform is used and then just not allow the preview to be shown.

[lostminds]

It should be able to detect if an instant uniform is used and then just not allow the preview to be shown.

Or, in the spirit of my renderer feature support flags proposal it could disable them if instance uniforms are not supported for canvas_item shaders in the current renderer, and define that in the renderer. So that if instance uniforms are supported for canvas_item shaders in the future this functionality will adapt automatically.

[clayjohn]

It should be able to detect if an instant uniform is used and then just not allow the preview to be shown.

Or, in the spirit of my renderer feature support flags proposal it could disable them if instance uniforms are not supported for canvas_item shaders in the current renderer, and define that in the renderer. So that if instance uniforms are supported for canvas_item shaders in the future this functionality will adapt automatically.

I don't think we should use macros for something like this. At most we should convert the instance uniform into a regular uniform

[clayjohn]

Looking into this a little, its going to require a decent amount of work. Right now we have a function to check if a given node will show the preview icon; has_output_port_preview() For most nodes it defaults to true, and then a few nodes override it and set it to return false.

The trouble is, this check isn't recursive. So If I have something like an instance uniform, or a depth texture node, the next node will still show the preview port, even though it shouldn't.

@Chaosus Should we make has_output_port_preview() cascade to further nodes? For example, if one of a node's inputs has has_output_port_preview() evaluate to false, it would also evaluate to false. Or is there a situation where an input can't be used for previews, but the current node can?

[Chaosus]

Should we make has_output_port_preview() cascade to further nodes?

I guess we should focus our forces to implement the canvas item instances instead.

[clayjohn]

Should we make has_output_port_preview() cascade to further nodes?

I guess we should focus our forces to implement the canvas item instances instead.

Well, the problem still presents itself for other things like varyings and depth_texture.

[Chaosus]

Ah, then yes, recursive check should be added like I already did for recursive connections (#35362).

[Chaosus]

I think I could fix it.

[lostminds]

At most we should convert the instance uniform into a regular uniform

After thinking some more about this, isn't it the case that within the context of the shader editor previews all uniforms can actually be considered basic shader uniforms (or even constants?) with the uniform default value (or 0 I guess if it doesn't have one)? I'm not sure about varyings but I guess depth_textures could also be just be given a default value in the previews. This would require some substitutions in the shader code generated for the previews, but would have the benefit of being able to show previews, work on all renderers and allows the user to see changes to the previews by changing these default values.

While hiding the previews might be correct if these can't be generated now, it's also quite confusing and bad from a usability point of view. And if that's also a lot of work to achieve cascading correctly I wonder if it's worth the effort?