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 missing shader built-in overloads #53066

Merged
merged 1 commit into from
Oct 10, 2021

Conversation

Chaosus
Copy link
Member

@Chaosus Chaosus commented Sep 25, 2021

No description provided.

@Chaosus Chaosus requested a review from a team as a code owner September 25, 2021 18:18
@Chaosus Chaosus added this to the 4.0 milestone Sep 25, 2021
@Chaosus Chaosus changed the title Added scalar versions of 'dot' and 'distance' to shader language Add scalar versions of dot and distance to shader language Sep 25, 2021
@Chaosus Chaosus changed the title Add scalar versions of dot and distance to shader language [WIP] Add missing shader built-in overloads Sep 25, 2021
@Chaosus Chaosus requested a review from a team as a code owner September 25, 2021 19:11
@Chaosus Chaosus force-pushed the shader_funcs branch 2 times, most recently from 2d4e2c1 to e125b58 Compare September 25, 2021 19:16
@Chaosus Chaosus closed this Sep 25, 2021
@Chaosus Chaosus deleted the shader_funcs branch September 25, 2021 19:35
@Chaosus Chaosus restored the shader_funcs branch October 4, 2021 14:32
@Chaosus Chaosus reopened this Oct 4, 2021
@Chaosus Chaosus changed the title [WIP] Add missing shader built-in overloads Add missing shader built-in overloads Oct 4, 2021
@Chaosus Chaosus changed the title Add missing shader built-in overloads [WIP] Add missing shader built-in overloads Oct 4, 2021
@Chaosus Chaosus changed the title [WIP] Add missing shader built-in overloads Add missing shader built-in overloads Oct 4, 2021
@Chaosus Chaosus changed the title Add missing shader built-in overloads [WIP] Add missing shader built-in overloads Oct 4, 2021
@akien-mga akien-mga removed the archived label Oct 4, 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.

I dont think all of the overloads you added are supported.

Here is what the spec has to say:

When the built-in functions are specified below, where the input arguments (and corresponding output) can be float, vec2, vec3, or vec4, genType is used as the argument. Where the input arguments (and corresponding output) can be int, ivec2, ivec3, or ivec4, genIType is used as the argument. Where the input arguments (and corresponding output) can be uint, uvec2, uvec3, or uvec4, genUType is used as the argument. Where the input arguments (or corresponding output) can be bool, bvec2, bvec3, or bvec4, genBType is used as the argument. For any specific use of a function, the actual types substituted for genType, genIType, genUType, or genBType have to have the same number of components for all arguments and for the return type. Similarly for mat, which can be any matrix basic type.
(pg 80 https://www.khronos.org/registry/OpenGL/specs/gl/GLSLangSpec.3.30.pdf)

step() for example has two overloads:

genType step (genType edge, genType x)
genType step (float edge, genType x)

So I don't think that the overloads added are valid.

@Chaosus
Copy link
Member Author

Chaosus commented Oct 5, 2021

@clayjohn Strange, they are working when testing with SHADERed and the Godot itself, this seems like a non-standard extension then? Ok, I will clean up this PR, and add some missing stuff like texture functions with Offset which GLES3 is supported.

@clayjohn
Copy link
Member

clayjohn commented Oct 5, 2021

@clayjohn Strange, they are working when testing with SHADERed and the Godot itself, this seems like a non-standard extension then? Ok, I will clean up this PR, and add some missing stuff like texture functions with Offset which GLES3 is supported.

I wonder if GLSL does an implicit type-conversion of the arguments passed into the functions. In which case, maybe we should allow them...
Edit: Looks like the compiler does an implicit conversion to the floating point equivalents, see: http://shader-playground.timjones.io/5dbb4aa04a7491496fa6fd78c04ce6fd

And for reference: GLSL 330 spec at section 4.1.10 "implicit conversions" and section 6.1 "function definitions"

@Chaosus
Copy link
Member Author

Chaosus commented Oct 5, 2021

I see, then I will remove that conversion overload stuff and for the future, I should probably implement that conversion implicitly.

@Chaosus Chaosus requested a review from clayjohn October 5, 2021 17:53
@Chaosus Chaosus changed the title [WIP] Add missing shader built-in overloads Add missing shader built-in overloads Oct 5, 2021
@Chaosus
Copy link
Member Author

Chaosus commented Oct 5, 2021

Added packing/unpacking functions (supersedes #52767) and added functions described in https://www.khronos.org/registry/OpenGL/specs/es/3.1/GLSL_ES_Specification_3.10.withchanges.pdf (bitfieldExtract, bitfieldInsert, bitfieldReverse, bitCount, findLSB, findMSB, uaddCarry, usubBorrow)

@akien-mga akien-mga merged commit 09b0293 into godotengine:master Oct 10, 2021
@akien-mga
Copy link
Member

Thanks!

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.

3 participants