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 spatial light shader built in for light_specular property #75055

Merged
merged 1 commit into from
May 5, 2023

Conversation

JohanAR
Copy link
Contributor

@JohanAR JohanAR commented Mar 18, 2023

Light3D has a light_specular property which is used to set the intensity of specular contributed by this light source, but it was previously only used by the default material light shader, and not possible to use in a custom light() shader.

@JohanAR JohanAR requested a review from a team as a code owner March 18, 2023 09:11
@JohanAR
Copy link
Contributor Author

JohanAR commented Mar 18, 2023

I don't think it's ideal to have both LIGHT_SPECULAR (this) and SPECULAR_LIGHT (output from light shader), but there are some arguments for it IMO:

  • The property on Light3D is named "light_specular", although if I read the code correctly, specular_amount is actually 2.0 * light_specular so it's not exactly the same.
  • I think it makes it more clear that this specular value comes from the light source and it's not the specular value set on a StandardMaterial3D.
  • It matches LIGHT_COLOR, which also comes from a Light3D property.

I did not try to change the gles3 renderer because I couldn't get omni or spot lights working in a custom light shader there, they did not appear to contribute any light at all.

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 seems very helpful!

2 things:

  1. Changes should be made for the OpenGL renderer as well
  2. While I agree with you that LIGHT_SPECULAR would be the ideal name, the similarity to SPECULAR_LIGHT is just too close to be ignored. Unfortunately, I think that SPECULAR_AMOUNT would be a better name.

Light3D has a light_specular property which is used to set the
intensity of specular contributed by this light source, but it was
previously only used by the default material light shader, and not
possible to use in a custom light() shader.
@JohanAR
Copy link
Contributor Author

JohanAR commented May 4, 2023

Thanks for the review. Updated according to suggestions.

Now I got omni lights working in the gles3 renderer without issues, dunno why it didn't last time.

@JohanAR
Copy link
Contributor Author

JohanAR commented May 4, 2023

Docs update godotengine/godot-docs#7277

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!

@clayjohn clayjohn modified the milestones: 4.x, 4.1 May 4, 2023
@clayjohn
Copy link
Member

clayjohn commented May 4, 2023

Note to whoever merges this, the Docs PR can be merged right after. godotengine/godot-docs#7277

@mhilbrunner mhilbrunner merged commit 5e9b7c3 into godotengine:master May 5, 2023
@mhilbrunner
Copy link
Member

Merged, merged the docs PR as well - thanks for contributing!

@akien-mga akien-mga changed the title Add spatial light shader built in for light_specular property Add spatial light shader built in for light_specular property May 10, 2023
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.

4 participants