-
Notifications
You must be signed in to change notification settings - Fork 277
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
working on some missing enum groups #431
base: main
Are you sure you want to change the base?
Conversation
LGTM so far. |
mostly for various gets, but VertexAttribEnumNV was used on a bunch of other functions, so added groups for those too VertexAttribEnum,VertexAttribPropertyARB -> GetVertexAttribPName VertexAttribPointerPropertyARB -> GetVertexAttribPointerPName VertexArrayPNameAPPLE -> VertexArrayParamAPPLE and add new VertexArrayPNameAPPLE for actual pname arg VertexArrayPName -> GetVertexArrayIndexedPName, GetVertexArrayIndexed64PName, GetVertexArrayPName, GetVertexArrayIntegeriPNameEXT, GetVertexArrayIntegerPNameEXT, GetVertexArrayPointeriPNameEXT, GetVertexArrayPointervPNameEXT VertexAttribEnumNV -> ProgramTargetNV, LoadProgramTargetNV, ProgramAttribPNameNV, ProgramParameterPNameNV, VertexProgramAttribPointerPNameNV, ProgramPNameNV, ProgramStringPNameNV, VertexProgramGetTrackMatrixPNameNV, VertexProgramTrackMatrixMatrixNV
Spec lists same set of args for all functions (query table lists preferred Get for each, but doesn't seem to restrict use of others, even if they might not be useful)
OK, possibly done with edits for now if people want to look at it more closely. Leaving PR as draft for now until I finish my local testing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a few things I'm uncertain about in here.
The TL;DR of it is that gl.xml is used by a lot of libraries, amassing a large amount of downstream consumers. All it'd take is an update in which their bindings are regenerated and they've potentially broken lots of projects.
For this reason, generally defer group removals and renames.
Other than that, this PR is great and thank you for being among the few improving the groupings! <3
xml/gl.xml
Outdated
@@ -450,7 +450,7 @@ typedef unsigned int GLhandleARB; | |||
<enum name="GL_CLAMP_READ_COLOR"/> | |||
</group> | |||
|
|||
<group name="VertexArrayPNameAPPLE" comment="Deprecated, use the group attributes instead."> | |||
<group name="VertexArrayParamAPPLE" comment="Deprecated, use the group attributes instead."> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes shouldn't be made to the old groups (you've renamed it here) as they are pending removal.
xml/gl.xml
Outdated
@@ -3873,17 +3873,14 @@ typedef unsigned int GLhandleARB; | |||
<enum name="GL_UNIFORM_ATOMIC_COUNTER_BUFFER_INDEX"/> | |||
</group> | |||
|
|||
<group name="SamplerParameterI" comment="Deprecated, use the group attributes instead."> | |||
<group name="SamplerParameterPName" comment="Deprecated, use the group attributes instead."> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
@@ -5638,20 +5635,20 @@ typedef unsigned int GLhandleARB; | |||
<enum value="0x1001" name="GL_TEXTURE_HEIGHT" group="TextureParameterName,GetTextureParameter"/> | |||
<enum value="0x1003" name="GL_TEXTURE_INTERNAL_FORMAT" group="TextureParameterName,GetTextureParameter"/> | |||
<enum value="0x1003" name="GL_TEXTURE_COMPONENTS" group="TextureParameterName,GetTextureParameter"/> | |||
<enum value="0x1004" name="GL_TEXTURE_BORDER_COLOR" group="SamplerParameterF,GetTextureParameter,TextureParameterName"/> | |||
<enum value="0x1004" name="GL_TEXTURE_BORDER_COLOR_EXT"/> | |||
<enum value="0x1004" name="GL_TEXTURE_BORDER_COLOR" group="SamplerParameterPName,GetTextureParameter,TextureParameterName"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get that the SamplerParameterF
name is inconsistent with literally every other group name here, however renaming this would technically break downstream consumers so I think this should be reverted.
I tend to defer breaking group changes unless the change poses a significant improvement for downstream consumers, such as if two groups were valid for the same parameter and are being merged, or a more generalized group is being added allowing use on more functions.
Seeing as you haven't annotated more functions with this group attribute and I don't think there are any other improvements other than making this name conform with the nomenclature used everywhere else, I don't think this is enough to justify the breaking changes in downstream libraries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main problem is that the grouping in the spec is divided into SamplerParameterI
and SamplerParameterF
. If we don't want to remove old groups, we would need to add enums from each to the other, then always remember to add them both. Not sure which is a better solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's an ugly situation we've gotta live with unfortunately due to how high-profile these groups are.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Somehow I missed 2 paragraphs of your first response, so my answer wasn't very clear. In this case, there are 2 groups (SamplerParameterI
and SamplerParameterF
) that are both (more or less) valid for every use of either group, so I merged them into one SamplerParameterPName
. Don't have much objection to keeping both as duplicates though, if you think that would be better, aside from the part where I have to go back to the spec to check that "more or less" to see if some uses are "valid but undefined" or whatever that might make them worth removing from one group.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
both (more or less) valid for every use of either group
In that case this should be fine, though I'd like to keep the old group names in addition to the new one.
@@ -33088,7 +33085,7 @@ typedef unsigned int GLhandleARB; | |||
<proto>void <name>glVertexAttribPointerNV</name></proto> | |||
<param><ptype>GLuint</ptype> <name>index</name></param> | |||
<param><ptype>GLint</ptype> <name>fsize</name></param> | |||
<param group="VertexAttribEnumNV"><ptype>GLenum</ptype> <name>type</name></param> | |||
<param group=""><ptype>GLenum</ptype> <name>type</name></param> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You've left this group attribute empty. Consider readding VertexAttribEnumNV?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, will check and see what that should be (VertexAttribEnumNV
was almost entirely incorrect if i remember right, which makes it a better candidate for complete replacement. Only had 1 enum, which isn't accepted by most of the parameters labelled with that group)
xml/gl.xml
Outdated
<enum value="0x2800" name="GL_TEXTURE_MAG_FILTER" group="SamplerParameterPName,GetTextureParameter,TextureParameterName"/> | ||
<enum value="0x2801" name="GL_TEXTURE_MIN_FILTER" group="SamplerParameterPName,GetTextureParameter,TextureParameterName"/> | ||
<enum value="0x2802" name="GL_TEXTURE_WRAP_S" group="SamplerParameterPName,GetTextureParameter,TextureParameterName"/> | ||
<enum value="0x2803" name="GL_TEXTURE_WRAP_T" group="SamplerParameterPName,GetTextureParameter,TextureParameterName"/> | ||
<unused start="0x2804" end="0x28FF" comment="Unused for TextureParameterName"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above (won't comment on any other occurances of this herein)
xml/gl.xml
Outdated
<enum value="0x8644" name="GL_PROGRAM_PARAMETER_NV" group="VertexAttribEnumNV"/> | ||
<enum value="0x8645" name="GL_ATTRIB_ARRAY_POINTER_NV"/> | ||
<enum value="0x8645" name="GL_VERTEX_ATTRIB_ARRAY_POINTER" group="VertexAttribPointerPropertyARB"/> | ||
<enum value="0x8645" name="GL_VERTEX_ATTRIB_ARRAY_POINTER_ARB" group="VertexAttribPointerPropertyARB"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's happened to these three groups?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
VertexAttribEnumNV
as mentioned above was pretty much useless and got replaced with a bunch of correct enums.
VertexAttribPointerPropertyARB
looks like it might have just been renamed, can change it back if that's a problem.
I'll look at trying to minimize changes that would affect existing groups, and keep existing names where possible, Some will still end up split and some parameter groups changed, though, unless we want to just keep piling "wrong" enums into random groups because they were once applied to the wrong function. |
@Perksey I merged was: (some names reformatted oddly since i queried them from my bindings)
new:
|
Looks like the only correct use of |
Removed changes to deprecated group tags, and renamed some groups to old names where they still matched reasonably well.
Let me know if you think it is worth duplicating |
@@ -5638,20 +5635,20 @@ typedef unsigned int GLhandleARB; | |||
<enum value="0x1001" name="GL_TEXTURE_HEIGHT" group="TextureParameterName,GetTextureParameter"/> | |||
<enum value="0x1003" name="GL_TEXTURE_INTERNAL_FORMAT" group="TextureParameterName,GetTextureParameter"/> | |||
<enum value="0x1003" name="GL_TEXTURE_COMPONENTS" group="TextureParameterName,GetTextureParameter"/> | |||
<enum value="0x1004" name="GL_TEXTURE_BORDER_COLOR" group="SamplerParameterF,GetTextureParameter,TextureParameterName"/> | |||
<enum value="0x1004" name="GL_TEXTURE_BORDER_COLOR_EXT"/> | |||
<enum value="0x1004" name="GL_TEXTURE_BORDER_COLOR" group="SamplerParameterPName,GetTextureParameter,TextureParameterName"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
both (more or less) valid for every use of either group
In that case this should be fine, though I'd like to keep the old group names in addition to the new one.
<enum value="0x2800" name="GL_TEXTURE_MAG_FILTER" group="SamplerParameterPName,GetTextureParameter,TextureParameterName"/> | ||
<enum value="0x2801" name="GL_TEXTURE_MIN_FILTER" group="SamplerParameterPName,GetTextureParameter,TextureParameterName"/> | ||
<enum value="0x2802" name="GL_TEXTURE_WRAP_S" group="SamplerParameterPName,GetTextureParameter,TextureParameterName"/> | ||
<enum value="0x2803" name="GL_TEXTURE_WRAP_T" group="SamplerParameterPName,GetTextureParameter,TextureParameterName"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed above,
<enum value="0x2800" name="GL_TEXTURE_MAG_FILTER" group="SamplerParameterPName,GetTextureParameter,TextureParameterName"/> | |
<enum value="0x2801" name="GL_TEXTURE_MIN_FILTER" group="SamplerParameterPName,GetTextureParameter,TextureParameterName"/> | |
<enum value="0x2802" name="GL_TEXTURE_WRAP_S" group="SamplerParameterPName,GetTextureParameter,TextureParameterName"/> | |
<enum value="0x2803" name="GL_TEXTURE_WRAP_T" group="SamplerParameterPName,GetTextureParameter,TextureParameterName"/> | |
<enum value="0x2800" name="GL_TEXTURE_MAG_FILTER" group="SamplerParameterPName,SamplerParameterI,GetTextureParameter,TextureParameterName"/> | |
<enum value="0x2801" name="GL_TEXTURE_MIN_FILTER" group="SamplerParameterPName,SamplerParameterI,GetTextureParameter,TextureParameterName"/> | |
<enum value="0x2802" name="GL_TEXTURE_WRAP_S" group="SamplerParameterPName,SamplerParameterI,GetTextureParameter,TextureParameterName"/> | |
<enum value="0x2803" name="GL_TEXTURE_WRAP_T" group="SamplerParameterPName,SamplerParameterI,GetTextureParameter,TextureParameterName"/> |
Won't comment on all instances of this.
<enum value="0x82D4" name="GL_VERTEX_ATTRIB_BINDING" group="GetVertexAttribPName"/> | ||
<enum value="0x82D5" name="GL_VERTEX_ATTRIB_RELATIVE_OFFSET" group="GetVertexAttribPName,VertexArrayPName"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should keep the old groups so that this change isn't too breaking.
<enum value="0x85BE" name="GL_STORAGE_CACHED_APPLE" group="VertexArrayPNameAPPLE"/> | ||
<enum value="0x85BF" name="GL_STORAGE_SHARED_APPLE" group="VertexArrayPNameAPPLE"/> | ||
<enum value="0x85BE" name="GL_STORAGE_CACHED_APPLE" group="VertexArrayParamAPPLE"/> | ||
<enum value="0x85BF" name="GL_STORAGE_SHARED_APPLE" group="VertexArrayParamAPPLE"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probably change the name of this group back, for the reason that it's only to keep consistency and has no other meaningful change and will break downstream consumers.
That change sounds fine, though if we are making breaking changes it's generally best to leave the old groups around so that all that's required is a simple implicit or explicit cast for users of the groups. |
@pdaniell-nv contains NVIDIA vendor extension grouping changes, comments would be appreciated, |
OK, will add the old group names back in addition to the merged ones, and rename the apple one. any suggestions for the name of the enum that actually gets passed to the What do you think about adding in a section for group aliases, so we can eventually drop the extra unused groups and just replace them with something like |
I skimmed through and didn't see anything objectionable. We don't use the enum groups so if they work for consumers of those properties I'm happy. |
Awesome, yeah just figured I'd ping you in here to make sure that none of the values being marked as applicable to the extension functions are bogus :) |
We would love to see this PR getting merged at opentk. Anything you need to be done that we can assist with? |
@frederikja163 : reports that at least some of it is actively useful, and that the rest doesn't break things for existing code is helpful. I'll push this back up towards the top of my todo list, but still a few things ahead of it so won't be able to get back to it immediately. If you have time before then, feel free to take care of some or all of the requested changes (I think it was mostly restoring old group names?). |
Not done yet, but could use more eyes (and runs through other binding generators) to see if there are any obvious mistakes so far.
Adding in anything missing that has been found by cl-opengl users, along with whatever I can find missing by scraping the cl-opengl wrappers (and also just went through the 4.6 compat spec and added most of the
glGet*
andIsEnabled
since that was a big chunk of what was missing for wrappers).