Perksey commented 4 years ago

Introduction

One thing that's annoyed me about the current XML schema is that the strongly-typed enum groupings are separate to the rest of the specification. Granted, they aren't that useful in C but downstream consumers (such as my Silk.NET library for C#) who are using the XML specification to bind OpenGL to languages that do better support strongly typed enums.

Today's problem

The problem with the group tags being separate is that there's a lesser emphasis on the correctness of the groupings. And, to be fair to the Khronos Group, I see why they wouldn't need to worry about the groupings as they don't use them themselves and, as such, won't want to dedicate cycles to ensuring the groupings' correctness.

I want to tackle this problem by merging the <enum> blocks and the <group> blocks. If these structures are merged, it encourages spec editors to group the enums properly upon creating a new extension, and should hopefully solve the problem of invalid groupings going forward because if creating proper enum groups is a requirement upon vendors and extension writers, we won't have any group problems going forward.

Solution in action

My proposed solution uses already established schema elements such that it wouldn't be too breaking for the Khronos Group themselves, but may affect downstream consumers. This is why I want to consult with these consumers so that we can establish whether this is a worthy enhancement to the spec.

Current syntax

<groups>
    <group name="GroupName">
        <enum name="GL_ENUM_NAME" />
    </group>
    <group name="AnotherGroupAMD">
        <enum name="GL_ANOTHER_ENUM_AMD" />
    </group>
    <group name="AGroupThatReusesTokensEXT">
        <enum name="GL_ENUM_NAME" />
        <enum name="GL_ANOTHER_ENUM_AMD" />
    </group>
</groups>

Proposed Syntax

<enums namespace="GL" start="0x0001" end="0x0002">
    <enum value="0x0001" name="GL_ENUM_NAME" group="GroupName,AGroupThatReusesTokensEXT" />
    <enum value="0x0002" name="GL_ANOTHER_ENUM_AMD" group="AnotherGroupAMD,AGroupThatReusesTokensEXT" />
</enums>

Pros

This syntax doesn't require multiple enum definitions for reuse of an enum in multiple groups. (we will use a comma to separate groups in the group element i.e. group="FirstGroup,SecondGroupINTEL")
Each enum addition to the spec being in its own group encourages extension authors to place all new enums in their own group, eliminating the need for contributors such as myself to go back into the spec later on to add the groups for them.
No impact to the Khronos group.

Cons

May affect downstream consumers that rely on the group tags (we could probably think of some way to tackle this, but that's why this is a consultation and not a PR)

Proposed course of action

Add the group attributes to the correct enums with the data as outlined by this proposal. The <group> blocks will remain unchanged for now.
Announce a date (either here or somewhere else where we can capture the attention of spec consumers) that the Khronos Group intends to remove the <group> blocks.
Remove the <group> blocks after as many enums have been grouped as possible.

Closing

I understand that the groupings affect the Khronos group in no way shape or form, but this change matters plenty to downstream consumers that might be depending on the groups and this change will be doing them a favour in the long-term.

Thank you for reading and I hope you take this into consideration.

Version History

Date	Message
06/12/2019	Initial proposal.
12/12/2019	Switch gears to using group attributes instead, eliminating problems with duplicates as suggested by Jon and jvbsl.
17/12/2019	Add token reuse using the `\\|` separator
31/12/2019	Switch to the `,` separator for token reuse

Stakeholders

OpenGLAda - @flyx cl-opengl - @3b CSGL - @thatonecheetah opengl4csharp - @giawa OpenGLDotNet - @carmack78 OpenGL.Net - @luca-piccioni OpenTK - @varon @jvbsl @frederikja163 Silk.NET - myself @frederikja163 @AzyIsCool dglOpenGL - @saschawillems JOGL - @sgothel LWJGL - @spasi LuaGL - @drahosp glMLite - @fccm ModernGL - @cprogrammer1994 PyOpenGLng - @FabriceSalvaire Racket OpenGL - @stephanh42 Ruby OpenGL - @vaiorabbit

Please tag anyone I may have missed who might be impacted by this.

oddhack commented 4 years ago

The expression of enum group allocation and use that the current XML contains is important to us - the SGI enum registry was literally the place gl.xml began from. I don't see how you can simultaneously use the enums tags for semantic grouping and numerical grouping without simply replicating every enum in at least two places, so I'm not feeling much enthusiasm for this idea - although I applaud your efforts to construct it in terms of the existing schema.

Perksey commented 4 years ago

Yeah I understand that, and perhaps we need to look further into the current state of the groupings to look at what can be done to achieve the main goal: enforcing validity of groupings without causing any further trouble for vendors and spec editors.

oddhack commented 4 years ago

A way of doing this that would probably have no effect on current consumers is to move the group attribute into the individual \ tags rather than using \ as a grouping mechanism. It is kinda wordy (expands the file size by 5-10% at a guess), but since it's just a new attribute on an existing tag, it should be ignored by our scripts, and probably by most processing tools unaware of it.

Perksey commented 4 years ago

Yeah that works for me, I’ll update the proposal next time I’m on a computer.

On Thu, 12 Dec 2019 at 10:37, Jon Leech notifications@github.com wrote:

A way of doing this that would probably have no effect on current consumers is to move the group attribute into the individual tags rather than using as a grouping mechanism. It is kinda wordy (expands the file size by 5-10% at a guess), but since it's just a new attribute on an existing tag, it should be ignored by our scripts, and probably by most processing tools unaware of it.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/KhronosGroup/OpenGL-Registry/issues/335?email_source=notifications&email_token=ACVEYI65PGT3FAECFPX345TQYIH5TA5CNFSM4JW74LVKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGWHKMY#issuecomment-564950323, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACVEYI7OEJX463XQCQXHDJ3QYIH5TANCNFSM4JW74LVA .

oddhack commented 4 years ago

OK. If we go this way, please add the attributes at the end of the tag, not the beginning - keeping the name & value first is preferable for me.

Perksey commented 4 years ago

Yeah I agree with that 100%, putting it elsewhere would make it a lot harder to edit the enums.

On Thu, 12 Dec 2019 at 11:41, Jon Leech notifications@github.com wrote:

OK. If we go this way, please add the attributes at the end of the tag, not the beginning - keeping the name & value first is preferable for me.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/KhronosGroup/OpenGL-Registry/issues/335?email_source=notifications&email_token=ACVEYIZBZQZ6FOWZV3ARXL3QYIPPDA5CNFSM4JW74LVKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGWMXQY#issuecomment-564972483, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACVEYI6NSXKSM75BUHGTM4LQYIPPDANCNFSM4JW74LVA .

Perksey commented 4 years ago

I've updated the issue, let me know if there's any further comments. I've also been hearing radio silence from the downstream consumers which is slightly worrying.

fccm commented 4 years ago

Maybe you also want to CC dbuenzli/tgls

Perksey commented 4 years ago

@dbuenzli any comments?

null77 commented 4 years ago

Impacts ANGLE as we use gl.xml to generate our entry points and other files. @null77 is my handle.

Perksey commented 4 years ago

@null77 Do you use the blocks at all? This proposal only affects this portion of the spec, if you're not parsing anything else then you won't be affected by this issue.

null77 commented 4 years ago

We do. They're helpful when converting a GLenum to string as some GLenum values overlap.

Perksey commented 4 years ago

Ah ok. In the Proposed course of action section of this document I outline a... well... proposed course of action. Is this good enough to ensure a smooth transition over to the group attributes?

null77 commented 4 years ago

The motivation sounds reasonable. Implementing a presubmit check that validates new enums have a group tag also sounds like it would solve your problem without causing conflicts downstream. What do you think? I'm assuming your motivation is that the groups aren't reliable right now.

Perksey commented 4 years ago

Yeah I was thinking about adding in a check as we already developed a small error checker application internally, but a presubmit check won’t fix the problem where the groupings are separate and, as a result, spec editors have to go out of their way to ensure group correctness whereas if they’re declared inline with the enum, it’s a lot easier for spec editors to declare the groups.

I want to work with downstream consumers before we do anything destructive like get rid of the blocks - I only want to do this once I know everyone’s ready

On Mon, 16 Dec 2019 at 22:54, Jamie Madill notifications@github.com wrote:

The motivation sounds reasonable. Implementing a presubmit check that validates new enums have a group tag also sounds like it would solve your problem without causing conflicts downstream. What do you think? I'm assuming your motivation is that the groups aren't reliable right now.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/KhronosGroup/OpenGL-Registry/issues/335?email_source=notifications&email_token=ACVEYI2GOVFPXPUMWSTVBWTQZABJDA5CNFSM4JW74LVKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHANACQ#issuecomment-566284298, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACVEYIYDYBMFWNBCIDFHGG3QZABJDANCNFSM4JW74LVA .

null77 commented 4 years ago

The proposal sounds reasonable to me. It is inconvenient a bit to have to redo our parsing. Also you might want to include an example of how you handle enums with multiple groups in your examples section.

Perksey commented 4 years ago

Yeah it’ll probably be a | separator. Will update the proposal when I’m next on a PC.

On Mon, 16 Dec 2019 at 23:00, Jamie Madill notifications@github.com wrote:

How do you handle enums that are in more than one group? I imagine you could use a separator token. Might want to include an example of this.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/KhronosGroup/OpenGL-Registry/issues/335?email_source=notifications&email_token=ACVEYI4JHPOXWJUHY5MKM23QZACBBA5CNFSM4JW74LVKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHANQBA#issuecomment-566286340, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACVEYI753EJ6Y5ZR7J5JAPDQZACBBANCNFSM4JW74LVA .

dbuenzli commented 4 years ago

I'm fine with the change. I dont have these things in my mind since I wrote them 6 years ago but a quick look seems to indicate I'm not using groups for generating the bindings in tgls (OCaml thin bindings to GL apis).

It's even likely my parser won't need to be changed since as far as I understand the idea is to drop groups and simply add a new attribute to enum (if that's correct I think it makes it clearer to present it that way). Please just make sure to update the readme.pdf that describes the format in case I need to get back to this.

Perksey commented 4 years ago

Alright have updated the proposal with the latest comments - thanks everyone for the help and co-operation btw, I understand that a lot of factors need to be considered when making such a huge change.

I think at the moment I'm gonna hold out for some formal comments from the OpenGL Working Group next time they have a OpenGL(ES) meeting, will make a PR after.

Perksey commented 4 years ago

@oddhack @pdaniell-nv Has this been discussed in an OpenGL(ES) working group meeting yet, if not have you any idea when it will be (if at all)?

oddhack commented 4 years ago

I expect we'll talk about it once meetings resume in mid-January. Many people are not available during the holidays and no working group meetings are happening. This is probably not going to be an objectionable change to Khronos, but I expect we'll want to have more confidence that XML consumers won't be unpleasantly surprised - staging it by adding the new attributes first and getting rid of the group tags later is a good approach. I'm unsure how to effectively reach a wide audience, although we could do an announcement on opengl.org / khronos.org newsfeed and maybe get some of the more active social media API bloggers to mention it.

I'm in favor of using , as the separator in the group attribute values, rather than |.

Perksey commented 4 years ago

I expect we'll talk about it once meetings resume in mid-January

Ah I didn't know they'd stopped for now. My apologies :|

but I expect we'll want to have more confidence that XML consumers won't be unpleasantly surprised

Yeah I've tried to do that as best I can by tagging maintainers of known bindings libraries here on GitHub, but doing an announcement somewhere is probably needed to notify all downstream consumers.

I'm in favor of using , as the separator in the group attribute values, rather than |.

Yeah | certainly isn't pretty and I was originally going to use ,, but opted for | to be consistent with the rest of the spec (i.e. supported="gles1|gles2"). I shall update the proposal now.

Once this has been discussed in a working group meeting, I shall create a PR moving the group definitions to the new attribute, but leave the old blocks alone until given the green-light by the working group.

giawa commented 4 years ago

Thanks for looping me in. My bindings actually process the man pages instead of using the xml schema (I didn't even know the xml existed). I'll look at using the xml schema in the future, but for now I am unaffected.

Perksey commented 4 years ago

Awesome, glad to hear :)

oddhack commented 4 years ago

@giawa be aware that the refpages are not a great place to get comprehensive API information from. They only go up to GL 4.5 and do not include extensions.

oddhack commented 4 years ago

@pdaniell-nv please put on the WG agenda when meetings start up again. I think this proposal has evolved far enough to be adoptable by us - there are some questions about how to transition from the old to the new schema but leaving the old group tags in place for a while should make this have low impact on XML consumers. The new attributes are likely to be ignored by existing consumers.

pdaniell-nv commented 4 years ago

We discussed the proposal in the OpenGL/ES joint working group meeting and we approve the plan. We look forward to seeing the PR(s).

Perksey commented 4 years ago

The initial migration PR is up. Once that has been merged in, all that is left is to decide on a date on which the group blocks will be removed, and then finally remove them.

frederikja163 commented 4 years ago

Just saw this, but I must say, anything to improve the useability of the enum groupings are very welcome to me, I'm not personally working with the gl.xml in the projects that are using it I'm involved with. However, I have faith that the people directly working with gl.xml are more than capable and motivated to go through with these changes. My main concern is the implementation and if the transfer will be done to an extent where it will be useable, the last thing we want at this point is two sets of broken enum groupings.

TL;DR: I'm in favor of this, but definitely think the new changes should be implemented before removing the old tags.

Perksey commented 4 years ago

Aha hey Fred 👋

Yeah definitely, and I don’t want to remove the old groups until a sufficient time has passed to allow downstream consumers (such as Google/the ANGLE team, OpenTK, Silk.NET, to name a few) to move over to the new format.

Mind, I want most of the engineering effort to be focused on the group attributes rather than the old groupings to avoid duplication of work and/or the new being out of sync with the old. This is why I have inserted a noticeable comment in each group in PR #343 - my first PR regarding this proposal which adds the group attributes to each enumerabt.

On Wed, 8 Jan 2020 at 22:40, frederikja163 notifications@github.com wrote:

Just saw this, but I must say, anything to improve the useability of the enum groupings are very welcome to me, I'm not personally working with the gl.xml in the projects that are using it I'm involved with. However, I have faith that the people directly working with gl.xml are more than capable and motivated to go through with these changes. My main concern is the implementation and if the transfer will be done to an extent where it will be useable, the last thing we want at this point is two sets of broken enum groupings.

TIL; I'm in favor of this, but definitely think the new changes should be implemented before removing the old tags.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/KhronosGroup/OpenGL-Registry/issues/335?email_source=notifications&email_token=ACVEYI3TJLEVWKMVS66JNVLQ4ZI6BA5CNFSM4JW74LVKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIOH4TY#issuecomment-572292687, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACVEYI4XI2WJOH3NQCPEMJLQ4ZI6BANCNFSM4JW74LVA .

oddhack commented 4 years ago

346 adds documentation of schema change from #343, should be accepted once #343 is merged.

Dav1dde commented 4 years ago

As the author of glad, the gl.xml has many issues which some of them have been solved already by newer specifications like Vulkan, I am all in favor for making the specification better but at the same time I want to warn about breaking changes without having some kind of (semantic) versioning on the specification.

There are many tools out there like glad or ones that are just a 200 lines Python script that are used by many people and haven't been maintained or updated in years and may break overnight without a clear indication. This has already happened before with only a small change (I would consider this a big change).

I only saw this because someone linked this issue in an unrelated issue to me, many others won't see it either (e.g. libepoxy hasn't been tagged either @ebassi @anholt).

TL;DR yes please improve the spec, but be careful about breaking changes or have a way to communicate them.

Sorry a little bit off-topic, but maybe something to consider.

Perksey commented 4 years ago

Hi David, thanks for your input. Ultz and Khronos both understand the implications of a change such as this, although it's worth mentioning that the groupings have been effectively deprecated since the demise of Silicon Graphics.

Regarding the breaking changes, we intend to keep the old-style group blocks in read-only mode to avoid parsers from breaking, giving the sufficient time to move over to the new format. They will be removed once Khronos has:

Allocated a date by which the old-style group blocks will be removed.
Figured out an efficient and effective way to communicate the impending change with downstream consumers. For example, here's Jon's earlier comment:

I'm unsure how to effectively reach a wide audience, although we could do an announcement on opengl.org / khronos.org newsfeed and maybe get some of the more active social media API bloggers to mention it.

This is the primary concern for now.

Perksey commented 4 years ago

My end goal of this proposal was to make it easier to add new groupings, in hopes that Khronos and other spec editors will add groupings for each new enumerant and extension.

I've spoke with @frederikja163 about this: I have no expectations for Khronos to fix the damage that has already been done regarding the groupings, but I hope that now it's easier to declare groups that with each enumerant that is declared, its group is also declared at the same time. Myself, Fred, and @jvbsl will continue to work to bring the existing groupings up-to-date.

Dav1dde commented 4 years ago

@Perksey I am looking forward to all your improvements on the spec, they are necessary and/or helpful, change is good, also it is great it see someone actively work on it and Khronos (+ all members attached) are open for it.

My concern was more general, something to remember and if there is a breaking change to do it consciously and at best with a clear way forward.

Perksey commented 4 years ago

Ah okay, yeah I fully understand. We won't remove the groupings until all the significant downstream consumers are ready. If we've missed any of them, please tag them in here!

Perksey commented 4 years ago

@nigels-com any comments? Will this break GLEW?

3b commented 4 years ago

Changes look good for cl-opengl, and I build bindings manually to edit new function names anyway, so transition doesn't matter too much for me.

I notice the new patch adds the group= attributes to enums/enum. I wonder if it would be better to add it to feature/require/enum and extension/require/enum instead?

It might be a bit more work for the initial translation, but would possibly make it easier to check against extension specs if the groups for a specific extension were together. Might also be easier for extension writers, since they would presumably be adding the extension/require/enum even if they don't need to add an enums/enum entry. It would also be useful for things like tools to detect which extensions or versions are actually used by a piece of code, or if people want to build "core profile only" bindings or similar.

Doesn't seem like it would complicate it much for users of the xml, just possibly removing some duplicates if an enum is added by more than 1 extension or feature level.

nigels-com commented 4 years ago

@Perksey I'm not too daunted from the GLEW point of view. It may break GLEW in some workflows, but my feeling is that comes with the territory.

oddhack commented 4 years ago

I don't have deep knowledge of associating XML schemas with XML documents, but https://www.w3.org/XML/2010/01/xml-model/ appears to bear on this. We could add some xml-model directives to gl.xml, although how to namespace minor iterations to the schema isn't entirely obvious. Perhaps the xml-model directive could refer to a github URL pointing at the latest update to the RNC schema at the time the document was edited. It would require some discipline whenever introducing changes to the schema as it's the sort of overhead that's easily forgotten, although if the changes break schema validation, CI could potentially catch it.

Perksey commented 4 years ago

Sounds like a plan. Will adjust the existing PR later on today.

On Mon, 13 Jan 2020 at 01:41, Jon Leech notifications@github.com wrote:

I don't have deep knowledge of associating XML schemas with XML documents, but https://www.w3.org/XML/2010/01/xml-model/ appears to bear on this. We could add some xml-model directives to gl.xml, although how to namespace minor iterations to the schema isn't entirely obvious. Perhaps the xml-model directive could refer to a github URL pointing at the latest update to the RNC schema at the time the document was edited. It would require some discipline whenever introducing changes to the schema as it's the sort of overhead that's easily forgotten, although if the changes break schema validation, CI could potentially catch it.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/KhronosGroup/OpenGL-Registry/issues/335?email_source=notifications&email_token=ACVEYI732VGS3B3OW54AYBDQ5PBEZA5CNFSM4JW74LVKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIXKRUQ#issuecomment-573483218, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACVEYI2IIV2HGHDBQ5KNDUDQ5PBEZANCNFSM4JW74LVA .

ebassi commented 4 years ago

From the perspective of libepoxy this change looks good; our XML → C dispatch generation only cares about enums/enum definitions and does not look at groups/enum at all, under the operating assumption that enumeration symbols are side-effect-free and harmless.

If we wanted to change the enumeration symbol definition code, having the group(s) in the same element as the enumeration would definitely make it easier/more efficient, as opposed to generating a lookup table on the side, so the proposed change looks good to me.

Perksey commented 4 years ago

Awesome, glad to hear. There aren't a lot of downstream consumers of the XML that use the groupings but we want to ensure the few that do are ready for the change.

Perksey commented 4 years ago

Please note: I'd also like to start thinking of a rough timeframe for removing the old blocks to defer use & modification of them. I get this isn't something that can't be done very suddenly but now we've seen the extent of these changes, we should be able to come up with a rough idea of how long the old format will last - months? a year? multiple years?

Perhaps this is something else that should be discussed at the working group meeting, although seeing as Khronos doesn't use the groupings themselves I'm not sure how useful that'll be without a bit of community input.

Dav1dde commented 4 years ago

My approach would be to keep the API as it is and generate the <groups> from the new <enum> information to have no breaking changes until there is a clear path (or decision) on how to deal with breaking changes.

If right now the decision is to have breaking changes at any time (also in the future), might as well remove the <groups> relatively soon.

Perksey commented 4 years ago

We could do that, however that involves setting up the necessary infrastructure to have that automatically done and I'm not in a position to do that at the moment.

I feel the <groups> being read-only herein until a given date (after which they'll be removed) is an acceptable way forward, but all I'm after is to finalize what that date actually is.

Perksey commented 4 years ago

I think we should allow around anywhere from 6 months to 1 year before we remove the old syntax to allow downstream dependencies in active development to adapt to the new syntax. Any project that doesn't update their copy of the spec within a year is probably dead anyway. This will probably need a little voice from the working group though.

Perksey commented 4 years ago

It's been a fair few months since the change was introduced and I think it's been a success - many thanks to Qualcomm for being a pioneer of this new system (which is exactly what I'd hoped to get out of this change: making it easier for official authors to introduce groups rather than them having to go far out of their way to introduce them)

The next step would be announcing an official removal date for the old group blocks, which won't be easy. From this thread, I've gathered there are downstream consumers using the groups:

~~GLAD (@Dav1dde)~~ all good
~~cl-opengl (@3b)~~ sufficiently prepared for the move
ANGLE (@null77)
~~OpenTK (@varon, @frederikja163)~~ sufficiently prepared for the move

If you could let me know of your status in the transition, this could help inform a decision as to when the old syntax should be deprecated and removed, which will likely need to be discussed with the OpenGL(ES) Working Group but I want to make sure that the consumers we do know of are prepared before the WG move to schedule when/if they'll be removed.

TL;DR lmk how your migrations to the group attributes are going - if you're all done then Khronos should probably decide when to remove the blocks.

Dav1dde commented 4 years ago

Glad has support for the new attributes, not much of a big change since I did not use them for codegen previously.

frederikja163 commented 4 years ago

OpenTK is using a fork of the spec for our current code generation, and our upcomming code generation is using the new onees, so not a big bias from here

KhronosGroup / OpenGL-Registry

XML Format Consultation: Merging <groups> and <enums> #335

Introduction

Today's problem

Solution in action

Current syntax

Proposed Syntax

Pros

Cons

Proposed course of action

Closing

Version History

Stakeholders

346 adds documentation of schema change from #343, should be accepted once #343 is merged.