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 PI, TAU and E constants to the shader language (3.x) #52315

Closed
wants to merge 1 commit into from

Conversation

Calinou
Copy link
Member

@Calinou Calinou commented Sep 1, 2021

3.x version of #48837.

Preview

image

@Calinou Calinou requested a review from a team as a code owner September 1, 2021 15:33
@Calinou Calinou added this to the 3.4 milestone Sep 1, 2021
@Calinou Calinou force-pushed the shader-add-constants-3.x branch from 3c7710c to 1810df8 Compare September 1, 2021 15:34
@Calinou Calinou changed the title Add PI, TAU and E constants to the shader language Add PI, TAU and E constants to the shader language (3.x) Sep 1, 2021
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! Just need to add it to GLES2 as well!

@lawnjelly
Copy link
Member

Is there any other standard name for the constant E? i.e. could we 'longify it' in some way? It just struck me that in some situations it could be a pain as this is global namespace, and if you were trying to convert a shader which used capitalized variable e.g. A, B, C, D, E etc, it could be an annoyance. Maybe I'm overthinking it though. 🤔

@aaronfranke
Copy link
Member

aaronfranke commented Sep 1, 2021

@lawnjelly Unfortunately it's hard to lengthen it without sounding awkward. Are NATURAL_EXPONENTIAL or EULERS_NUMBER better? E is the base of the natural exponential function and is also called Euler's number (since it was discovered by Bernoulli 🙃)

@Calinou
Copy link
Member Author

Calinou commented Sep 1, 2021

Is there any other standard name for the constant E? i.e. could we 'longify it' in some way? It just struck me that in some situations it could be a pain as this is global namespace, and if you were trying to convert a shader which used capitalized variable e.g. A, B, C, D, E etc, it could be an annoyance. Maybe I'm overthinking it though. thinking

While I think a global E constant would be problematic in a traditional programming language, it's not as much of an issue in a shader language.

@Calinou
Copy link
Member Author

Calinou commented Sep 25, 2021

Added support for the new constants in GLES2:

image

@Calinou Calinou force-pushed the shader-add-constants-3.x branch from 1810df8 to d43e5c5 Compare September 25, 2021 22:36
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!

I am a little worried about breaking user's shaders when they use the E constant. I've seen a lot of users copy-paste the following code for the "Uncharted 2 Tonemapping function"

float A = 0.15;
float B = 0.50;
float C = 0.10;
float D = 0.20;
float E = 0.02;
float F = 0.30;
float W = 11.2;

vec3 Uncharted2Tonemap(vec3 x)
{
   return ((x*(A*x+C*B)+D*E)/(x*(A*x+B)+D*F))-E/F;
}

Source

Given that shader programming often favours short variable names, I'd wager that more than a few users are already using E in their code and adding the global define will break their code.

That being said, the error message is helpful (if they can just track down which shader is resulting in the error)

My gut is telling me to err on the side of caution and not risk breaking user code in the 3.x branch.

@lawnjelly
Copy link
Member

EULERS_NUMBER or anything with a bunch of characters is far better imo. E is just a disaster waiting to happen. 🤦 😁

@aaronfranke
Copy link
Member

It's been awhile since I posted my comment, now I think NATURAL_EXPONENTIAL would be better. It's possible that people would get Euler's number mixed up with Euler angles since those are very common (there's also Euler's constant 0.5772156649, but it's not common in gamedev).

@akien-mga akien-mga modified the milestones: 3.4, 3.5 Nov 8, 2021
@akien-mga akien-mga force-pushed the 3.x branch 2 times, most recently from 71cb8d3 to c58391c Compare January 6, 2022 22:40
@akien-mga
Copy link
Member

At this stage, I'm not sure we should bother adding this kind of convenience features to the 3.x branch. 4.0 is close to beta and most tutorials for 3.x have already been written and published, so adding new features like this will only be noticed by a few users and may not be worth the trouble.

I'm not opposed to it either, but generally speaking we still have a lot of feature PRs targeting 3.x which for the most part are QoL improvements that are not particularly asked for (but are good changes made in master for incremental improvement of UX).

@akien-mga akien-mga modified the milestones: 3.5, 3.x Jul 3, 2022
@clayjohn
Copy link
Member

clayjohn commented Jul 4, 2022

At this stage, I'm not sure we should bother adding this kind of convenience features to the 3.x branch. 4.0 is close to beta and most tutorials for 3.x have already been written and published, so adding new features like this will only be noticed by a few users and may not be worth the trouble.

I'm not opposed to it either, but generally speaking we still have a lot of feature PRs targeting 3.x which for the most part are QoL improvements that are not particularly asked for (but are good changes made in master for incremental improvement of UX).

I tend to agree, this change will break at least a few users shaders and will provide only a slight QoL improvement.

@aaronfranke
Copy link
Member

One option is to add PI and TAU, but not E. This should avoid breaking compatibility with shaders like @clayjohn posted.

@clayjohn
Copy link
Member

clayjohn commented Jul 4, 2022

One option is to add PI and TAU, but not E. This should avoid breaking compatibility with shaders like @clayjohn posted.

Yeah, for PI and TAU it's less of a big deal because users are almost definitely using them for the mathematical values. So, even though this change would break their shaders, they could just delete their local definition of the constant and then the shader would work fine again.

Personally, I still don't think it is worth the annoyance. But if there is demand from users than we can definitely consider adding PI and TAU to a 3.x release

@charliewhitfield
Copy link

charliewhitfield commented Nov 24, 2022

Just to give a concrete example, the E constant will break existing shaders in our astronomical simulator @ivoyager:

In orbital mechanics, 'E' has a conventional meaning (different than 'e'). Although it violates variable capitalization convention, we prioritize orbital mechanics convention in specific math functions in our code (including in shaders):

a // semi-major axis
e // eccentricity
i // inclination
Om // longitude of the ascending node
w // argument of periapsis
M0 // mean anomaly at epoch
n // mean motion
M // mean anomaly
E // eccentric anomaly

I suppose this is a done deal for 4.0? I might open a proposal to change but I'm not sure if my edge case is worth anyone's time...

@clayjohn
Copy link
Member

I suppose this is a done deal for 4.0? I might open a proposal to change but I'm not sure if my edge case is worth anyone's time...

Yes, this is a done deal for 4.0 unfortunately. Sorry to hear that this will break your team's shaders. Feel free to open a proposal, but know it would be targeting Godot 5.0

@Calinou
Copy link
Member Author

Calinou commented Jun 27, 2023

Closing per the above comments due to concerns with compatibility.

@Calinou Calinou closed this Jun 27, 2023
@YuriSizov YuriSizov removed this from the 3.x milestone Dec 2, 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.

7 participants