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

Added support for uniform arrays in shaders #49485

Merged
merged 1 commit into from
Oct 4, 2021

Conversation

Chaosus
Copy link
Member

@Chaosus Chaosus commented Jun 10, 2021

A draft pull request which intended to add support for uniform arrays to shaders and should fix godotengine/godot-proposals#931

Currently, this adds support only for scalar shader types like float, vec* or mat*. Support for texture samplers will be later.
image

I'm not sure whether I did everything correct - especially about offsets of binding of array uniforms in the renderer. There are some bugs that lead to the shader crash which I should fix somehow. Anyway, if this PR is not correct - it can be salvageable by the user with more experience in rendering than me (the part about shader editor is fully complete).

@Chaosus Chaosus requested review from a team as code owners June 10, 2021 12:40
@Chaosus Chaosus marked this pull request as draft June 10, 2021 12:43
@Chaosus Chaosus force-pushed the shader_uniform_arrays branch from 4a7bd1f to c433554 Compare June 10, 2021 13:09
Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Just some early comments. I still need to review the renderer_storage_rd.cpp file

servers/rendering/shader_language.cpp Outdated Show resolved Hide resolved
servers/rendering/shader_language.cpp Show resolved Hide resolved
@clayjohn
Copy link
Member

The rest looks fine to me too. I wonder about the data alignment stuff. I notice that with arrays you are adding in padding manually (e.g. with bool, you are storing the array like [b1, 0, 0, 0, b2, 0, 0, 0,...] but when not using arrays we are just adding a single bool without padding. That seems odd to me.

I also wonder why the alignment for mat2, mat3, and mat4 needed to change. It makes sense that you have made the alignment the same as as size. But I wonder what the reason for it always being 16 was in the first place. I will do a local build and see if I can figure out some of this stuff soon.

@clayjohn
Copy link
Member

I was trying to reproduce a crash and I ended up writing the following code:

shader_type canvas_item;

uniform bool onoff[1];
uniform vec3[5] colors;

void fragment() {
	for (int i=0;i<5;i++) {
		if (UV.x > float(i)/5.0 && onoff[i]) {
			COLOR.xyz = colors[i];
		}
	}
}

This didn't produce a crash, but it did create a bug where the values of "colors" overlapped with "onoff". This code draws 5 vertical strips over a ColorRect, the visibility of each strip is controlled by "onoff". However, when the size of "onoff" is too small it overlaps with "colors" so you end up reading the x-compenent of "colors" instead of "onoff" and you end up controlling the visibility of the strips with "colors"

@Chaosus
Copy link
Member Author

Chaosus commented Jun 11, 2021

The rest looks fine to me too. I wonder about the data alignment stuff. I notice that with arrays you are adding in padding manually (e.g. with bool, you are storing the array like [b1, 0, 0, 0, b2, 0, 0, 0,...] but when not using arrays we are just adding a single bool without padding. That seems odd to me.

I wonder that too but failed in trying to attempt using it without this alignment. @reduz also leave a comment "UBO sizes must be multiples of 16". You should check and play with: https://github.com/godotengine/godot/blob/c433554419c3c348e26a457ad09e8f5c48e536f0/servers/rendering/renderer_rd/shader_compiler_rd.cpp#L658-L671

I also wonder why the alignment for mat2, mat3, and mat4 needed to change. It makes sense that you have made the alignment the same as as size. But I wonder what the reason for it always being 16 was in the first place. I will do a local build and see if I can figure out some of this stuff soon.

Otherwise, renderer (not shader) will crash if using such arrays. That because incorrect size check.

@Chaosus
Copy link
Member Author

Chaosus commented Jun 11, 2021

I was trying to reproduce a crash and I ended up writing the following code:

The crash can be produced by:

shader_type spatial;

uniform vec3 k[3];
uniform mat4 uniform_mat4_array[2];

void fragment() {
	ALBEDO = k[1].rgb;
}

if you change the order like:

uniform mat4 uniform_mat4_array[2];
uniform vec3 k[3];

It will not crash

This didn't produce a crash, but it did create a bug where the values of "colors" overlapped with "onoff".

That interesting... need to play with this too...

@Chaosus Chaosus force-pushed the shader_uniform_arrays branch from c433554 to a751bce Compare June 11, 2021 08:54
@Chaosus
Copy link
Member Author

Chaosus commented Jun 11, 2021

@clayjohn We need to somehow pass a length (and default values?) to the array property to prevent the user from setting the array size manually in the inspector (and lock a size property for these arrays).

@clayjohn
Copy link
Member

We need to somehow pass a length (and default values?) to the array property to prevent the user from setting the array size manually in the inspector (and lock a size property for these arrays).

Yes, it looks like we will probably have to make changes to the editor code that determines how arrays are displayed.

@sairam4123
Copy link

Will there be support for passing custom object arrays in Shaders?

@Chaosus
Copy link
Member Author

Chaosus commented Jun 28, 2021

Will there be support for passing custom object arrays in Shaders?

Do you mean objects declared with struct? I doubt with this PR, it's too complex.

@sairam4123
Copy link

sairam4123 commented Jun 28, 2021 via email

@YuriSizov YuriSizov added this to the 4.0 milestone Jul 18, 2021
@Chaosus Chaosus force-pushed the shader_uniform_arrays branch from a751bce to 7b3e510 Compare July 19, 2021 05:05
@Chaosus Chaosus force-pushed the shader_uniform_arrays branch 4 times, most recently from d3832b7 to 8d650dc Compare August 10, 2021 07:59
@Chaosus
Copy link
Member Author

Chaosus commented Aug 10, 2021

@clayjohn I think I fixed the crashes, and revert the values back to normal (just a changing algorithm of finding offset a bit)

@Chaosus Chaosus force-pushed the shader_uniform_arrays branch 4 times, most recently from 369e1c1 to 7686f81 Compare August 10, 2021 09:46
@clayjohn
Copy link
Member

@Chaosus Great! I will take a look and test it out again.

@Chaosus Chaosus force-pushed the shader_uniform_arrays branch from 7686f81 to 67e70a3 Compare August 11, 2021 08:26
@19PHOBOSS98
Copy link
Contributor

19PHOBOSS98 commented Aug 29, 2021

Hi! Is there a chance we can have this in the next 3.4 update? really need it for my RTX shader :)

@Chaosus
Copy link
Member Author

Chaosus commented Aug 29, 2021

@19PHOBOSS98 No, of course:

  1. 3.4 is near to release and this means no new big features are acceptable.
  2. This feature is still under development and has not been merged to master.

@Chaosus Chaosus force-pushed the shader_uniform_arrays branch from 67e70a3 to dfaf746 Compare August 29, 2021 06:31
@Chaosus Chaosus force-pushed the shader_uniform_arrays branch 3 times, most recently from 98fd653 to 1e99d04 Compare September 25, 2021 07:28
@Chaosus Chaosus force-pushed the shader_uniform_arrays branch 2 times, most recently from 53c7f37 to f251323 Compare September 25, 2021 12:47
@Chaosus
Copy link
Member Author

Chaosus commented Sep 25, 2021

I finally managed to correctly setup texture arrays:
texture_arrays
@clayjohn can you test again?

you can grab a test project:
Test.zip

@Chaosus Chaosus requested a review from clayjohn September 25, 2021 13:45
@clayjohn
Copy link
Member

@Chaosus Certainly I will test and re-review as soon as I can. :) Great work by the way!

