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

Renamed "cap" parameter of "glEnable", "glDisable", "glIsEnabled" commands to "target" #593

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Rytis-Stan
Copy link
Contributor

The renaming was done to be consistent with "glEnablei", "glDisablei" and "glIsEnabledi" commands (both in this XML and in the specification PDFs). Note that the parameter of "glIsEnabled" command should technically be called "value" (according to the specification PDFs), but it was renamed to "target" for sake of consistency among all the related commands. Alternatively, it can be renamed to "value" again to completely match the specifications

…mands to "target"

The renaming was done to be consistent with "glEnablei", "glDisablei" and "glIsEnabledi" commands (both in this XML and in the specification PDFs). Note that the parameter of "glIsEnabled" command should technically be called "value" (according to the specification PDFs), but it was renamed to "target" for sake of consistency among all the related commands. Alternatively, it can be renamed to "value" again to completely match the specifications
@Rytis-Stan
Copy link
Contributor Author

Rytis-Stan commented Oct 28, 2023

The naming is quite chaotic even in the documentation pages, which use the "cap" name. For example:
https://registry.khronos.org/OpenGL-Refpages/gl4/html/glEnable.xhtml
https://registry.khronos.org/OpenGL-Refpages/gl2.1/xhtml/glEnable.xml

Would be great to fix the naming everywhere consistently. Besides the naming mismatches in different places, there seems to be a tension of what naming system to use. Ideally, everything should match the specifications. But keeping naming consistent among related commands would also be great.

If anyone has suggestions on how to best approach this, comments would be welcome.

@Rytis-Stan Rytis-Stan changed the title Renamed "cap" parameter of "glEnable", "glDisable", "glIsEnabled" comands to "target" Renamed "cap" parameter of "glEnable", "glDisable", "glIsEnabled" commands to "target" Oct 28, 2023
…mands to "target"

The renaming was done to be consistent with "glEnablei", "glDisablei" and "glIsEnabledi" commands (both in this XML and in the specification PDFs). Note that the parameter of "glIsEnabled" command should technically be called "value" (according to the specification PDFs), but it was renamed to "target" for sake of consistency among all the related commands. Alternatively, it can be renamed to "value" again to completely match the specifications
@SunSerega
Copy link
Contributor

Well, if the parameter is consistently target now (tho in some places, like glDisableClientState - it's array) - then the group EnableCap should also get renamed...

@Rytis-Stan
Copy link
Contributor Author

Well, if the parameter is consistently target now (tho in some places, like glDisableClientState - it's array) - then the group EnableCap should also get renamed...

Any suggestions on how to rename EnableCap?

@SunSerega
Copy link
Contributor

I was thinking the same way, from EnableCap to EnableTarget - to use the same term.

@Rytis-Stan
Copy link
Contributor Author

@SunSerega - Renamed EnableCap to EnableTarget. Not sure if array parameters should be renamed (the name target sounds better to me than array, but array is the name used in the specification PDFs).

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.

I'm generally against renaming groups without them being fundamentally broken or misrepresentative. Renaming a parameter is fine as the scenarios in which this would break a user anywhere are much more limited (but not zero). Renaming a group would cause widespread breaking changes in transitive downstream consumers which typically rely on these names as a core part of their API surface.

We've made these types of breaking changes before, but only when the groups have been massively misrepresentative of what they're actually doing or otherwise invalid and in desperate need of change. This would be breaking a potentially massive number of users over a nitpick.

I would suggest reverting the group changes until #481 is in place.

@SunSerega
Copy link
Contributor

I would suggest reverting the group changes until #481 is in place.

Wouldn't that be a time when improvements like these would be impossible for sure? And even if not, #481 has been silent for a while...

@Perksey
Copy link
Contributor

Perksey commented Nov 3, 2023

Not really, we can decide that we want to break everything, but that has to be a decision made collectively. It is irresponsible to knowingly break potentially hundreds of downstream projects without publishing formal documentation saying we reserve the right to do so. That is what #481 is about.

@Rytis-Stan
Copy link
Contributor Author

@Perksey - What would be your suggestion for this PR? To just rename parameters? Or to keep this PR on hold until #481 has come to a conclusion?

@Perksey
Copy link
Contributor

Perksey commented Nov 19, 2023

I think it would be irresponsible to go ahead with this PR unless we have set a precedent for doing so per #481. If there is particular desire from contributors to go ahead with changes like these, I'd recommend such contributors work with the necessary stakeholders to drive #481 to consensus.

@SunSerega
Copy link
Contributor

Fair enough... I have a lot of doubts about continuing things like #543 - having guidelines would solve that, one way or another.
But as it is, I feel powerless about #481.
I mean, I voiced my stance on it... Which, honestly, feels far from the consensus. And maybe a bit overly dramatized, the way I put it.
But even without that - what else can I do? I don't think it's a solution to keep bugging people pinged in the header of #481. I guess we are still just waiting for more of them to react?

@NogginBops
Copy link
Contributor

If people have had more than 2 years to respond to a ping I think we can assume they are not going to reply.
I personally think we should set a goal where we want the enum groups to be in terms of consistency, correctness and expressiveness, and then we can evaluate enum group work according to if that gets us towards the goal of how we want the enums to behave.

In my own bindings I'm likely going to implement a translation table for enum group names so that I can rename them myself to remain "backwards compatible", won't fix members being added or removed but yeah I think that if you are using the latest gl.xml from the registry I think you should "expect" breakages.

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.

4 participants