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

Fix length() array function usage in shader #49456

Merged
merged 1 commit into from
Jun 10, 2021

Conversation

Chaosus
Copy link
Member

@Chaosus Chaosus commented Jun 9, 2021

I think I broke it in #48933:

image

Update:
Also fixes incorrect array element size returning from the arrays:

image

@Chaosus Chaosus requested a review from a team as a code owner June 9, 2021 11:14
@Chaosus Chaosus added this to the 4.0 milestone Jun 9, 2021
@Chaosus Chaosus force-pushed the fix_shader_length branch from 297a6f1 to 7dd65a0 Compare June 10, 2021 07:04
@lawnjelly
Copy link
Member

Just out of interest because I don't know when this was put in, how come it is length and not size, as size seems to be used throughout Godot? (that's presuming length is to give how many elements in the array)

@Chaosus
Copy link
Member Author

Chaosus commented Jun 10, 2021

@lawnjelly it's from GLSL, I don't know whether it's a good idea to modify it to size - it may break user's shaders

@Chaosus Chaosus requested a review from clayjohn June 10, 2021 07:45
@lawnjelly
Copy link
Member

@lawnjelly it's from GLSL, I don't know whether it's a good idea to modify it to size - it may break user's shaders

Ah if it is GLSL standard, then agree that is best to stick with, even if different from Godot. 👍

Copy link
Member

@lawnjelly lawnjelly 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 to me (although I'm not up on the shader compiler in 4.x).

@Chaosus Chaosus merged commit 9539e4e into godotengine:master Jun 10, 2021
@Chaosus Chaosus deleted the fix_shader_length branch June 10, 2021 08:08
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.

2 participants