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

[wgpu-hal] Allow importing external WGL contexts as with EGL #6152

Merged
merged 1 commit into from
Sep 4, 2024

Conversation

MarijnS95
Copy link
Contributor

@MarijnS95 MarijnS95 commented Aug 24, 2024

Connections
Depends on #6150

Description
Allows importing an external WGL context into wgpu to run the raw-gles example.

Testing

cargo r --example raw-gles

Checklist

  • Run cargo fmt.
  • Run cargo clippy. If applicable, add:
    • --target wasm32-unknown-unknown
    • --target wasm32-unknown-emscripten
  • Run cargo xtask test to run tests.
  • Add change to CHANGELOG.md. See simple instructions inside file.

@MarijnS95
Copy link
Contributor Author

Still WIP because ad-hoc (un)currenting the imported context creates a mess - I have to check how EGL importing is implemented.

If only wgpu used glutin... 🤤

wgpu-hal/src/gles/wgl.rs Outdated Show resolved Hide resolved
@MarijnS95 MarijnS95 marked this pull request as ready for review August 27, 2024 22:36
@MarijnS95 MarijnS95 requested a review from a team as a code owner August 27, 2024 22:36
@MarijnS95
Copy link
Contributor Author

@ErichDonGubler can you review this one? I prefer to clean up the .take() before merging though.

MarijnS95 added a commit to MarijnS95/wgpu that referenced this pull request Aug 28, 2024
PR gfx-rs#6150 suffered a much larger rebase "hell" than I anticipated.  On
my Linux box I made this change, but lost it while force-pushing from
Windows (and created some other compiler errors while at it...).

By disabling all features on `glutin`/`glutin-winit` (the latter only
uses `x11`, and only forwards `wayland` to `glutin`) we may have dropped
a lot of "unused" dependencies for other GL backends, but also made the
crate unable to import X11 (Xlib/Xcb) and Wayland handles into EGL.

Also import the missing `glutin::context::Version` struct again which
was added last-minute to gfx-rs#6150 (to make sure my Intel card on Windows
creates a GLES 3.0+ instead of GLES 2.0 context) while the import was
accidentally squashed into gfx-rs#6152 (not merged yet).
teoxoy pushed a commit that referenced this pull request Aug 28, 2024
PR #6150 suffered a much larger rebase "hell" than I anticipated.  On
my Linux box I made this change, but lost it while force-pushing from
Windows (and created some other compiler errors while at it...).

By disabling all features on `glutin`/`glutin-winit` (the latter only
uses `x11`, and only forwards `wayland` to `glutin`) we may have dropped
a lot of "unused" dependencies for other GL backends, but also made the
crate unable to import X11 (Xlib/Xcb) and Wayland handles into EGL.

Also import the missing `glutin::context::Version` struct again which
was added last-minute to #6150 (to make sure my Intel card on Windows
creates a GLES 3.0+ instead of GLES 2.0 context) while the import was
accidentally squashed into #6152 (not merged yet).
@ErichDonGubler
Copy link
Member

@ErichDonGubler can you review this one? I prefer to clean up the .take() before merging though.

😬 Iiii don't consider myself really qualified to review GL-related code, since I'm thoroughly unfamiliar with GL abstractions, but I'm happy to give it a shot and disclaim what I don't know! I've migrated some winit and glutin code, but that was mostly a transformation of existing code.

In particular, I don't know much about what conditions GL contexts need to be made current, when they might not be current, and what is safe and unsafe to do when they're not current.

Will finalize review after that (#6152 (comment)), then. For now, I don't have any questions I expect will be useful to ask until more substantial review is satisfied.

wgpu-hal/src/gles/wgl.rs Show resolved Hide resolved
wgpu-hal/src/gles/egl.rs Outdated Show resolved Hide resolved
Copy link
Member

@ErichDonGubler ErichDonGubler left a comment

Choose a reason for hiding this comment

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

Since it seems we've already shipped unsafe APIs that conflict with the Soundness Property, I'm willing to defer reconciliation with it until after this PR.

makes my teeth chatter tho 😬

@MarijnS95
Copy link
Contributor Author

MarijnS95 commented Sep 4, 2024

makes my teeth chatter tho 😬

I was similarly frightened when opening this code for the first time, hence brought up glutin in Matrix. That doesn't solve this issue though, as it only concerns itself with the context API but not anything that's subsequently loaded/executed on it (which is deferred to glow or crates from gl-rs).

@ErichDonGubler ErichDonGubler enabled auto-merge (squash) September 4, 2024 14:59
@ErichDonGubler ErichDonGubler merged commit 960555a into gfx-rs:trunk Sep 4, 2024
25 checks passed
@MarijnS95 MarijnS95 deleted the wgl-external branch September 5, 2024 09:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants