Skip to content

Shader: Fix the default behavior when mat uniforms are null #106592

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 22, 2025
Merged

Shader: Fix the default behavior when mat uniforms are null #106592

merged 1 commit into from
May 22, 2025

Conversation

beicause
Copy link
Contributor

Fixes #106561.

@beicause beicause changed the title ShaderMaterial: Fix default parameter behavior in editor ShaderMaterial: Fix mat default value behavior in editor after setting shader May 19, 2025
@beicause beicause force-pushed the shader-fix-editor-mat-default branch 2 times, most recently from d9c943b to 9aa0166 Compare May 19, 2025 13:23
@AThousandShips AThousandShips requested a review from a team May 19, 2025 16:21
@AThousandShips AThousandShips added this to the 4.5 milestone May 19, 2025
@AThousandShips AThousandShips added the cherrypick:4.4 Considered for cherry-picking into a future 4.4.x release label May 19, 2025
clayjohn
clayjohn requested changes May 20, 2025
View reviewed changes
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.

Editor code should not be in the scene/ folder. There are a few places where it is unavoidable, but I don't think this is one of those places since this only impacts mat types and not the other types. I think you should take a look at how other types are handled and then see if mat is just missed somewhere

@beicause
Copy link
Contributor Author

beicause commented May 21, 2025 •
edited
Loading

I think the reason is that when the ShaderMaterial and Shader are newly created, the material parameters are not set, but get_property_list has already generated default values as properties. The default values of mat (and color alpha I guess, not tested yet) are not zero.

Perhaps this behavior should also be fixed for the game, not just for the editor

@beicause beicause force-pushed the shader-fix-editor-mat-default branch from 9aa0166 to 996680b Compare May 21, 2025 04:55
@beicause beicause changed the title ShaderMaterial: Fix mat default value behavior in editor after setting shader Shader: Fix the default behavior when mat uniforms are null May 21, 2025
@beicause
Copy link
Contributor Author

beicause commented May 21, 2025 •
edited
Loading

Pushed another solution.

The alpha of color is already handled before _fill_std140_ubo_empty(). So it makes sense to deal with mat in the same way.

@beicause beicause force-pushed the shader-fix-editor-mat-default branch from 996680b to d97948d Compare May 21, 2025 05:04
@beicause beicause force-pushed the shader-fix-editor-mat-default branch from d97948d to 4621d3e Compare May 21, 2025 05:28
clayjohn
clayjohn approved these changes May 22, 2025
View reviewed changes
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.

This looks great! Thank you for taking another look and finding a better solution

@clayjohn clayjohn removed the cherrypick:4.4 Considered for cherry-picking into a future 4.4.x release label May 22, 2025
@Repiteo Repiteo merged commit 452dc66 into godotengine:master May 22, 2025
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented May 22, 2025

Thanks!

@beicause beicause deleted the shader-fix-editor-mat-default branch May 23, 2025 05:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clayjohn clayjohn clayjohn approved these changes

Assignees
No one assigned
Labels
bug topic:rendering
Projects
None yet
Milestone
4.5
Development

Successfully merging this pull request may close these issues.

The matrix uniform parameter of newly created ShaderMaterial is zero matrix instead of identity matrix
4 participants
@beicause @Repiteo @clayjohn @AThousandShips