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

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

Merged
merged 28 commits into from
Sep 29, 2022

Conversation

SunSerega
Copy link
Contributor

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.

"CheckFramebufferStatusTarget" was removed from <param>, but then re-added as new group "FramebufferTarget" with same enums
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
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
Same enums, but:
ColorMaterialFace not used at all
CullFaceMode and StencilFaceDirection are barely used anywhere

cleanup after KhronosGroup#355
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
This group does not exist
@NogginBops
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

Sounds like a good idea to me.

Copy link
Contributor

@Perksey Perksey left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

SunSerega commented Jun 25, 2022

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
Copy link
Contributor

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

@Perksey
Copy link
Contributor

Perksey commented Jul 20, 2022

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
Copy link
Contributor

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
Copy link
Contributor Author

SunSerega commented Jul 20, 2022

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
Copy link
Contributor

Perksey commented Jul 20, 2022

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

oddhack commented Jul 22, 2022

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
Copy link
Contributor Author

SunSerega commented Jul 22, 2022

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
Copy link
Contributor

oddhack commented Jul 22, 2022

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
Copy link
Contributor

Perksey commented Jul 22, 2022

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
Copy link
Contributor

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
Copy link
Contributor Author

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 Outdated Show resolved Hide resolved
@pdaniell-nv
Copy link
Contributor

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
Copy link
Contributor

I have no complaints for this PR 🙂

@Perksey
Copy link
Contributor

Perksey commented Aug 17, 2022

Approved for groups.

@pdaniell-nv
Copy link
Contributor

@oddhack this is approved to merge.

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.

[xml] Unused groups
6 participants