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

Add support for struct uniforms in shaders #93603

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Chaosus
Copy link
Member

@Chaosus Chaosus commented Jun 25, 2024

struct_test

Notes:

  • This version uses a Dictionary type for the uniform type. I did not find the other type which fits better for this. Still, the interface sucks and do not auto-reflect the changes made in a shader struct, but at least It's something (user need to manually adds members to it and change their types) -> (UPDATE: Dictionary in the inspector now reflects the changes from the shader).

  • Structs needs to be declared before MaterialUniform in order to they can be used for uniforms, that's why I pushed them under #STRUCT statements inside each shader file.

  • There are few limitations with this implementation, currently:

    • You cannot assign a default value, e.g:
     struct Foo {
     	vec3 v;
     };
     uniform Foo foo = Foo(vec3(0, 1, 0));
    
    • instance/global scope initializers are not supported
  • Arrays of common datatype in struct are supported (you can pass an array of conformal type):

struct_alpha_array

  • Compatibility renderer is supported

  • set_shader_parameter via script is supported (You need to pass a dictionary, remember).

  • UPDATE: You can declare an array of struct, e.g:

     struct Foo {
     	vec4 v;
     };
     uniform Foo foos[2];
    
  • UPDATE 2: You can use a struct inside a struct, e.g.:

     struct A {
     	float t;
     };
     struct B {
     	A a;
     };
     uniform B b;
    

@QbieShay, @Calinou If you're interested to check and play with it, you're welcome.

@Chaosus Chaosus added this to the 4.x milestone Jun 25, 2024
@Chaosus Chaosus force-pushed the shader_struct_uniform_support branch 6 times, most recently from e048e67 to 31d21f9 Compare June 25, 2024 19:34
@RadiantUwU
Copy link
Contributor

image
I've tested the uniform struct out, it's a wildly amazing idea, but it doesn't work.
It seems to be either writing 2 floats before the actual vec4, or reading 2 floats after the vec4

shader_type spatial;
render_mode unshaded;

instance uniform vec3 color : source_color;

struct RayColorIdk {
	vec3 orig;
	vec3 dir;
	vec4 color;
};
uniform RayColorIdk ray;

void fragment() {
	ALBEDO = ray.color.rgb;
	//EMISSION = color;
}

This is the entirety of the code used in the example, hope this helps!

@RadiantUwU
Copy link
Contributor

Another issue spotted is me being able to delete fields or add fields to that dictionary. Other than that, amazing!

@Chaosus
Copy link
Member Author

Chaosus commented Jun 26, 2024

I've tested the uniform struct out, it's a wildly amazing idea, but it doesn't work.

Yeah, this is because that struct layout is not aligned to 16 bytes, if you add a member to it, it will work:

struct RayColorIdk {
	vec3 orig;
	float reserved;
	vec3 dir;
	float reserved2;
	vec4 color;
};

We should probably add reserved members internally.

Another issue spotted is me being able to delete fields or add fields to that dictionary.

Well, this is ok, at least for now, as noted in the topic description. If the member is absent, the default value (0) is passed to the compiler.

@Chaosus Chaosus force-pushed the shader_struct_uniform_support branch 3 times, most recently from 674f7c4 to ae68a00 Compare June 26, 2024 05:31
@Chaosus
Copy link
Member Author

Chaosus commented Jun 26, 2024

It seems to be either writing 2 floats before the actual vec4, or reading 2 floats after the vec4

@RadiantUwU I think I've fixed it!

@RadiantUwU
Copy link
Contributor

RadiantUwU commented Jun 26, 2024

RadiantUwU@6d9cfab
This would fix the issue of the tests failing. (Line numbers are different for me since this is another godot fork)

@Chaosus Chaosus force-pushed the shader_struct_uniform_support branch from ae68a00 to c3e7ccc Compare June 26, 2024 06:55
@Chaosus
Copy link
Member Author

Chaosus commented Jun 26, 2024

Also fixed the dictionary in the inspector to reflect the changes from the shader.

@Chaosus Chaosus force-pushed the shader_struct_uniform_support branch 10 times, most recently from 322287b to 6fc0953 Compare June 26, 2024 10:13
@Chaosus Chaosus force-pushed the shader_struct_uniform_support branch 13 times, most recently from 4bd77c5 to 33a323b Compare June 28, 2024 08:41
for (int i = 0; i < uniform.array_size; i++) {
Dictionary dict;
for (const ShaderLanguage::ShaderNode::Uniform::Member &member : uniform.members) {
dict[member.name] = ShaderLanguage::get_default_datatype_value(member.type, member.array_size, ShaderLanguage::ShaderNode::Uniform::HINT_NONE);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if the user has an initialization for the struct?

Like for example, having a default for the struct

struct A {
    vec3 col;
    float alph;
};
uniform A struct_uniform = A(vec3(1.0),1.0);

Wouldn't this just skip over it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's currently not implemented as stated in the topic header.

@Chaosus Chaosus force-pushed the shader_struct_uniform_support branch from 33a323b to 8afc8f3 Compare June 28, 2024 10:53
Copy link
Contributor

@RadiantUwU RadiantUwU left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine for the most part, i'd recommend in a future pull request to implement a way for developers to have a way of setting default values.
image
LGTM
Can't wait to see this into 4.4 stable

@Chaosus
Copy link
Member Author

Chaosus commented Jun 28, 2024

Looks fine for the most part, i'd recommend in a future pull request to implement a way for developers to have a way of setting default values.

It's a draft pull request that mean I will add new features and bug fixes until a certain point in time. Don't worry about default values, it's on the list to implement.

@Chaosus Chaosus force-pushed the shader_struct_uniform_support branch 7 times, most recently from dcb73aa to 118da91 Compare June 29, 2024 10:44
@Chaosus
Copy link
Member Author

Chaosus commented Jun 29, 2024

Added support for the structs inside structs (they will be parsed as mixed Array/Dictionary hierarchy):
изображение

@Chaosus Chaosus force-pushed the shader_struct_uniform_support branch 2 times, most recently from 15e34cf to 59e3386 Compare June 29, 2024 11:15
@Chaosus Chaosus force-pushed the shader_struct_uniform_support branch from 59e3386 to 1190af5 Compare June 29, 2024 11:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for uniform structs in the shader language
4 participants