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] Definition of object namespaces #428

Merged
merged 12 commits into from
Oct 28, 2020

Conversation

SunSerega
Copy link
Contributor

@SunSerega SunSerega commented Sep 3, 2020

Object name that you get from glCreateBuffers can not be passed to glActiveProgram. Any high-level-ish binding would want to enforce this.

And well, my bindings already had system for that, for a while now. This list was made by going through 4.6 core specification and manually deciding which object type go where. When making this system - i was thinking of at some point translating it as-is into xml files. But when i actually sat down to do it - i decided to apply it to every function i can find.


And to how i implemented it:

There already are attributes like group="Texture", but they are really inconsistent (mostly at existing). And i don't think it's a good idea to use single attribute name for all sorts of different things, better keep group for groups of enum values. So i made new attribute object_name_type.

I made values of this attribute lower case and space separated i.e. as formatless as i can make it, so they are easy to format into any naming convention, like vertexArray, vertex_array, etc.


I'm not sure what to do with these functions:

glAttachObjectARB
glBindAttribLocationARB
glCompileShaderARB
glCreateProgramObjectARB
glCreateShaderObjectARB
glDetachObjectARB
glGetAttribLocationARB
glGetUniformivARB
glGetUniformfvARB
glGetUniformLocationARB
glLinkProgramARB
glShaderSourceARB
glUseProgramObjectARB
glValidateProgramARB
glGetActiveUniformARB
glGetActiveAttribARB

They define core function as alias, but they take GLhandleARB instead of GLuint as object name. And it's defined as folowing:

        <type name="GLhandleARB">#ifdef __APPLE__
typedef void *GLhandleARB;
#else
typedef unsigned int GLhandleARB;
#endif</type>

Should they also have this attribute, even though argument type isn't quite the same?


Also, binaryFormat argument of glGetProgramBinary and glProgramBinary is a number, which value is implementation defined and only values that you get from glGetProgramBinary should be used glProgramBinary. So, i guess, same reasoning as to object names applies here. But i'm not sure if i should use same attribute for it.


I didn't actually add object_name_type for all object types - i wanted to add object types defined in extensions separately, if this pull goes well.
But i might as well start discussion on it here... Here is everything i found, except clearly irrelevant or just dead extensions like AMD_performance_monitor:

Extension Types of object names
ATI_vertex_array_object vertex array object
[ARB,NV]_bindless_texture texture handle
NV_command_list state, command list
NV_path_rendering path
NV_occlusion_query occlusion query
EXT_external_objects memory object, semaphore
[APPLE,NV]_fence fence
NVX_progress_fence progress fence
INTEL_performance_query performance query
NV_query_resource_tag query resource tag
INTEL_performance_query performance query id, performance query handle

And and bit of offtop - i noticed that last argument of glBindImageTextureEXT is:

            <param><ptype>GLint</ptype> <name>format</name></param>

While in glBindImageTexture it's defined as:

            <param group="InternalFormat"><ptype>GLenum</ptype> <name>format</name></param>

Maybe one in glBindImageTextureEXT should be changed to match glBindImageTexture?

@SunSerega
Copy link
Contributor Author

Also uploaded schema changes, but still struggling to validate them without installing python...
Besides online validators - i tried a few listed here. The ones i managed to get working seem to hate the very first symbol of registry.rnc, which is a # (comment start)... Any advices on what to use?

@pdaniell-nv
Copy link
Contributor

@oddhack are there any downsides to all these new attributes? I guess folks will need to remember to add them when adding new commands.

@null77
Copy link
Contributor

null77 commented Sep 16, 2020

ANGLE maintains its own separate object type annotations for entry points. Our work is similar to this change. We use the types to map from GLuint to strictly typed ID handles (nominally a zero-cost transformation). Link to our mapping below:

https://chromium.googlesource.com/angle/angle.git/+/3018d5e57091200217e2bd688df28f52f079bab9/scripts/entry_point_packed_gl_enums.json

These are used in code gen and turn into code like this:

void GL_APIENTRY BindBuffer(GLenum target, GLuint buffer)
{
    Context *context = GetValidGlobalContext();
    if (context) {
        BufferBinding targetPacked                            = FromGL<BufferBinding>(target);
        BufferID bufferPacked                                 = FromGL<BufferID>(buffer);
        bool isCallValid =
            (context->skipValidation() || ValidateBindBuffer(context, targetPacked, bufferPacked));
        if (isCallValid) {
            context->bindBuffer(targetPacked, bufferPacked);
        }
    }
}

We also annotate enum types with specific types. Our enum type annotation is also similar to the related enum work in #335 .

I do notice two things about this change:

  • you're missing Semaphore and MemoryObject types. these aren't common so it's unsurprising they're missed
  • Programs and Shaders actually share the same namespace. ANGLE defines these as ShaderProgramID. since splitting them is a super-set of the information it's not hard to work with that design if some implementations need the more detailed info.

There's also a concern that this new field is easy to miss on new APIs and we might be missing other object types. A presubmit check would be great if such a thing is possible. Also the API is moving more slowly now so it might not big a bug burden since there are fewer new object types being introduced.

I'm also not sure why you didn't squash the XML changes together. Might make it easier to review, not sure.

@null77
Copy link
Contributor

null77 commented Sep 16, 2020

you're missing Semaphore and MemoryObject types. these aren't common so it's unsurprising they're missed

Ah, I see you address this in your introduction where you want to do extensions separately. That seems reasonable as long as there's some checking that we don't miss out on any object types.

@SunSerega
Copy link
Contributor Author

Programs and Shaders actually share the same namespace.

Oh, i never noticed this in specs before you said it...
But is there actually case where both shader and program objects can be passed to the same argument of a function? Or are combined namespaces here just an implementation technicality?

@null77
Copy link
Contributor

null77 commented Sep 16, 2020

@SunSerega the specs clearly indicate they're from the same namespace in the error conditions

e.g. for glDeleteShader:

An INVALID_VALUE error is generated if shader is neither zero nor the
name of either a program or shader object.

All the APIs work the same way.

@SunSerega
Copy link
Contributor Author

the specs clearly indicate they're from the same namespace in the error conditions

Yeah i went though specs before asking. And for all functions i could find (in 4.6 core spec) i see that full story is:

An INVALID_VALUE error is generated if shader is neither zero nor the
name of either a program or shader object.
An INVALID_OPERATION error is generated if shader is not zero and is
the name of a program object.

So while passing program object into shader argument gives different error than if you pass buffer object there, it is no less invalid. That leads me to think it's only an implementation technicality based on time when there was a reason to combine these namespaces.

If so - it shouldn't affect object_name_type attribute, as it's point is to be a common info storage for devs of multiple high-level bindings to use (and potentially submit fixes) for separation of object name types, which can't be used interchangeably, instead of everyone making their own system.
I imagine it would greatly reduce bugs for every of those devs, even if adding this attribute when creating new functions wouldn't be required by Khronos in any way.

Though documentation i added need to change then, i just assumed that each object type has own namespace. (this is mostly reminder for myself)


But i would prefer to first hear out someone who knows why do shaders and programs have a common namespace.

@null77
Copy link
Contributor

null77 commented Sep 22, 2020

@SunSerega it's totally fine to split programs and shaders as it's strictly more information than merging them. ANGLE would have no problem handling that. And some integrations might require them to be separate. So SGTM.

@SunSerega
Copy link
Contributor Author

Ah ok, well if it's not an objection - might as well change docs now.

@Perksey
Copy link
Contributor

Perksey commented Sep 23, 2020

Love it!

A few nits from here:

  • Not a fan of the snake case name. We could probably change this to just object or class maybe?
  • Make sure you update the RNC specification to make this new attribute optional like groups as they won't be used for Khronos purposes.

@SunSerega
Copy link
Contributor Author

Not a fan of the snake case name

Well, if anything short - then probably type. But, while long name is indeed ugly, i wanted to make it self explanatory...
Though that was before i remembered to also put explanation in comments in registry.rnc, so now i'm not so sure if long name is necessary.

make this new attribute optional

Ah, i thought i made them optional from the start... Thanks for reminding.
(and i would probably still need to install python on this pc, as i didn't manage to get any of RelaxNG verifiers working without it... i would've caught this on my own if i did verify all xml's)

@Perksey
Copy link
Contributor

Perksey commented Sep 25, 2020

Yeah registry.rnc is probably the best place for explanation, doesn't have to be the name! type could look misleading on a ptype, so perhaps something like class, category, object, etc.

@SunSerega
Copy link
Contributor Author

class it is. At least i can't come up with single word which would be better.

Also, i looked again at Makefile and... Noticed i wasn't adding -c argument when calling jing directly, that's why it wasn't working. Well, now .xml's are validated.

xml/registry.rnc Outdated Show resolved Hide resolved
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.

Awesome, thanks for implementing my changes (I know I can be annoying sometimes haha)

Looks good on my side, seems like a pretty benign yet brilliant change!

cc @Dav1dde as GLAD was affected last time we introduced a new attribute, most other parsers should be able to gloss over unrecognized attributes.

@pdaniell-nv
Copy link
Contributor

@oddhack, this is approved to merge.

@oddhack
Copy link
Collaborator

oddhack commented Oct 14, 2020

@SunSerega please add descriptions of schema additions to xml/registry.tex. I'll regenerate the PDF and merge after that's done.

@SunSerega
Copy link
Contributor Author

xml/registry.tex

You mean xml/readme.tex? Ok sec.

@SunSerega
Copy link
Contributor Author

Done.

I think this way it makes much more sense...
@pdaniell-nv pdaniell-nv requested a review from oddhack October 27, 2020 19:02
@pdaniell-nv
Copy link
Contributor

@oddhack this can be merged if you're happy with the update.

@oddhack oddhack merged commit 6bd9221 into KhronosGroup:master Oct 28, 2020
<param group="SyncObjectMask"><ptype>GLbitfield</ptype> <name>flags</name></param>
<param><ptype>GLuint64</ptype> <name>timeout</name></param>
</command>
<command>
<proto group="SyncStatus"><ptype>GLenum</ptype> <name>glClientWaitSyncAPPLE</name></proto>
<param group="sync"><ptype>GLsync</ptype> <name>sync</name></param>
<param><ptype>GLsync</ptype> <name>sync</name></param>
Copy link
Collaborator

Choose a reason for hiding this comment

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

This was a typo, parameter was duplicated. Will be fixed shortly.

oddhack added a commit that referenced this pull request Oct 28, 2020
regenerate headers for a new QCOM extension.
XZiar pushed a commit to XZiar/OpenGL-Registry that referenced this pull request Apr 28, 2021
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.

5 participants