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

Reading VIEWPORT_SIZE in light() function breaks spatial shader. #76861

Closed
EMBYRDEV opened this issue May 8, 2023 · 4 comments · Fixed by #76977
Closed

Reading VIEWPORT_SIZE in light() function breaks spatial shader. #76861

EMBYRDEV opened this issue May 8, 2023 · 4 comments · Fixed by #76977

Comments

@EMBYRDEV
Copy link
Contributor

EMBYRDEV commented May 8, 2023

Godot version

4.0.2.stable

System information

Windows 11, Vulkan, RTX2070 Super

Issue description

Reading from VIEWPORT_SIZE in the light() function causes spatial shader to break and renders as default white.

Steps to reproduce

image

when line 51 is present the fails to compile with the following message in the output log but no compilation errors.

servers/rendering/renderer_rd/forward_clustered/scene_shader_forward_clustered.cpp:171 - Condition "!shader_singleton->shader.version_is_valid(version)" is true.
  drivers/vulkan/rendering_device_vulkan.cpp:5361 - Condition "!shader" is true. Returning: RID()
  drivers/vulkan/rendering_device_vulkan.cpp:5835 - Condition "!us" is true.

commenting out line 51 solves this issue.

The documentation states that VIEWPORT_SIZE should be available to the light() function.

Minimal reproduction project

N/A

@clayjohn clayjohn added this to the 4.1 milestone May 8, 2023
@clayjohn
Copy link
Member

clayjohn commented May 8, 2023

Related to #76556 We need to make sure that the light builtins are properly exposed in the light shader with the appropriate names (the variables are likely already available, they just need to be renamed)

@So-Yu-Him
Copy link

@clayjohn Hi, I am a newbee to open source projects and currently being a computer science student in HKUST, I want to gain more project experiences before graduation so I want to help for open source projects, I am willing to learn new techniques and help with this issue. Can you assign this issue to me and give me more information about it? Do I need to fix #76556 before fixing this issue?

@Calinou
Copy link
Member

Calinou commented May 8, 2023

Can you assign this issue to me

For future reference, we only use the assignees feature for core contributors (and we don't always use it in the first place). Since contributors can leave at any time for any reason, we'd rather avoid a situation where nobody is working on an issue because it's assigned to someone who is no longer active.

@clayjohn
Copy link
Member

clayjohn commented May 9, 2023

@clayjohn Hi, I am a newbee to open source projects and currently being a computer science student in HKUST, I want to gain more project experiences before graduation so I want to help for open source projects, I am willing to learn new techniques and help with this issue. Can you assign this issue to me and give me more information about it? Do I need to fix #76556 before fixing this issue?

You do not need to fix #76556 before fixing this issue. The light shader code is defined here:

void light_compute(vec3 N, vec3 L, vec3 V, float A, vec3 light_color, bool is_directional, float attenuation, vec3 f0, uint orms, float specular_amount, vec3 albedo, inout float alpha,

You can see here the variables that are renamed to be compatible with the names expected by the light shader:

mat4 inv_view_matrix = scene_data_block.data.inv_view_matrix;
vec3 normal = N;
vec3 light = L;
vec3 view = V;

Here you can see that VIEWPORT_SIZE is translated into ``scene_data.viewport_size:
https://github.com/godotengine/godot/blob/b0b23082c4d8cdcf20c090159f33726317504bdc/servers/rendering/renderer_rd/forward_clustered/scene_shader_forward_clustered.cpp#LL591C6-L591C6

So you need to ensure that scene_data.viewport_size is available in the shader at the appropriate time.

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