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 groups, which are supposed to differ only in vendor id's #543

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

SunSerega
Copy link
Contributor

I agree with this, adding vendor suffixes to group names was a bad idea.
But this PR doesn't remove them all, because the suffix can be trivially removed on a per-binding basis, most of the time. Either because there is no group without a suffix, or because it has the same set of enum values.
I can also make a PR that removes all vendor suffixes while retaining all info of XML, but that will probably break too many things downstream.

However, there are a few cases like ConvolutionTarget/ConvolutionTargetEXT where both groups are meant to be one. Still, the addition of enum values to them was inconsistent. And so now they can't be merged trivially.
In these cases, I merged them back into one group. And added a check to my binding generators' log, so I will notice if such behavior arises again.

I was working on this in and out before #520 got merged, and there is one more case I'd like to look into, a bit more confusing than the rest. But also I'd love some feedback. So draft for now.

Were only in SGI:
TEXTURE_COLOR_TABLE
PROXY_TEXTURE_COLOR_TABLE
Remove collision with VertexArrayPName
https://registry.khronos.org/OpenGL/extensions/PGI/PGI_misc_hints.txt
Targets not explicitly accepted by glHintPGI:
	PERSPECTIVE_CORRECTION_HINT = C50
	POINT_SMOOTH_HINT = C51
	LINE_SMOOTH_HINT = C52
	POLYGON_SMOOTH_HINT = C53
	FOG_HINT = C54
	PACK_CMYK_HINT = 800E
	UNPACK_CMYK_HINT = 800F
	PHONG_HINT = 80EB
	CLIP_VOLUME_CLIPPING_HINT = 80F0
	TEXTURE_MULTI_BUFFER_HINT = 812E
	GENERATE_MIPMAP_HINT = 8192
	PROGRAM_BINARY_RETRIEVABLE_HINT = 8257
	CONVOLUTION_HINT = 8316
	SCALEBIAS_HINT = 8322
	LINE_QUALITY_HINT = 835B
	VERTEX_PRECLIP = 83EE
	VERTEX_PRECLIP_HINT = 83EF
	TEXTURE_COMPRESSION_HINT = 84EF
	VERTEX_ARRAY_STORAGE_HINT = 851F
	MULTISAMPLE_FILTER_HINT = 8534
	TRANSFORM_HINT = 85B1
	TEXTURE_STORAGE_HINT = 85BC
	FRAGMENT_SHADER_DERIVATIVE_HINT = 8B8B
	BINNING_CONTROL_HINT = 8FB0

Also note: glHintPGI's <mode> parameter takes values other then "VertexHintsMaskPGI". That group was created for:
https://registry.khronos.org/OpenGL/extensions/PGI/PGI_vertex_hints.txt
@Perksey
Copy link
Contributor

Perksey commented Sep 29, 2022

Tbf I think it's probably time we take a serious look at the groups (per #481) and try and establish a sane baseline i.e. let's remove the suffixes where we think they're no longer relevant, as well as fix as many more issues as possible so we can get to a point where we're happy with what we have and move forward. e.g. BufferTargetARB probably doesn't need the ARB suffix anymore.

xml/gl.xml Outdated
@@ -26900,7 +26900,7 @@ typedef unsigned int GLhandleARB;
</command>
<command>
<proto>void <name>glVertexArrayParameteriAPPLE</name></proto>
<param group="VertexArrayPNameAPPLE"><ptype>GLenum</ptype> <name>pname</name></param>
<param group="VertexArrayParameterNameAPPLE"><ptype>GLenum</ptype> <name>pname</name></param>
Copy link
Contributor

Choose a reason for hiding this comment

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

iirc most groups use PName instead of ParameterName. I think changing this should be a separate issue/PR. And the APPLE suffix isn't removed either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I didn't remove vendor suffixes when possible in this PR. However, it might be better to remove them, depending on the discussion in #481.

The reason for renaming is that VertexArrayPName is already a group. And a separate one, with its own uses and enum values.
And tbh I'm sure there was a reason I expanded P into Parameter, I think something to do with functions in which this group is used. But I can't find it now.
Well, I'm all ears for naming suggestions, as always, because naming sense is not my forte.

Regardless, this PR is exactly about resolving such collisions between groups, be it by merging or splitting by renaming.
Of course, every group is its own case, but I think making a swarm of PR is a bad idea.
That said, I can make 1 PR for all the group merges and 1 PR for all splits by renaming. Do you think that would be better?

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.

3 participants