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

Added constants PI, TAU and E to the shader language #48837

Merged
merged 1 commit into from
May 25, 2021

Conversation

Soupstraw
Copy link
Contributor

@Soupstraw Soupstraw commented May 19, 2021

I added some of the more commonly used constants to the shading language.
Issue #20726

Bugsquad edit: Closes #20726. master version of #52315.

@Soupstraw Soupstraw requested a review from a team as a code owner May 19, 2021 12:19
@akien-mga akien-mga requested a review from a team May 19, 2021 12:27
@akien-mga akien-mga added this to the 4.0 milestone May 19, 2021
@Chaosus
Copy link
Member

Chaosus commented May 19, 2021

Yep, checked, seems worked correctly. Thanks for your first contribution to Godot Engine! These constants are really useful.

@Soupstraw Soupstraw requested a review from a team as a code owner May 19, 2021 12:49
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.

A couple thoughts

  1. It may be easier to just define the constants in the .cpp files with actions.rename
  2. You shouldn't define the extra constants in shaders that don't use them. For example, cubemap roughness and cubemap down sampler do not use TAU or E. So there is no need to touch them.

@Soupstraw Soupstraw requested a review from clayjohn May 19, 2021 14:52
@akien-mga
Copy link
Member

Note that once this is ready, we'll ask you to squash the commits into one before this is merged. See PR workflow on instructions on how to rebase/squash commits in your branch.

@Soupstraw Soupstraw force-pushed the shader-pi branch 5 times, most recently from 520f5a4 to 2013d31 Compare May 20, 2021 09:40
@Soupstraw Soupstraw requested a review from clayjohn May 20, 2021 10:30
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.

Implementation looks fine to me, but I question why we are just throwing a bunch of constants into the shader code without considering need.

For example, where would LN2 be useful? I don't even know what constant it represents. And it appears it is only used once in the entire Godot codebase.

@Soupstraw
Copy link
Contributor Author

They just happened to be defined in the same file as PI and TAU. I guess they aren't necessary as long as the shader compiler is smart enough to compute ln(2) at compile time.

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.

Thanks for the clarification. And yeah, I think it's best to add the requested constants for now and if anyone requests the others we can add them the same way.

@akien-mga akien-mga merged commit f1abfbb into godotengine:master May 25, 2021
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

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.

PI is not defined in shaders
4 participants