@Chaosus Chaosus force-pushed the shader_uniform_arrays branch from f251323 to 436c483 Compare October 3, 2021 06:37
@clayjohn
Copy link
Member

clayjohn commented Oct 4, 2021

Looks mostly good, besides that one comment everything seems great.

Do you have anything else you want to add before you take this out of draft and it is ready to merge?

@Chaosus
Copy link
Member Author

Chaosus commented Oct 4, 2021

@clayjohn Indeed, but there is an editor issue that described in godotengine/godot-proposals#3149 - it's not comfortable to use for the users now + texture arrays in the inspector are not supported and the only way to pass texture array is using scripts. I worry that it would not pass a quality control of the Godot.

@Chaosus Chaosus force-pushed the shader_uniform_arrays branch from 436c483 to 6873eca Compare October 4, 2021 11:00
@Chaosus Chaosus marked this pull request as ready for review October 4, 2021 11:23
@Chaosus
Copy link
Member Author

Chaosus commented Oct 4, 2021

Well, maybe I should create a new PR regarding that stuff later and this is good to merge for now.

Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Looks good for now!

Indeed it is a concern that users can set the arrays to arbitrary sizes, but that is a problem with the editor code for displaying arrays and likely should be made in another PR. Creating arrays of textures in the editor would be a nice feature, but I'm not sure how much it is needed. My guess is that most people will be updating the array uniforms from script rather than from editor anyway.

@Chaosus
Copy link
Member Author

Chaosus commented Oct 4, 2021

Alright, let's merge it - if there are bugs/regressions we have time to fix them during alpha.

@Chaosus Chaosus merged commit 87e7f79 into godotengine:master Oct 4, 2021
@Chaosus Chaosus deleted the shader_uniform_arrays branch October 4, 2021 16:16
@davids91
Copy link

Hey! Sorry I might understand this incorrectly but I suppose this feature should be supported in https://github.com/godotengine/godot/releases/tag/3.4-stable ?
Or is it 4.x only?

@Chaosus
Copy link
Member Author

Chaosus commented Nov 29, 2021

Only 4.0 - but you can try to ask @lyuma to attempt to port it.

@nobuyukinyuu
Copy link
Contributor

@clayjohn

Creating arrays of textures in the editor would be a nice feature, but I'm not sure how much it is needed. My guess is that most people will be updating the array uniforms from script rather than from editor anyway.

One use case scenario I can think of would be to store palettes as an array of GradientTexture, as with gradient mapping for a palletizing shader. It's fine if this can still be done in tool code to create a material object with the params that can be saved at design time, but it definitely would be nicer to be able to add more entries and modify them directly from the editor.

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.

Support uniform arrays in the shader language
7 participants