Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Vulkan shader error when passing screen texture to function #72290

Closed
ueshita opened this issue Jan 29, 2023 · 7 comments · Fixed by #72300
Closed

Vulkan shader error when passing screen texture to function #72290

ueshita opened this issue Jan 29, 2023 · 7 comments · Fixed by #72300

Comments

@ueshita
Copy link
Contributor

ueshita commented Jan 29, 2023

Godot version

v4.0.beta16.official.518b9e580

System information

Windows 11, Vulkan, AMD Radeon 680M

Issue description

If I pass a uniform sampler2D with hint_screen_texture to the function, I get a Vulkan shader error.
This issue does not occur before beta15.
It seems to me that perhaps #71455 has an impact.

Steps to reproduce

Specify the following shader to MeshInstance3D and Run.

shader_type spatial;

uniform sampler2D screen_tex : hint_screen_texture;

vec3 sample_rgb(sampler2D tex, vec2 uv) {
	return texture(tex, uv).rgb;
}

void fragment() {
	ALBEDO = sample_rgb(screen_tex, SCREEN_UV);
}

Minimal reproduction project

ShaderError.zip

@ueshita
Copy link
Contributor Author

ueshita commented Jan 29, 2023

The error log is this.

shader_error.txt

@clayjohn clayjohn added this to the 4.0 milestone Jan 29, 2023
@clayjohn
Copy link
Member

The issue here is that when using Multiview (for XR) Godot's shader compiler internally converts the screen_tex into a sampler2DArray. We have some logic that will automatically adjust calls to texture() to account for that, but we don't automatically change function signatures. It looks like we need to

Here is a copy of the error that includes the full shader printout: shader-error.txt

CC @BastiaanOlij and @Chaosus

@Chaosus
Copy link
Member

Chaosus commented Jan 29, 2023

Well, that will require to generate the version of function with samplerArray parameter internally. Plus, the UV coordinates should also be handled somehow (they are vec3 in that case). Maybe just forbid to pass these sampler types (depth, screen, normal_roughness) to the functions? If the user wants to use the output of these functions in the custom functions, he/she could always sample it in the main fragment function and just pass the RGBA output.

@BastiaanOlij
Copy link
Contributor

Interesting problem, just to understand a bit more of what is going on, @ueshita, is your project using stereoscopic rendering (i.e. are you using this for VR) because that would never have worked even before we changed the screen texture approach.

If you aren't then your code shouldn't fail as we shouldn't be compiling the multiview versions of our shaders and thus we're still only using sampler2D.
Still breaks if we are using multiview though.

@clayjohn
Copy link
Member

A different approach to solving this same problem is to add a new sampler type to user-space shaders. Earlier we rejected that approach as being too complex, but it may make sense for the XR usecase which will always need both multiview and non-multiview variants of the shader

Lyuma has a PR with this approach for reference #62130

My feeling is that we should use Chaosus' approach for Godot 4.0 and then see if we can remove the limitation in a later release.

@BastiaanOlij
Copy link
Contributor

BastiaanOlij commented Jan 30, 2023

Owh I remember @lyuma solution, that was indeed an interesting approach. The problem there is that we have a standing principle for not wanting large changes to the shader code base for edge cases, which this certainly is (multiview support, not breaking functions that take in samplers as a parameter).

Agree that Chaosus approach is the best thing we can do with such a short timeframe before release, though I think it should be combined with merging that old PR of yours (#63829) that properly disables multiview shaders, and then only applying the multiview checks when multiview shaders are enabled.

This way people who aren't writing XR games aren't hindered by this restriction, only those who actually enable multiview shaders will not be able to use functions together with screen textures.

@ueshita
Copy link
Contributor Author

ueshita commented Jan 31, 2023

i.e. are you using this for VR

I am not using VR/XR now.
If possible, I would be happy to use the shader code that I have been able to use so far without modification.
Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants