Glxml fixes #480
Glxml fixes #480
Conversation
The 'target' parameter of the glGetInternalformati* functions belongs to the TextureTarget group. GL_RENDERBUFFER is a valid value for 'target' in the glGetInternalformati* functions. Hence, GL_RENDERBUFFER was added to the TextureTarget group.
* GL_FETCH_PER_SAMPLE_ARM (EnableCap, GetPName) * GL_FRAGMENT_SHADER_FRAMEBUFFER_FETCH_MRT_ARM (GetPName)
| @@ -9180,7 +9180,7 @@ typedef unsigned int GLhandleARB; | |||
| <param group="CheckedInt32"><ptype>GLint</ptype> <name>level</name></param> | |||
| <param group="CheckedInt32"><ptype>GLint</ptype> <name>xoffset</name></param> | |||
| <param><ptype>GLsizei</ptype> <name>width</name></param> | |||
| <param group="PixelFormat"><ptype>GLenum</ptype> <name>format</name></param> | |||
| <param group="InternalFormat"><ptype>GLenum</ptype> <name>format</name></param> | |||
There was a problem hiding this comment.
It's awesome to see that vendors are starting to pitch in on the groups!
This makes sense, but I think we need to establish a breaking change policy for groups going forwards. First Google and now Arm, it looks like even though Khronos themselves don't use the groups they're becoming of increasing importance among the members therein.
Other than that fine with this.
There was a problem hiding this comment.
If you're able to provide details on your use case for the groups this'd help, but no worries if not. I'll open a separate issue for establishing this breaking change policy.
There was a problem hiding this comment.
It's for pretty-printing traced API calls so that GLenum/GLbitfields appear with their name instead of their integer value. gl.xml is used for generating a lookup table that makes this conversion possible.
GLenum value to name is not a one-to-one mapping (e.g. 0 can be GL_POINTS or GL_ZERO). The parameter groups make it possible to retrieve the relevant set of GLenum names for a specific parameter of a specific function.
…ions The group of the 'format' parameter in the glCompressed*TexSubImage* functions was set to PixelFormat. However, the correct group seems to be InternalFormat, which is containing the valid values for that parameter.
joakim-arm
force-pushed
the
glxml-fixes
branch
from
14b7772 to
6ee5bfe
Compare
| @@ -4948,7 +4948,7 @@ typedef unsigned int GLhandleARB; | |||
| <enum value="0x8D40" name="GL_FRAMEBUFFER" group="ObjectIdentifier,FramebufferTarget,CheckFramebufferStatusTarget"/> | |||
| <enum value="0x8D40" name="GL_FRAMEBUFFER_EXT"/> | |||
| <enum value="0x8D40" name="GL_FRAMEBUFFER_OES" group="FramebufferTarget"/> | |||
| <enum value="0x8D41" name="GL_RENDERBUFFER" group="ObjectIdentifier,RenderbufferTarget,CopyImageSubDataTarget"/> | |||
| <enum value="0x8D41" name="GL_RENDERBUFFER" group="ObjectIdentifier,RenderbufferTarget,CopyImageSubDataTarget,TextureTarget"/> | |||
There was a problem hiding this comment.
I'm not sure on this change, as currently TextureTarget kind of unofficially means "enums that you can pass to glTexImage*" and GL_RENDERBUFFER isn't really in that group.
Generally I think we might need to create more groups than just TextureTarget as it's also used for glBindTexture which doesn't allow you to pass a proxy texture as a target.
Other examples of functions that use TextureTarget but specifies a different valid enum set include glGetInternalformat (as pointed out in this PR) and glGetTexLevelParameter.
There was a problem hiding this comment.
I'm fine with leaving it like this for this PR (I think), but I think we need to start a discussion on whether we should create more groups for this.
There was a problem hiding this comment.
Agreed. Creating more groups seems to be the best long-term solution. However, glGetInternalformat, glGetTexLevelParameter, and glBindTexture seem all to use different sets of valid values for the target parameters. This means potentially three new groups. In addition, the valid target values of the different functions depend on the API version.
Instead of doing this complex refactoring, I went for the simpler route -- keep the group that glGetInternalformat is already using (TextureTarget) and add GL_RENDERBUFFER to it.
There was a problem hiding this comment.
I think the "depending on opengl version" enums is something that should be figured out from the <feature> tags.
Having the enums groups be the set of all possibly valid enums that can then be filtered out by version and other factors like extension is going to be the most useful form of these enum groups.
There was a problem hiding this comment.
@NogginBops are you okay with this PR? I'm not sure if you're requesting a change, or just pointing out gl.xml is not perfect yet.
There was a problem hiding this comment.
As I think the groups discussion will result in us looking at these groups more thoroughly again (and hopefully fix them), I think there is not much harm in merging this. So yes, I think this can be merged as is.
joakim-arm
force-pushed
the
glxml-fixes
branch
from
6ee5bfe to
acc094d
Compare
|
@oddhack this is approved to merge. |
Some minor fixes in gl.xml:
Add
GL_RENDERBUFFERto the TextureTarget groupThe parameter
targetof theglGetInternalformati*functions belongs to theTextureTargetgroup. SinceGL_RENDERBUFFERis a valid value fortargetin those functions, it needs to be added to theTextureTargetgroup.Define groups for some enums
GL_FETCH_PER_SAMPLE_ARM(EnableCap,GetPName)GL_FRAGMENT_SHADER_FRAMEBUFFER_FETCH_MRT_ARM(GetPName)Change group of parameter
formatinglCompressed*TexSubImage*functionsThe group of
formatin theglCompressed*TexSubImage*functions was set toPixelFormat. However, the correct group seems to beInternalFormat, which is containing the valid values for that parameter.