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

Cleaning the implementation of GL_EXT_texture_shadow_lod. Moving the #3345

Merged
merged 1 commit into from
Oct 2, 2023

Conversation

pmistryNV
Copy link
Contributor

@pmistryNV pmistryNV commented Sep 28, 2023

parameter verification to a centralized place where all other built-ins are verified for correctness.

@pmistryNV
Copy link
Contributor Author

@alelenv please take a look

Copy link
Collaborator

@ncesario-lunarg ncesario-lunarg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM pending CI.

@pmistryNV
Copy link
Contributor Author

I need to clean it up a bit more. Kindly hold on the merge for a day.

@alelenv
Copy link
Contributor

alelenv commented Sep 29, 2023

@ncesario-lunarg or @arcady-lunarg : Can you clarify why was TempParser added in first place?
It seems pretty inelegant way to do it compared to pmistryNV's current change
Are we missing some edge case?

@ncesario-lunarg
Copy link
Collaborator

@alelenv The "TempParser" was added as a workaround for getting the mangled name of a function, as currently the only way to get the mangled name of a function name is by running the full parser. The end goal was to associate only specific overloads of a function with a given extension (GL_EXT_texture_shadow_lod in this case). At the time I made the change it seemed like a more "direct" route than adding some manual type checking later, but I'm in favor of getting rid of the temp parser as it was always a hack.

@pmistryNV pmistryNV force-pushed the ext_texture_lod_fixup branch from fe61826 to 63739f1 Compare September 29, 2023 19:57
parameter verifictation to a centralized place where all ther builtins
are verified for correctness.

Added verification for the new builtins with version and extension check
These builtins are supported on GLSL since version 130 and GLES since
version 300
@pmistryNV pmistryNV force-pushed the ext_texture_lod_fixup branch from 63739f1 to 855417d Compare September 29, 2023 20:27
@pmistryNV
Copy link
Contributor Author

pmistryNV commented Sep 29, 2023

General feedback since glslang is the reference compiler for GLSL, it's paramount that any extension implemented should have extensive coverage:

  1. Appropriate version control check for ES and GL version.
  2. If certain arguments or signatures are not suppose to compile with certain version or with certain set of extension then that should be covered. For example for this extension there is a dependency on OES_texture_cube_map_array, EXT_texture_cube_map_array in GLES. However dependency on ARB_texture_cube_map_array is not necesary to capture because token "samplerCubeArrayShadow" is turned on by default. For any builtins TParseContext::builtInOpCheck is the appropriate place to check for any dependencies that the grammar file cannot capture.
  3. Extensive set of test cases should be added for passing and failing cases for both ES and GL.

This helps anyone using the compiler to cross validate the correctness of the spec. Having a buggy implementation makes the user confused and causes the spec to be not implemented correctly in shaders.

@pmistryNV
Copy link
Contributor Author

@ncesario-lunarg @arcady-lunarg kindly review and merge this change.

@@ -0,0 +1,160 @@
glsl.ext.textureShadowLod.frag
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that this is not an "error" test, is it supposed to be producing errors here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This covers both errors and non errors. This is in line with other glsl shaders. Where both good and bad cases are kept in the same shader file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

feel free to add more test case as a follow up change. I just wanted to clean it up. Validation is now correct. We can add more test cases for ES.

@arcady-lunarg arcady-lunarg added the kokoro:run Trigger Google bot runs label Oct 2, 2023
@kokoro-team kokoro-team removed the kokoro:run Trigger Google bot runs label Oct 2, 2023
@arcady-lunarg arcady-lunarg merged commit 5ff0c04 into KhronosGroup:main Oct 2, 2023
20 checks passed
@arcady-lunarg
Copy link
Contributor

I corrected the grammar on the commit message a bit, in general we prefer that those are written in the simple present. Thanks for the cleanup and the good commit message.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants