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

Gentz' Misc Changes #1058

Closed
wants to merge 33 commits into from

Conversation

goddessfreya
Copy link
Contributor

@goddessfreya goddessfreya commented Aug 14, 2018

Adds a way to create opengl contexts from an already existing window.

This might result in suboptimal config for windows on X11, but it should still work.

I currently only plan to implement this functionality on linux and windows. Currently untested.

Implemented:

  • Linux - X11 - GLX
  • Linux - X11 - EGL
  • Linux - Wayland - EGL
  • Windows - EGL
  • Windows - HWND

Tested:

  • Linux - X11 - GLX
  • Linux - X11 - EGL
  • Linux - Wayland - EGL
  • Windows - EGL
  • Windows - HWND

Signed-off-by: Hal Gentz [email protected]

  • 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 an example program if it would help users understand this functionality

Fixes #1089, closes #1091.

@goddessfreya goddessfreya force-pushed the raw-contexts branch 2 times, most recently from c3bc9cd to 9532b8d Compare August 14, 2018 07:43
@kvark
Copy link
Contributor

kvark commented Aug 15, 2018

I assume this is a successor to #942 ?

@goddessfreya
Copy link
Contributor Author

goddessfreya commented Aug 15, 2018

Not really, unlike PureContext, we do make a surface. GlSeparatedContext is identical to a GlWindow, only difference is that the window is stored externally. (And it's window is always made first, something which is inconvenient on X11).

Edit:
The "successor" to PureContext would probably be the new HeadlessContext from this PR: #1029 (which may or may not make a surface)

@goddessfreya goddessfreya changed the title [WIP] Raw contexts [WIP] Raw contexts + context sharing support for windows and linux Sep 5, 2018
@goddessfreya goddessfreya force-pushed the raw-contexts branch 3 times, most recently from 0ce3893 to dcce7ab Compare September 16, 2018 02:38
@goddessfreya goddessfreya changed the title [WIP] Raw contexts + context sharing support for windows and linux Raw contexts + context sharing support for windows and linux Sep 16, 2018
@goddessfreya
Copy link
Contributor Author

r? @francesca64

Copy link
Member

@msiglreith msiglreith left a comment

Choose a reason for hiding this comment

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

just taking a brief look over the changes (only API and windows side)

really looking forward to these changes!

src/lib.rs Outdated
///
/// Errors should be very rare and only occur in case of permission denied,
/// incompatible system, out of memory, etc.
pub unsafe fn new_shared(
Copy link
Member

Choose a reason for hiding this comment

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

This function is only temporary until sharing is implemented for all APIs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, new_shared is am unsafe variant of new which allows sharing a context with an existing context. It was added because most people don't need context sharing and so would prefer to stay with the safe version.

match *ctxt {
Context::HiddenWindowWgl(_, ref c)
| Context::Wgl(ref c) => c.get_hglrc(),
_ => panic!(),
Copy link
Member

Choose a reason for hiding this comment

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

what's the reason for panic here? Or rather what sort of situation are we facing here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The context it's being shared with might have been made with EGL. Maybe? Honestly can't remember the reasoning which when behind it anymore.

When I think about it, I'm pretty sure I read somewhere that both shared contexts must have the same OpenGL version (at least on some platforms), which means the user should probably be required to pass us the same GlRequest, so the other context being made with EGL might be impossible.

Anyways, I don't currently have access to the old windows PC I had tested the windows changes on, so I don't want to randomly touch things without being able to test them.

On an unrelated note, the previous and my new version of the windows backend are totally borked, as far as I can tell, as they don't give a flying shit which version the user requested, and GlThenGles doesn't fallback. Does windows even support GLES?

}
_ => panic!(),
Copy link
Member

Choose a reason for hiding this comment

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

(see remark below)

Raw context support for both Wayland and X11.
Add sharing support to windows.

Signed-off-by: Hal Gentz <[email protected]>
@goddessfreya
Copy link
Contributor Author

I have resolved the merge conflicts.

@goddessfreya
Copy link
Contributor Author

r? @francesca64

@goddessfreya goddessfreya changed the title Raw contexts + context sharing support for windows and linux Raw contexts + context sharing support for windows and linux, and Unify Context new and new_shared functions, add ContextBuilder::buid method, and clarify docs Dec 28, 2018
@goddessfreya
Copy link
Contributor Author

Backported @Osspial's changes in #1092

Signed-off-by: Hal Gentz <[email protected]>
Signed-off-by: Hal Gentz <[email protected]>
Signed-off-by: Hal Gentz <[email protected]>
This reverts commit da247d3.
Signed-off-by: Hal Gentz <[email protected]>
Signed-off-by: Hal Gentz <[email protected]>
Signed-off-by: Hal Gentz <[email protected]>
Signed-off-by: Hal Gentz <[email protected]>
Signed-off-by: Hal Gentz <[email protected]>
Signed-off-by: Hal Gentz <[email protected]>
Signed-off-by: Hal Gentz <[email protected]>
@francesca64
Copy link
Member

So, before this sits here for a whole year... @zegentzy, would you like to maintain glutin? I'm positive you'd be much better at it than me (the iOS backend is the only one I actually know anything about, and I use Metal now anyway), and I'm extremely appreciative that you've been responding to people/etc.

@goddessfreya
Copy link
Contributor Author

@francesca64 I'd be happy to maintain it.

unlimitedbacon added a commit to unlimitedbacon/stl-thumb that referenced this pull request Feb 24, 2019
This reverts commit 391fba3.
The new headless API does not actually work in situations where
X11 is not available. Waiting on rust-windowing/glutin#1058
to make it work right.
Signed-off-by: Hal Gentz <[email protected]>
@francesca64
Copy link
Member

@zegentzy great! I sent @tomaka an email about it.

@kvark
Copy link
Contributor

kvark commented Feb 25, 2019

@zegentzy if you are about to become the maintainer (which is awesome news!), please pay more attention to the shape of the commits being merged:

  1. Every commit should be compilable on its own in most cases (with an exception of rare cases where the changes grow too big and there is no way to make them compilable before finishing).
  2. Every commit message should tell cleanly what is being done. E.g. messages like "Changes" and "Compile?" are not acceptable. It's fine to have those temporary when you fight CI, for example, but once figured out, you've got to clean those up before asking for a merge.
  3. No need for merge commits (e.g. "Merge branch 'master' into raw-contexts"). They pollute commit history and make bisection difficult since it's no longer linear.

@francesca64
Copy link
Member

@kvark "squash and merge" is the only option enabled on glutin, so 1 and 3 won't generally be issues. 2 is super important though.

@zegentzy enjoy your new powers! Please take good care of the CHANGELOG.

@kvark
Copy link
Contributor

kvark commented Feb 25, 2019

@francesca64 uh oh, that's quite a weird policy. Merging multi-thousand-LOC PRs just like that is quite destructive (to the history).

@francesca64
Copy link
Member

@kvark it's the case with winit as well. It seems to be what tomaka decided on, and is something I have no direct control over. I agree with you, but it hasn't been enough of an issue so far for me to raise the issue with him (...though for Event Loop 2.0 it will definitely be a problem).

@goddessfreya
Copy link
Contributor Author

#1101

@useronym
Copy link

useronym commented Apr 1, 2019

Did this make it into 0.20? I can't find any docs regarding the separated contexts, and there is no mention of them in the CHANGELOG either.

@goddessfreya
Copy link
Contributor Author

These changes did make it in, but separated contexts were decided against. Rationale was here: #1105 (comment)

For now, instead use the platform-specific RawContextExts:
https://docs.rs/glutin/0.20.0/glutin/os/unix/trait.RawContextExt.html
https://docs.rs/glutin/0.20.0/i686-pc-windows-msvc/glutin/os/windows/trait.RawContextExt.html

@goddessfreya
Copy link
Contributor Author

@Osense

@useronym
Copy link

useronym commented Apr 2, 2019

@zegentzy I see, thanks!

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.

Unnecessary API Restrictions on Context::new_shared ContextBuilder missing a build() method
8 participants