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

Implement per-light Specular property in DirectionalLight3D #83917

Merged

Conversation

Calinou
Copy link
Member

@Calinou Calinou commented Oct 25, 2023

@Calinou Calinou requested review from a team as code owners October 25, 2023 04:23
@Calinou Calinou force-pushed the directionallight3d-add-specular branch from 72a953a to 7faedc2 Compare October 25, 2023 04:23
@Calinou Calinou added this to the 4.2 milestone Oct 25, 2023
@clayjohn clayjohn modified the milestones: 4.2, 4.3 Oct 25, 2023
@akien-mga akien-mga modified the milestones: 4.3, 4.4 Jun 28, 2024
@Calinou Calinou force-pushed the directionallight3d-add-specular branch from 7faedc2 to b7c40ca Compare October 28, 2024 18:41
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 to me for RD. I expect a slight performance decrease when using DirectionalLights from this change as I think that part of the shader is relatively load/store bound. However, this was a missing feature so there is no way around it. I would avoid cherry picking this PR for that reason though

For compatibility, have you checked if the property is being sent from the CPU?

@Calinou
Copy link
Member Author

Calinou commented Nov 28, 2024

I expect a slight performance decrease when using DirectionalLights from this change as I think that part of the shader is relatively load/store bound.

Can we do bitpacking or use FP16 in the future to reduce the performance impact? This particular value doesn't need much precision, as it's between 0.0 and 1.0 most of the time (rarely above 3.0). Half precision float would be fine, possibly even lower if it's converted from a fixed-point value.

@clayjohn
Copy link
Member

Can we do bitpacking or use FP16 in the future to reduce the performance impact? This particular value doesn't need much precision, as it's between 0.0 and 1.0 most of the time (rarely above 3.0). Half precision float would be fine, possibly even lower if it's converted from a fixed-point value.

Bitpacking would most likely help as long as the bottleneck continues to be load/store related. You just need to find a property to pack with it

@Repiteo
Copy link
Contributor

Repiteo commented Dec 10, 2024

Needs rebase before it can be merged

@Calinou Calinou force-pushed the directionallight3d-add-specular branch from b7c40ca to 7758c72 Compare December 16, 2024 18:10
@Calinou Calinou force-pushed the directionallight3d-add-specular branch from 7758c72 to e42def1 Compare December 16, 2024 18:18
@Calinou
Copy link
Member Author

Calinou commented Dec 16, 2024

Rebased and tested, it works as expected in Forward+/Mobile. I've also managed to fix it in Compatibility (both with shadows enabled and disabled): I was replacing the wrong argument in light_compute() in drivers/gles3/scene.glsl, which is why it wasn't working previously.

While testing this, I've also found that the Specular property doesn't do anything with any light type on vertex-shaded materials. This is an issue on all rendering methods. I believe it should be addressed separately as it'll require more shader changes.

@clayjohn
Copy link
Member

While testing this, I've also found that the Specular property doesn't do anything with any light type on vertex-shaded materials. This is an issue on all rendering methods. I believe it should be addressed separately as it'll require more shader changes.

This is intentional. Neither metallic nor specular are used.

@akien-mga akien-mga merged commit d701bc0 into godotengine:master Dec 17, 2024
20 checks passed
@akien-mga
Copy link
Member

Thanks!

@Calinou Calinou deleted the directionallight3d-add-specular branch December 23, 2024 17:27
@Calinou
Copy link
Member Author

Calinou commented Dec 23, 2024

This is intentional. Neither metallic nor specular are used.

I meant the light's specular property, not the material's specular property (which is a derivative of its metallic behavior). Light3D Specular only affects the specular lobe (not other reflections), and it should be possible to make it affect vertex-shaded materials too.

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.

DirectionalLight3D currently has no Specular property
5 participants