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

glx: Support making a context current without surface #1711

Merged
merged 3 commits into from
Nov 2, 2024

Conversation

mbernat
Copy link
Contributor

@mbernat mbernat commented Oct 29, 2024

This mirrors the EGL behavior and follows in footsteps of #1710.

This change should be valid according to https://registry.khronos.org/OpenGL/extensions/ARB/GLX_ARB_create_context.txt (emphasis mine)

"If either <draw> or <read> are not a valid GLX drawable, a
GLXBadDrawable error is generated, unless draw and read are both
None
and the OpenGL version supported by <ctx> is 3.0 or greater. In
this case the context is made current without a default framebuffer,
as defined in chapter 4 of the OpenGL 3.0 Specification."

  • Tested on all platforms changed
  • Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
  • Created or updated an example program if it would help users understand this functionality

@MarijnS95 MarijnS95 changed the title Support making a context current with GLX glx: Support making a context current without surface Oct 29, 2024
@MarijnS95
Copy link
Member

I presume this comes from the suggestion at #1710 (comment)?

@mbernat
Copy link
Contributor Author

mbernat commented Oct 29, 2024

@MarijnS95 Yes, indeed.

Didn't mean to open the PR just yet, hence the stump form of the PR request, but I'd be happy to see it through properly.

@MarijnS95
Copy link
Member

MarijnS95 commented Oct 29, 2024

@mbernat no worries, just figured we should update the title to include the without surface or surfaceless keyword :)

Happy that you included the documentation that clarifies that this is allowed, because glxMakeContextCurrent() does not (as expected).

Like the rest of glutin, how about we add some extra version/extension validation before calling the function like this, and have appropriate error handling otherwise?

@mbernat
Copy link
Contributor Author

mbernat commented Oct 29, 2024

@MarijnS95 Let me know whether the validation I added is along the lines you had in mind.

I also implemented the API for WGL elsewhere but the validation is harder to do because its Display doesn't carry a version info, unlike in the other backends. Also, I don't really have a windows use case to test this against. But other than that I can post what I have so far as a separate PR and we can take it from there.

@mbernat
Copy link
Contributor Author

mbernat commented Oct 31, 2024

I have fixed the validation, the previous commit was using the GLX version stored in Display rather than the OpenGL version as determined by context attributes and profiles (oops).

It's a bit hairy now since the version is only computed in the middle of the context creation and I needed to get the information about surfaceless support out from there; it would be better to compute the version much earlier. Let me know and I'd be happy to try refactoring it.

Other than that, this should be ready for review.

@MarijnS95 @kchibisov please take a look whenever it's convenient for you 🙏
Not urgent but I'm not very experienced with open source and the review process here, so just a gentle reminder 😇

@strohel
Copy link

strohel commented Oct 31, 2024

I've just successfully tested this one (in a proprietary application).

@kchibisov kchibisov merged commit ed240ab into rust-windowing:master Nov 2, 2024
43 checks passed
@kchibisov
Copy link
Member

Just for the reference, querying the context version afterwards is not really possible, and it's just the same as it was passed to the API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants