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

Remove "equal to" in the GL_INVALID_VALUE error of glGetActiveUniformsiv #118

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

andylin-hao
Copy link

The document of glGetActiveUniformsiv states that GL_INVALID_VALUE is generated if uniformCount is greater than or equal to the value of GL_ACTIVE_UNIFORMS for program.

However, it's reasonable for OpenGL programs to give a value of uniformCount equaling the number of active uniforms in a specified shader program. This is because uniformCount specifies the length of the uniform array passed to glGetActiveUniformsiv. As long as this value does not exceed (i.e., is greater than) the actual number of active uniform variables, things should be fine.

@andylin-hao
Copy link
Author

andylin-hao commented Jun 16, 2022

I also find that the error message of glTexStorage3D is not correct in certain ES versions. The concerned error message states that GL_INVALID_VALUE is generated if target is GL_TEXTURE_2D_ARRAY or GL_TEXTURE_CUBE_MAP_ARRAY and width and height are not equal, or depth is not a multiple of six. However, as stated in glTexStorage3D's documentation, when target is GL_TEXTURE_2D_ARRAY or GL_TEXTURE_CUBE_MAP_ARRAY, glTexStorage3D is equivalent to:

    for (i = 0; i < levels; i++)
    {
        glTexImage3D(target, i, internalformat, width, height, depth, 0, format, type, NULL);
        width = max(1, (width / 2));
        height = max(1, (height / 2));
    }

That is, glTexStorage3D should provide similar error message as glTexImage3D. However, in the document of glTexImage3D we can see that the same error is generated only when the target is GL_TEXTURE_CUBE_MAP_ARRAY, but not when the target is GL_TEXTURE_2D_ARRAY. This is reasonable because only cube requires 6 * N surfaces and width==height.

In all, I think GL_TEXTURE_2D_ARRAY should be removed from the error message in glTexStorage3D's document.

@oddhack
Copy link
Contributor

oddhack commented Sep 14, 2023

@pdaniell-nv this is interesting, because there's no error corresponding to this in the API spec, and probably should be. This call is constructed as "equivalent to a later, more generic call" with a loop over 0..uniformCount-1, but even in GL 4.2 before the more generic call was introduced, there's no error for this. I suspect the right answer is to update the error here as suggested by @andylin-hao, and add this error to the spec (and to other calls that behave similarly, like GetUniformIndices).

@pdaniell-nv
Copy link
Collaborator

@pdaniell-nv this is interesting, because there's no error corresponding to this in the API spec, and probably should be. This call is constructed as "equivalent to a later, more generic call" with a loop over 0..uniformCount-1, but even in GL 4.2 before the more generic call was introduced, there's no error for this. I suspect the right answer is to update the error here as suggested by @andylin-hao, and add this error to the spec (and to other calls that behave similarly, like GetUniformIndices).

I think the GL_INVALID_VALUE is generated if uniformCount is greater than or equal to the value of GL_ACTIVE_UNIFORMS for program. statement in https://registry.khronos.org/OpenGL-Refpages/gl4/html/glGetActiveUniformsiv.xhtml is totally bogus and should just be removed. The only use of "uniformCount" is for the size of the app provided uniformIndices and params parameters and doesn't depend on the active uniforms in the program.

@andylin-hao
Copy link
Author

Thanks for the confirmation, guys! Let me know if you need me to make changes to the PR.

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.

None yet

3 participants