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 a default shader specular render mode to SCHLICK_GGX #51155

Merged
merged 1 commit into from
Aug 9, 2021

Conversation

Chaosus
Copy link
Member

@Chaosus Chaosus commented Aug 1, 2021

Makes shader pipeline use SPECULAR_SCHLICK_GGX(desktop) and SPECULAR_BLINN(mobile) - parameter by default (and if the user didn't define render_mode directly).

revert #41533

Bugsquad edit: Fixes #46838.

@Chaosus Chaosus requested review from a team as code owners August 1, 2021 20:22
@Chaosus Chaosus added this to the 4.0 milestone Aug 1, 2021
@Chaosus Chaosus requested a review from clayjohn August 1, 2021 20:24
@Chaosus Chaosus force-pushed the shader_fix_specular_mode branch 3 times, most recently from 8ccee4c to 7fb6868 Compare August 2, 2021 08:42
@akien-mga akien-mga requested a review from BastiaanOlij August 2, 2021 11:00
@Chaosus Chaosus force-pushed the shader_fix_specular_mode branch 2 times, most recently from 5cbf8f8 to c83c279 Compare August 5, 2021 11:04
@reduz
Copy link
Member

reduz commented Aug 5, 2021

I would really prefer to not add so much stuff in the shaders, what was the original problem?

@Chaosus
Copy link
Member Author

Chaosus commented Aug 5, 2021

what was the original problem?

#46838

@Chaosus Chaosus force-pushed the shader_fix_specular_mode branch from c83c279 to ef69238 Compare August 6, 2021 10:12
Copy link
Contributor

@BastiaanOlij BastiaanOlij left a comment

Choose a reason for hiding this comment

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

I would be fine with the change as shown, my only remark was that the setting between GGX/BLINN was over kill so I am happy with the current solution to make GGX the default on desktop and BLINN the default on mobile. The check at the top of the shader makes it very clear to anyone reading the code what the desired behavior is.

Do need to make sure this is properly documented.

@Calinou
Copy link
Member

Calinou commented Aug 6, 2021

I would be fine with the change as shown, my only remark was that the setting between GGX/BLINN was over kill so I am happy with the current solution to make GGX the default on desktop and BLINN the default on mobile.

According to #22637, Blinn could be removed entirely since GGX has the same number of instructions. Maybe Phong should be removed too, I'm not sure.

@Chaosus Chaosus force-pushed the shader_fix_specular_mode branch from ef69238 to 47a6bb2 Compare August 6, 2021 13:02
@Chaosus
Copy link
Member Author

Chaosus commented Aug 6, 2021

@Calinou Ok changed to schilck in both cases.

@Chaosus Chaosus changed the title Fix a default shader specular render mode to (SCHLICK_GGX/BLINN) Fix a default shader specular render mode to (SCHLICK_GGX) Aug 6, 2021
@clayjohn
Copy link
Member

clayjohn commented Aug 6, 2021

As a note to whoever merges this #46838 is a 3.x issue and should not be closed by this PR.

@Chaosus Chaosus changed the title Fix a default shader specular render mode to (SCHLICK_GGX) Fix a default shader specular render mode to SCHLICK_GGX Aug 8, 2021
@akien-mga akien-mga merged commit 85399a9 into godotengine:master Aug 9, 2021
@akien-mga
Copy link
Member

Thanks!

@Chaosus Chaosus deleted the shader_fix_specular_mode branch August 9, 2021 06:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants