[xml] Fixes for defined but unused enum groups #520

SunSerega · 2022-06-24T14:45:21Z

fix #360
I mainly stalled in fixing it because of groups like TextureMinFilter: It can be passed to, for instance, glTextureParameteri, but there is currently no way to denote that in XML - because the same parameter can also take TextureMagFilter, TextureWrapMode, etc., depending on the <pname>.
But since that issue, I updated my data scrappers and generators, so it became easier to separate these cases from actual misdefined groups.

I thought of making a bunch of small PRs, but I really don't like how unreadable that looks on git commit graphs.
So instead, I separated fixes by one or a few related groups. But I can still break this into many PRs if that would be easier to review.

https://github.com/KhronosGroup/OpenGL-Registry/blob/237f076633e67d7396cbed15f112e515a7d85251/extensions/QCOM/QCOM_shader_framebuffer_fetch_noncoherent.txt#L61

Renamed in <param> but not definition KhronosGroup@db455e3#diff-e264a54745d72adc78ba865d49dfe4a83689d6307ed6ea03133096d211dc03c6L14360

"CheckFramebufferStatusTarget" was removed from <param>, but then re-added as new group "FramebufferTarget" with same enums

A single use for a group of single enum value: https://github.com/KhronosGroup/OpenGL-Registry/blob/91aa993da3cc9e7ea1eecf3075de8c03d9a15d09/extensions/ARB/ARB_shader_objects.txt#L1768

https://www.khronos.org/registry/OpenGL-Refpages/gl2.1/xhtml/glTexEnv.xml Not all functions with TextureEnvParameter accept all enums from this group But this change is mostly to show that these enums only belong to deprecated functions and can be ignored

KhronosGroup#400 "VertexBufferObjectParameter" was removed from <param>, replaced by "BufferPNameARB" But not from group definitions

https://github.com/KhronosGroup/OpenGL-Registry/blob/main/extensions/SGIS/SGIS_texture_filter4.txt GL_FILTER4_SGIS is only used in glTexParameter* and gl[Get]TexFilterFuncSGIS

These groups have same enums And only *SGI variants were ever used

KhronosGroup#401 "GlslTypeToken" wasn't removed after removing it's uses Though "GL_UNSIGNED_INT_ATOMIC_COUNTER" is now groupless... But I guess it can be neither attribute nor uniform, so this define is useless.

Same as PixelMap, but never used

https://github.com/KhronosGroup/OpenGL-Registry/blob/main/extensions/ARB/ARB_transform_feedback3.txt#L693

Same enums, but: ColorMaterialFace not used at all CullFaceMode and StencilFaceDirection are barely used anywhere cleanup after KhronosGroup#355

https://github.com/KhronosGroup/OpenGL-Registry/blob/91aa993da3cc9e7ea1eecf3075de8c03d9a15d09/extensions/SGIX/SGIX_pixel_texture.txt#L130-L131 https://github.com/KhronosGroup/OpenGL-Registry/blob/91aa993da3cc9e7ea1eecf3075de8c03d9a15d09/extensions/SGIX/SGIX_impact_pixel_texture.txt#L41-L47 Also see: KhronosGroup#519

ReplacementCodeSUN isn't defined Instead TriangleListSUN is expected Also glReplacementCodeuivSUN was missing group But I'm not adding it to glReplacementCodeusSUN and such, because they have different enum size

https://github.com/KhronosGroup/OpenGL-Registry/blob/91aa993da3cc9e7ea1eecf3075de8c03d9a15d09/extensions/INTEL/INTEL_map_texture.txt#L61-L63

This group does not exist

NogginBops · 2022-06-24T22:27:22Z

Not entirely sure why MaterialFace got preferred over something like FaceCullMode. glCullFace taking a MaterialFace instead of FaceCullMode doesn't make much sense even though the group contains the same enums

SunSerega · 2022-06-25T04:39:42Z

why MaterialFace got preferred

Because it was used in almost all cases, where these enums are accepted. This way it's a minimal change.
Well, If that's not a good reason, how about renaming it to something like TriangleFace?

NogginBops · 2022-06-25T08:25:57Z

Sounds like a good idea to me.

Perksey

Some of these groups are unused because we haven't decided on #481 and as a result haven't touched the functions that could use them.

I also don't think we should remove groups purely because they're not declared for usage on a function parameter, they may be used for other reasons: analysers (I know ARM have a tool which consumes the groups used to validate OpenGLES calls and state), storage/fields, etc.

By removing groups we are actively removing information from this specification, and I'm not sure them looking unused on the surface is a good excuse for that.

SunSerega · 2022-06-25T15:58:45Z

I've only completely removed 5 groups. Look at => X in commits and at descriptions of them:

GetPixelMap => X

Same as PixelMap, but never used

That is because I've looked at all the enums of these groups and found where they are used, mainly adding the group attribute to <param> or merging them with other groups.
And then there is also the TextureMinFilter case I described in the very beginning - these kinds of groups were left untouched, even though they are still not used anywhere in XML.
So, at least theoretically, no correct information should be lost in these changes. But I would be glad if anyone could give a fresh look at what exactly is changed.

pdaniell-nv · 2022-07-19T18:21:14Z

Are we still waiting for approval from @null77 and @joakim-arm?

Perksey · 2022-07-20T07:33:42Z

Ideally yes. We can continue without their approval, but ultimately this will likely impact them as group users and given they represent Khronos members/projects, I feel like we should solicit their opinion first!

If we haven't heard back in a little bit (say 2 weeks) then I think we should merge.

joakim-arm · 2022-07-20T17:40:53Z

Looks good, but just one comment:

Removing the group TraceMaskMESA from the bitmask values makes things a bit inconsistent. All other bitmask values are assigned to a group.

@SunSerega, did you add this group back?

SunSerega · 2022-07-20T18:21:52Z

No, it has an unresolved conversation here.
I encourage other reviewers to also look at unresolved conversations. They are what other people found non-trivial and what I left unresolved because I would appreciate more feedback.

I personally think these enums should be removed from XML because there is no documentation/functions in this repo, corresponding to them.
Seeing how smoothly #523 went, I guess this kind of change isn't seen as breaking, so I'll go ahead and comment these out.

Perksey · 2022-07-20T19:43:14Z

I personally think these enums should be removed from XML because there is no documentation/functions in this repo, corresponding to them.
Seeing how smoothly #523 went, I guess this kind of change isn't seen as breaking, so I'll go ahead and comment these out.

I don't agree with this, mainly because:

Possibly fun historical note: early in GL history, when it was still all workstation manufacturers, one of the CAD vendors came up with a "secret" extension. They convinced multiple GL implementers to support it in their drivers, but not to expose it in the GL_EXTENSIONS string. We speculated the purpose was to give them some sort of competitive edge over other CAD vendors, while keeping the driver suppliers from realizing what was going on.

oddhack

We shouldn't actively remove information from the registry just because it doesn't look to be used. We have 30 years of history to cover here, can you really say that this information wasn't relevant for the entirety of those 30 years? Moreover, it's unknowable what dependency someone will have taken on any part of the registry, so I don't think it's really great to make these sorts of breaks lightly. If a Khronos member approves the removal of this, given groups aren't WG-controlled we can remove it but only if there is reasonable cause & consensus.

joakim-arm · 2022-07-21T10:17:37Z

Looks good, but just one comment:

I just wanted to clarify that this change doesn't break anything for our internal tool. However, I am afraid I don't have enough background and resources to comment on other possible implications that this change could introduce.

What Perksey says about covering history makes sense to me.

NogginBops · 2022-07-21T21:10:06Z

I'm fine with these changes too.
They are going to cause a lot of breaking changes in bindings, but as we are preparing for a new major version, I personally have no objection.

oddhack · 2022-07-22T02:09:07Z

I agree with @Perksey - completely removing enums from the registry affects the Historical Record. If an enum isn't being used by an extension but was still reserved that's useful to document, and nobody needs to be actually processing it if it isn't tagged as part of an extension or core API.

SunSerega · 2022-07-22T06:17:47Z

Ok, then how about defining a special group/attribute for such enums, to show they only exist as a legacy?

Note enums in question aren't even reserved in this case, because they are bitmasks.

oddhack · 2022-07-22T10:26:21Z

Ok, then how about defining a special group/attribute for such enums, to show they only exist as a legacy?

Note enums in question aren't even reserved in this case, because they are bitmasks.

We don't have any historical preservation need to add new information about these enums. I'm not sure why it even comes up for anyone. If there's no dependency path from an extension requiring these values, they should be ignored by whatever is consuming the XML. If the people concerned with the groups collectively agree there's some value to adding it, though, feel free.

Perksey · 2022-07-22T11:21:34Z

I don't agree that adding an attribute for these is worth it. It's more maintenance, more work to figure out how to represent such information, and even then there will be exactly one person who will be using such info as it currently stands. Moreover, drawing a "legacy" line is a hard thing to do. As I said in my previous comment, all of this information was relevant at one point, so there's no real reason to do anything other than keep it in the registry. If downstream consumers don't want to care about these, they can filter them out at their own accord.

NogginBops · 2022-07-22T20:08:11Z

I would also be careful with removing enums, and I don't think there is a need for a special group to mark them.
Though I would be open to hear why such a group might be useful if you (@SunSerega) want to argue for it.

SunSerega · 2022-07-23T12:37:42Z

I think this has grown a bit out of the scope of this pull, so I'll remove changes to TraceMaskMESA and make a separate issue after sorting my thoughts a bit more.

xml/gl.xml

pdaniell-nv · 2022-08-16T19:59:16Z

As far as I can tell, this PR is complete and awaiting sign-off from the OpenGL/ES working group. We'll approve this tomorrow. Please let me know if anyone has objections to this getting merged, otherwise it will likely be approved for merge tomorrow.

NogginBops · 2022-08-16T20:23:05Z

I have no complaints for this PR 🙂

Perksey · 2022-08-17T09:26:48Z

Approved for groups.

pdaniell-nv · 2022-08-17T17:26:24Z

@oddhack this is approved to merge.

SunSerega added 22 commits June 23, 2022 14:14

FramebufferFetchNoncoherent => GetPName,EnableCap

1f0cad3

https://github.com/KhronosGroup/OpenGL-Registry/blob/237f076633e67d7396cbed15f112e515a7d85251/extensions/QCOM/QCOM_shader_framebuffer_fetch_noncoherent.txt#L61

DrawBufferModeATI => DrawBufferMode

b83d93f

Renamed in <param> but not definition KhronosGroup@db455e3#diff-e264a54745d72adc78ba865d49dfe4a83689d6307ed6ea03133096d211dc03c6L14360

CheckFramebufferStatusTarget => FramebufferTarget

a48970b

"CheckFramebufferStatusTarget" was removed from <param>, but then re-added as new group "FramebufferTarget" with same enums

ContainerType: add to <param>

dde9df9

A single use for a group of single enum value: https://github.com/KhronosGroup/OpenGL-Registry/blob/91aa993da3cc9e7ea1eecf3075de8c03d9a15d09/extensions/ARB/ARB_shader_objects.txt#L1768

VertexBufferObjectParameter => X

7c1dee6

KhronosGroup#400 "VertexBufferObjectParameter" was removed from <param>, replaced by "BufferPNameARB" But not from group definitions

TextureFilterFuncSGIS => X

333afe5

https://github.com/KhronosGroup/OpenGL-Registry/blob/main/extensions/SGIS/SGIS_texture_filter4.txt GL_FILTER4_SGIS is only used in glTexParameter* and gl[Get]TexFilterFuncSGIS

[Get]ColorTableParameterPName[SGI] => ColorTableParameterPName

66403ab

These groups have same enums And only *SGI variants were ever used

[Get]ConvolutionParameter[EXT] => ConvolutionParameter

58d235e

GlslTypeToken => X

1404006

KhronosGroup#401 "GlslTypeToken" wasn't removed after removing it's uses Though "GL_UNSIGNED_INT_ATOMIC_COUNTER" is now groupless... But I guess it can be neither attribute nor uniform, so this define is useless.

LightEnvModeSGIX: add to <param>

6758ca0

PointParameterNameSGIS =+> PointParameterNameARB,GetPName

41d7bb9

GetPixelMap => X

e0b3356

Same as PixelMap, but never used

TransformFeedbackTokenNV: add to <param>

03a9fe6

https://github.com/KhronosGroup/OpenGL-Registry/blob/main/extensions/ARB/ARB_transform_feedback3.txt#L693

ColorMaterialFace,CullFaceMode,StencilFaceDirection => MaterialFace

55d4565

Same enums, but: ColorMaterialFace not used at all CullFaceMode and StencilFaceDirection are barely used anywhere cleanup after KhronosGroup#355

ReplacementCodeSUN => TriangleListSUN

fbf1a31

ReplacementCodeSUN isn't defined Instead TriangleListSUN is expected Also glReplacementCodeuivSUN was missing group But I'm not adding it to glReplacementCodeusSUN and such, because they have different enum size

TextureParameterName: add GL_TEXTURE_MEMORY_LAYOUT_INTEL

68a466f

https://github.com/KhronosGroup/OpenGL-Registry/blob/91aa993da3cc9e7ea1eecf3075de8c03d9a15d09/extensions/INTEL/INTEL_map_texture.txt#L61-L63

