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

NogginBops · 2022-08-08T23:17:15Z

Fixes #517

I've used the word kind here to indicate that the label indicates the kind if value the parameter is (eg ClampedFloat32 or CoordF).
I was thinking about other words to use such as "attrib", "category", "variety", "set" or "label", but personally it seemed like "kind" was the best, most descriptive name.

@Perksey @SunSerega comments?

… any enum group.

Perksey

Needs to be added to registry.rnc. Otherwise looks good.

SunSerega · 2022-08-09T14:12:38Z

The changes are definitely better than keeping it as is.
But I have my nitpicks, mostly minor improvements that are better done now before the fate of kind starts to feel sealed:

(no more)

Long term (probably other PR's)

Some of CheckedFloat32 seems to be more of a "checked and clamped", rather than "checked with errors", as it sounds to me.
But it's better to leave them for the next PR because that change will be significant on its own.

kind's mentioned in the "Extension" table from [xml] Definition of object namespaces #428, like Path, FenceNV, etc. should be classes.
Well, I was planning to make classes for them myself... I can always make another pull after this one.

What was PathElement describes a more general pattern: The type of values in the array is determined by another parameter. This can also be for instance found in glColorPointer.
Similar case with the pname parameter. For instance, the glTex[,ture]Parameteri can take many different groups depending on pname. Same but simpler with SpriteModeSGIX from Enum fixes found while making #534 #546.

Kinds for *Uniformui64v* and *UniformHandleui64v*?^[.]

Completed

I don't think there is a point in having both class and kind attributes on the same parameter, like in the case of texture, framebuffer, etc.
The point of kind I can see (besides removing the wrong group uses) is to indicate some additional info, which is not necessary in any case but is sometimes good for improving bindings.
When I was adding class=texture I looked at all group=texture, but didn't delete them, because I didn't want to touch them yet.

kind=handleARB doesn't add any info, because it only exists on parameters of the type GLhandleARB.
There are a few cases like the return value of glGetImageHandleARB: GLuint64 is used instead of GLhandleARB.
If you consider them, it makes sense to use common kind or even class to include them.
On the other hand, it seems to me that everything with GLhandleARB is outdated. So, I'm not sure if there is a point in adding/keeping kind/class attributes.

Half16NV is also a copy of the parameter type. But unlike handleARB - I don't see the reason to keep it.
Same for kind=Boolean. And that one case with BooleanPointer (which, if anything, is a BooleanPointerPointer).

There are multiple of kind=Clamped..., but they all mean the same. I think combining them into something like Clamped[0; 1] is better.

Same for kind=Color... and kind=Coord.... Type is already defined on the parameter, so it's better to combine it into something like kind=Color or kind=ColorValue. (and a separate one for Coord...)

DrawBufferName is not a name, but rather an index (0..15)

BufferOffset and BufferOffsetARB, as well as BufferSize and BufferSizeARB, are the same.

PathElement describes something more general: The type of values in the array is determined by another parameter.
This pattern can be found in many other places, including core spec. And I think I saw a few other kinds with the same meaning, but can't find them now...
Regardless, it is not very useful to mark this relation using the kind attribute, because there is no info on what parameter describes type. So I think this kind should be removed, and maybe at some point replaced with a different attribute.

ProgramCharacterNV, as far as I can see is not a program-specific thing, but rather a regular char, disguised as GLubyte, just like in glGetString, so it should also be kind=string. Or maybe change both to kind=char, for consistency with other kinds?

kind=LineStipple exists only once and doesn't do anything special. Its parameter can accept any 16-bit number.

Same for SampleMaskNV.

ReplacementCodeSUN actually is a group. In [xml] Fixes for defined but unused enum groups #520 I've merged it with TriangleListSUN.

I'm not sure if cl_context, cl_event, and vdpauSurfaceNV should be kind's, class's, or neither...
They definitely have their own namespace. But also maybe they shouldn't be mixed with other classes, which are defined only in and for OpenGL.

It seems PathCommand should instead be a group PathCoordType. Though I don't have a thorough understanding of this, so maybe it's better if someone from NV confirms. @pdaniell-nv?

MaskedColorIndexValue, MaskedStencilValue, and StencilValue.

Floating-point color values (like in glBlendColor) are also clamped. kind="Clamped01,Color"?
Tbh, I expected there to be many more cases where the parameter has multiple kinds at the same time...

How about <kind name="..." description="..."/> somewhere at the top, to make them more well-defined?
Unlike classes, which are solely defined by their uses and being a namespace, each unique kind represents a new parameter usage pattern. And that pattern can't always be inferred from uses. For instance, all the Clamped..., as far as I can see, are clamped between 0 and 1. (P.S. Wrong, some are [-1; +1])
And the process of making descriptions will probably uncover more things to improve.

Non-kind_attribute changes

(this is all good, noting here just to keep track)

Added all the metadata to the functions of the extension NV_path_rendering.

And in the process, group InstancedPathCoverMode was split off from PathCoverMode.
InstancedPathCoverMode only differs by additionally having a BOUNDING_BOX_OF_BOUNDING_BOXES enum.

Removed groups:

Dup of info of parameter type:

handleARB: Dup of GLhandleARB.

Half16NV: Dup of GLhalfNV.

Boolean (and BooleanPointer): Dup of GLboolean.

cl_context: Dup of struct _cl_context.

cl_event: Dup of struct _cl_event.

vdpauSurfaceNV: Dup of GLvdpauSurfaceNV.

Dup of info of class attribute:

Texture: Dup of texture.

List: Dup of display list.

sync: Dup of sync.

Framebuffer: Dup of framebuffer.

Renderbuffer: Dup of renderbuffer

PathElement: Not useful as it was, to be reworked.

LineStipple and SampleMaskNV: Didn't mean anything.

glClearNamedBufferSubData: Added len="COMPSIZE(format,type)".

PathCommand renamed to PathCoordType.

oddhack · 2022-08-09T14:16:58Z

Also needs to be added to xml/readme.tex, the schema description document.

pdaniell-nv · 2022-08-16T20:00:57Z

It appears this isn't ready yet for approval. Let me know if you think otherwise.

…tries

SunSerega · 2022-12-04T23:26:36Z

Well, there are also glGetUniform*, which don't fit (they can be used with any size of vectors)
And there are a few exotic ones: *Uniformui64v* and *UniformHandleui64v*, which, I think, only accept individual values, not vectors.

But anyway, I think it's better to now finish up Clamp01 andSampleMaskNV, and un-draft this pull.
IMO there is nothing left that needed to be done before the first version of the kind attribute. The rest can be done in separate incremental pulls.

NogginBops · 2022-12-05T12:30:55Z

I've fixed the Clamp01 and SampleMaskNV things.

The functions
glUniformui64vNV,
glProgramUniformui64vNV,
glUniformHandleui64vARB,
glUniformHandleui64vIMG,
glUniformHandleui64vNV,
glProgramUniformHandleui64vARB,
glProgramUniformHandleui64vIMG, and
glProgramUniformHandleui64vNV

all take special values such as buffer addresses or bindless texture handles.
I feel like kind=Vector1 would be the wrong kind to put here. I would suggest we add something like kind=BindlessTextureHandle (or some other name) in a future PR.

xml/registry.rnc

Co-authored-by: Sun Serega <sunserega2@gmail.com>

NogginBops · 2022-12-05T22:32:31Z

Is there anything more to do or should I make this ready for review?

SunSerega

Yeah, I think it's ready.

pdaniell-nv · 2022-12-06T20:39:06Z

@Perksey would you like to give this another review?

Perksey

I'm pretty happy with these changes, I think the final form of these attributes makes sense given these are meant to be a "hint" and not gospel for what a parameter value represents - it's a good balance.

There are a few things worth double checking (and they may already have been discussed, so apologies if so!) but these needn't block merging, I'm happy with this is as is.

xml/gl.xml

Perksey · 2022-12-06T21:03:49Z

xml/gl.xml

@@ -9072,29 +9110,29 @@ typedef unsigned int GLhandleARB;
            <proto>void <name>glColorTableParameterfv</name></proto>
            <param group="ColorTableTarget"><ptype>GLenum</ptype> <name>target</name></param>
            <param group="ColorTableParameterPName"><ptype>GLenum</ptype> <name>pname</name></param>
-            <param group="CheckedFloat32" len="COMPSIZE(pname)">const <ptype>GLfloat</ptype> *<name>params</name></param>
+            <param kind="CheckedFloat32" len="COMPSIZE(pname)">const <ptype>GLfloat</ptype> *<name>params</name></param>


This kind hasn't been declared at this top of the file.

Some of CheckedFloat32 and CheckedInt32 kinds will need to be turned into Clamped[*], some may be more complex.
I proposed to leave them undocumented to say they are unsupported, while not deleting existing info from XML.
Similar in the case of Path and FenceNV kinds - but they need to turn into their respective class attribute.
Look at the "Long term (probably other PR's)" section of my first review.

Perksey · 2022-12-06T21:07:01Z

xml/gl.xml

@@ -9072,29 +9110,29 @@ typedef unsigned int GLhandleARB;
            <proto>void <name>glColorTableParameterfv</name></proto>
            <param group="ColorTableTarget"><ptype>GLenum</ptype> <name>target</name></param>
            <param group="ColorTableParameterPName"><ptype>GLenum</ptype> <name>pname</name></param>
-            <param group="CheckedFloat32" len="COMPSIZE(pname)">const <ptype>GLfloat</ptype> *<name>params</name></param>
+            <param kind="CheckedFloat32" len="COMPSIZE(pname)">const <ptype>GLfloat</ptype> *<name>params</name></param>


I'm fine either way, but wanted to point it out anyway: it seems the precedent set elsewhere in your changes remove type information from the kind names (e.g. Coord instead of CoordF) which makes sense given you can get this from the ptype. Is there any reason why this isn't done with this kind? (i.e. should this just be Checked?)

CheckedFloat32 is a name that comes from the old groups and I've probably not looked at what kind this particular parameter should have.
I can do a pass over them to see if any of them are easily changed or removed, but otherwise I think we can fix them in future PRs.

I have a working branch where I have already done the changes to remove all of the CheckedInt32 and CheckedFloat32 kinds, and removed them or replaced them with the appropriate replacement.
https://github.com/NogginBops/OpenGL-Registry/tree/more-kind-changes

NogginBops · 2022-12-07T13:46:11Z

I just realized I need to remove the Clamped[0; 1] on glClearColor, will do that soon.

oddhack

Aside from the note on restricting kind annotations to not contain the ',' separator, I'll need to run this through the schema validator and ensure the generated headers aren't affected before merging, but it seems fine.

xml/readme.tex

* Add SpriteModeSGIX group * Removed SpriteModeSGIX from glSpriteParameterfvSGIX * Added TextureEnvTarget, TextureEnvParameter, and TextureEnvMode groups * Removed group=SpriteModeSGIX from glSpriteParameterfSGIX * Fix broken xml

NogginBops · 2022-12-11T13:12:29Z

Should the kind="Color" attribute be changed to kinds="Color" to indicate that it is a list of kinds instead of a single kind.
I initially thought about kind="" as a single value, and it is kind of stuck in my mind, and I'm wondering if changing the plurality of the word used would be more clear?

Perksey · 2022-12-11T14:13:11Z

I think it's fine as is, as to me it reads as "the parameter is of a kind that is a color and is clamped to a specific range" i.e. they're describing the properties of the kind, they are not kinds themselves.

pdaniell-nv · 2023-02-14T18:24:32Z

@NogginBops could you resolve the merge conflicts? I assume otherwise this is ready to go in? Thanks.

pdaniell-nv · 2023-02-14T18:27:11Z

Aside from the note on restricting kind annotations to not contain the ',' separator, I'll need to run this through the schema validator and ensure the generated headers aren't affected before merging, but it seems fine.

@oddhack did you get a chance to run this through the schema validator?

pdaniell-nv · 2023-02-15T17:19:48Z

@oddhack, this is approved to merge as soon as the merge conflicts are resolved and you're happy with it.

…ename-groups-that-are-not-enums

NogginBops · 2023-02-15T20:13:11Z

Fixed merge conflicts.

NogginBops added 2 commits August 9, 2022 01:12

Rename group attribute for non GLenum parameters not corresponding to…

28073d6

… any enum group.

Changed all Boolean group attributes into the new kind attribute

4ff6e3f

Perksey suggested changes Aug 8, 2022

View reviewed changes

NogginBops mentioned this pull request Aug 8, 2022

[5.0] Fix non GLenum enums opentk/opentk#1493

Merged

NogginBops added 23 commits August 29, 2022 17:39

Removed kind tag for parameters that already had a class tag

e2a495f

Removed kind=handleARB

85426a8

Remove kind=Half16NV

f9848e5

Remove kind=Boolean

ffa6e73

Removed kind=BooleanPointer

ed8aeb9

ColorB -> Color

94e25f4

ColorS -> Color

f24d734

ColorI -> Color

091bb11

ColorD -> Color

fa711ce

ColorF -> Color

e4f3427

CoordS -> Coord

f55b9fc

CoordI -> Coord

4fa2caf

CoordD -> Coord

0a5c53c

CoordF -> Coord

3db09c0

Removed kind=vdpauSurfaceNV

b03d539

Rename kind=DrawBufferName to kind=DrawBufferIndex and add missing en…

94a9301

…tries

BufferOffsetARB -> BufferOffset

b467e4e

BufferSizeARB -> BufferSize

e31ac8b

Removed kind=PathElement

255fc83

Remove kind=LineStipple

77c4279

ProgramCharacterNV -> String

03ab4ee

Removed ReplacementCodeSUN

6576693

Removed accidentally added line at the end of gl.xml

8d20967

NogginBops added 2 commits December 5, 2022 13:06

Fixed kind=Clamped[0; 1] for glClearColor

fc1c642

Removed group=SampleMaskNV from glSampleMaskIndexedNV

679eb6e

SunSerega reviewed Dec 5, 2022

View reviewed changes

xml/registry.rnc Outdated Show resolved Hide resolved

Update xml/registry.rnc

0b11f9f

Co-authored-by: Sun Serega <sunserega2@gmail.com>

SunSerega approved these changes Dec 6, 2022

View reviewed changes

NogginBops marked this pull request as ready for review December 6, 2022 12:53

pdaniell-nv requested a review from oddhack December 6, 2022 20:38

Perksey approved these changes Dec 6, 2022

View reviewed changes

NogginBops added 2 commits December 8, 2022 08:50

Removed kind=Clamped[0; 1] from glClearColor

d533d62

Removed trailing space in <kind> tags

9cf76f6

oddhack requested changes Dec 9, 2022

View reviewed changes

xml/readme.tex Show resolved Hide resolved

Updated readme.tex

5d75aa7

pdaniell-nv assigned oddhack Feb 15, 2023

pdaniell-nv added this to the Approved to Merge milestone Feb 15, 2023

Merge branch 'main' of github.com:KhronosGroup/OpenGL-Registry into r…

604b96b

…ename-groups-that-are-not-enums

oddhack merged commit 63161d6 into KhronosGroup:main Feb 20, 2023

SunSerega mentioned this pull request May 24, 2023

[XML] Performance query group and class definitions #576

Merged

NogginBops deleted the rename-groups-that-are-not-enums branch October 16, 2023 12:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

NogginBops commented Aug 8, 2022

Perksey left a comment

SunSerega commented Aug 9, 2022 •

edited

oddhack commented Aug 9, 2022

pdaniell-nv commented Aug 16, 2022

SunSerega commented Dec 4, 2022 •

edited

NogginBops commented Dec 5, 2022

NogginBops commented Dec 5, 2022

SunSerega left a comment

pdaniell-nv commented Dec 6, 2022

Perksey left a comment

Perksey Dec 6, 2022

SunSerega Dec 6, 2022 •

edited

Perksey Dec 6, 2022 •

edited

NogginBops Dec 7, 2022

NogginBops Dec 8, 2022

NogginBops commented Dec 7, 2022

oddhack left a comment

NogginBops commented Dec 11, 2022

Perksey commented Dec 11, 2022 •

edited

pdaniell-nv commented Feb 14, 2023

pdaniell-nv commented Feb 14, 2023

pdaniell-nv commented Feb 15, 2023

NogginBops commented Feb 15, 2023

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

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

Conversation

NogginBops commented Aug 8, 2022

Perksey left a comment

Choose a reason for hiding this comment

SunSerega commented Aug 9, 2022 • edited

oddhack commented Aug 9, 2022

pdaniell-nv commented Aug 16, 2022

SunSerega commented Dec 4, 2022 • edited

NogginBops commented Dec 5, 2022

NogginBops commented Dec 5, 2022

SunSerega left a comment

Choose a reason for hiding this comment

pdaniell-nv commented Dec 6, 2022

Perksey left a comment

Choose a reason for hiding this comment

Perksey Dec 6, 2022

Choose a reason for hiding this comment

SunSerega Dec 6, 2022 • edited

Choose a reason for hiding this comment

Perksey Dec 6, 2022 • edited

Choose a reason for hiding this comment

NogginBops Dec 7, 2022

Choose a reason for hiding this comment

NogginBops Dec 8, 2022

Choose a reason for hiding this comment

NogginBops commented Dec 7, 2022

oddhack left a comment

Choose a reason for hiding this comment

NogginBops commented Dec 11, 2022

Perksey commented Dec 11, 2022 • edited

pdaniell-nv commented Feb 14, 2023

pdaniell-nv commented Feb 14, 2023

pdaniell-nv commented Feb 15, 2023

NogginBops commented Feb 15, 2023

SunSerega commented Aug 9, 2022 •

edited

SunSerega commented Dec 4, 2022 •

edited

SunSerega Dec 6, 2022 •

edited

Perksey Dec 6, 2022 •

edited

Perksey commented Dec 11, 2022 •

edited