CommandOpcodesNV: add to <param>

0314c60

PerformanceQueryCapsMaskINTEL: add to <param>

58cbb9b

TraceMaskMESA => X

b0340e2

This group does not exist

VertexHintsMaskPGI: add to <param>

78931c3

SunSerega mentioned this pull request Jun 24, 2022

[xml] Group fixes for GL_ATI_fragment_shader #521

Merged

MaterialFace => TriangleFace

2015eb4

Perksey suggested changes Jun 25, 2022

View reviewed changes

SunSerega mentioned this pull request Jun 25, 2022

GL_PIXEL_TEX_GEN_ALPHA_REPLACE_SGIX #519

Closed

Re-add TraceMaskMESA

cd4145d

SunSerega force-pushed the unused-groups branch from a130319 to cd4145d Compare July 23, 2022 12:38

Perksey reviewed Jul 23, 2022

View reviewed changes

xml/gl.xml Outdated Show resolved Hide resolved

SunSerega added 2 commits July 23, 2022 15:53

Merge branch main into unused-groups

644efba

Remove tabs and resurrected PixelTexGenMode

bb04531

Perksey approved these changes Jul 23, 2022

View reviewed changes

SunSerega mentioned this pull request Aug 9, 2022

Rename group attribute for non GLenum parameters not corresponding to any enum group #534

Merged

Merge branch 'main' into unused-groups

50b8306

pdaniell-nv added this to the Approved to Merge milestone Aug 17, 2022

pdaniell-nv assigned oddhack Aug 17, 2022

oddhack merged commit 5bae873 into KhronosGroup:main Sep 29, 2022

SunSerega deleted the unused-groups branch September 29, 2022 11:16

SunSerega mentioned this pull request Sep 29, 2022

[XML] Fixes for groups, which are supposed to differ only in vendor id's #543

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[xml] Fixes for defined but unused enum groups #520

[xml] Fixes for defined but unused enum groups #520

SunSerega commented Jun 24, 2022

NogginBops commented Jun 24, 2022

SunSerega commented Jun 25, 2022

NogginBops commented Jun 25, 2022

Perksey left a comment

SunSerega commented Jun 25, 2022 •

edited

Loading

GetPixelMap => X

pdaniell-nv commented Jul 19, 2022

Perksey commented Jul 20, 2022

joakim-arm commented Jul 20, 2022

SunSerega commented Jul 20, 2022 •

edited

Loading

Perksey commented Jul 20, 2022

joakim-arm commented Jul 21, 2022

NogginBops commented Jul 21, 2022

oddhack commented Jul 22, 2022

SunSerega commented Jul 22, 2022 •

edited

Loading

oddhack commented Jul 22, 2022

Perksey commented Jul 22, 2022

NogginBops commented Jul 22, 2022

SunSerega commented Jul 23, 2022

pdaniell-nv commented Aug 16, 2022

NogginBops commented Aug 16, 2022

Perksey commented Aug 17, 2022

pdaniell-nv commented Aug 17, 2022

[xml] Fixes for defined but unused enum groups #520

[xml] Fixes for defined but unused enum groups #520

Conversation

SunSerega commented Jun 24, 2022

NogginBops commented Jun 24, 2022

SunSerega commented Jun 25, 2022

NogginBops commented Jun 25, 2022

Perksey left a comment

Choose a reason for hiding this comment

SunSerega commented Jun 25, 2022 • edited Loading

GetPixelMap => X

pdaniell-nv commented Jul 19, 2022

Perksey commented Jul 20, 2022

joakim-arm commented Jul 20, 2022

SunSerega commented Jul 20, 2022 • edited Loading

Perksey commented Jul 20, 2022

joakim-arm commented Jul 21, 2022

NogginBops commented Jul 21, 2022

oddhack commented Jul 22, 2022

SunSerega commented Jul 22, 2022 • edited Loading

oddhack commented Jul 22, 2022

Perksey commented Jul 22, 2022

NogginBops commented Jul 22, 2022

SunSerega commented Jul 23, 2022

pdaniell-nv commented Aug 16, 2022

NogginBops commented Aug 16, 2022

Perksey commented Aug 17, 2022

pdaniell-nv commented Aug 17, 2022

SunSerega commented Jun 25, 2022 •

edited

Loading

SunSerega commented Jul 20, 2022 •

edited

Loading

SunSerega commented Jul 22, 2022 •

edited

Loading