Skip to content

GL Final Showdown#3340

Merged
bors[bot] merged 1 commit intogfx-rs:masterfrom
katharostech:gl-final-showdown
Aug 27, 2020
Merged

GL Final Showdown#3340
bors[bot] merged 1 commit intogfx-rs:masterfrom
katharostech:gl-final-showdown

Conversation

@zicklag
Copy link
Copy Markdown
Contributor

@zicklag zicklag commented Aug 17, 2020

WIP getting GL finally working again with surfman.

PR checklist:

  • make succeeds (on *nix)
  • make reftests succeeds ( Fails because of things unimplemented in GL backend. Out of scope for this PR )
  • tested examples with the following backends:
    • vulkan
    • gl

@zicklag
Copy link
Copy Markdown
Contributor Author

zicklag commented Aug 17, 2020

@kvark I've got a quick question about EGL, which is used by surfman for X11. I'm using a container for my dev environment because I'm not root on my build machine, and the surfman examples run fine inside the container where I've got all kinds of dev libraries installed and such. Outside of the container, though, the examples bomb out when creating the surfman X11 connection with an error: egl function was not loaded.

Do you know what needs to be installed on the host to get EGL to load?

@kvark
Copy link
Copy Markdown
Member

kvark commented Aug 17, 2020

Probably libegl1-mesa package?

@zicklag
Copy link
Copy Markdown
Contributor Author

zicklag commented Aug 18, 2020

Strangely, I have that and it still won't work anyway. Oh, well, thanks for the hint. I'm not going to worry too much about it . I just needed it for the debugger to work, but I'll be fine without it for now.


OK, so I've gotten your changes to surfman pulled locally and I've gotten all setup to work on the gfx-backend-gl. I just wanted to make sure that this sounds right on how we organize the sharing between contexts because I can't remember exactly how we were going to put it together:

  • When you create the Instance ( currently actually when we enumerate adapters ) we will want to create essentially a "root" context.
  • The root context is the one that we create here. That is the context that we use to create the gl container with the from_fn_proc function.
  • When surfaces are created for that Instance, each surface will create its own context that will have the "root" context in the share_with of its ContextAttributes.

Is that along the right lines?

@kvark kvark force-pushed the master branch 3 times, most recently from 3e1f8b1 to 375af89 Compare August 19, 2020 14:50
@kvark
Copy link
Copy Markdown
Member

kvark commented Aug 20, 2020

OK, so I've gotten your changes to surfman pulled locally

My changes were merged into surfman proper in servo/surfman#185, you shouldn't need my branch, just use master.

Is that along the right lines?

Correct!

Also, the adapters may need to create their own GL contexts, depending on exposed/requested features. If you expose only a single adapter, you can just re-use the context from the instance, that's simpler.

@zicklag
Copy link
Copy Markdown
Contributor Author

zicklag commented Aug 26, 2020

Haha! Finally got this working I think!

image

I haven't switched to using the official servo/surfman yet. I'm using a fork of @kvark's old fork 'cause I got stuck without internet for a few days and it was what I had on my machine. 😉

I also started work on porting WGPU to the latest GFX and including support for OpenGL and it is going very well so far. Hope to have this finished soon!

Copy link
Copy Markdown
Member

@kvark kvark left a comment

Choose a reason for hiding this comment

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

Great work here!

Comment thread src/backend/gl/src/window/surfman.rs
Comment thread src/backend/gl/src/window/surfman.rs Outdated
Comment thread src/backend/gl/src/window/surfman.rs Outdated
@zicklag zicklag force-pushed the gl-final-showdown branch from 7b6dd03 to 74d9891 Compare August 26, 2020 21:17
@zicklag zicklag marked this pull request as ready for review August 26, 2020 21:19
@zicklag zicklag requested a review from kvark August 26, 2020 21:20
@zicklag
Copy link
Copy Markdown
Contributor Author

zicklag commented Aug 26, 2020

OK, I think this is good now. ( Pending CI ).

Copy link
Copy Markdown
Member

@kvark kvark left a comment

Choose a reason for hiding this comment

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

Looks like we are almost there!

Comment thread src/backend/gl/Cargo.toml Outdated
Comment thread src/backend/gl/Cargo.toml Outdated
Comment thread src/backend/gl/src/lib.rs Outdated
Comment thread src/backend/gl/src/window/surfman.rs Outdated
@kvark
Copy link
Copy Markdown
Member

kvark commented Aug 26, 2020

Seems to be working on macOS. Although, we'd need to test the wgpu-rs samples before publishing the gfx backend.

@zicklag
Copy link
Copy Markdown
Contributor Author

zicklag commented Aug 26, 2020

Seems to be working on macOS.

Oh, sweet.

Although, we'd need to test the wgpu-rs samples before publishing the gfx backend.

Yeah, sounds like a good idea. That was the issue last time: we didn't find the issues with the backend until we tried hooking it up to WGPU.

@zicklag zicklag force-pushed the gl-final-showdown branch from 74d9891 to 7ce64ca Compare August 26, 2020 22:32
@kvark
Copy link
Copy Markdown
Member

kvark commented Aug 26, 2020

Nice to have a near-instant CI now, isn't it? :)

@zicklag
Copy link
Copy Markdown
Contributor Author

zicklag commented Aug 26, 2020

Uh, yeah, that's pretty nice. :)

Copy link
Copy Markdown
Member

@kvark kvark left a comment

Choose a reason for hiding this comment

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

Awesome, one last minor thing!

Comment thread examples/Cargo.toml
@zicklag zicklag force-pushed the gl-final-showdown branch from 7ce64ca to 31f61a5 Compare August 26, 2020 22:41
@zicklag
Copy link
Copy Markdown
Contributor Author

zicklag commented Aug 26, 2020

If I've estimated correctly I should be able to test this with WGPU very soon. I'll have a PR on WGPU and we'll figure out how well this works with the WGPU examples.

@kvark
Copy link
Copy Markdown
Member

kvark commented Aug 26, 2020

Please rebase and squash

@zicklag zicklag force-pushed the gl-final-showdown branch from 31f61a5 to 8f9fcac Compare August 26, 2020 23:22
@zicklag
Copy link
Copy Markdown
Contributor Author

zicklag commented Aug 26, 2020

Done. And, hey, we deleted more lines of code than we added. 🎉

@zicklag
Copy link
Copy Markdown
Contributor Author

zicklag commented Aug 26, 2020

Ah, oops, I made the GL backend only included in the examples for cfg(unix) platforms and now CI is failing for windows. Do we want to completely disable OpenGL for windows, or do we leave it? I don't think I broke GL on windows with the surfman stuff, but I never tested it before or after so I don't know if it's working.

@kvark
Copy link
Copy Markdown
Member

kvark commented Aug 26, 2020

Let's disable it on Windows completely

@kvark
Copy link
Copy Markdown
Member

kvark commented Aug 26, 2020

that also means we don't need to enable WGL dependency

@zicklag zicklag force-pushed the gl-final-showdown branch from 8f9fcac to 68658df Compare August 26, 2020 23:46
Fix the OpenGL backend for Linux and MacOS, utilizing the new context
sharing in surfman 0.3.
@zicklag zicklag force-pushed the gl-final-showdown branch from 68658df to a1bf8a2 Compare August 26, 2020 23:59
Copy link
Copy Markdown
Member

@kvark kvark left a comment

Choose a reason for hiding this comment

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

Beautiful!
Let's get this into master.
bors r+

@bors
Copy link
Copy Markdown
Contributor

bors bot commented Aug 27, 2020

@bors bors bot merged commit c9fcd08 into gfx-rs:master Aug 27, 2020
@zicklag zicklag deleted the gl-final-showdown branch August 27, 2020 00:37
@zicklag
Copy link
Copy Markdown
Contributor Author

zicklag commented Aug 27, 2020

Yay! 🎉 Thanks so much for your help @kvark. Couldn't have done it without your guidance. 😄

Next up, WGPU!

bors bot added a commit to gfx-rs/wgpu that referenced this pull request Nov 5, 2020
907: Implement OpenGL Backend For Unix Platforms r=kvark a=zicklag

**Connections**
Requires: gfx-rs/gfx#3340 ( merged )
Should be merged first: #910
Works towards: #450

**Description**
Integrates the GFX GL backend for Unix platforms

**Testing**
Runs the basic cube and triangle examples  for wgpu-rs, but not without rendering artifacts on the cube.
<!--
Non-trivial functional changes would need to be tested through:
  - [wgpu-rs](https://github.com/gfx-rs/wgpu-rs) - test the examples.
  - [wgpu-native](https://github.com/gfx-rs/wgpu-native/) - check the generated C header for sanity.

Ideally, a PR needs to link to the draft PRs in these projects with relevant modifications.
See #666 for an example.
If you can add a unit/integration test here in `wgpu`, that would be best.
-->


Co-authored-by: Zicklag <zicklag@katharostech.com>
bors bot added a commit to gfx-rs/wgpu that referenced this pull request Nov 5, 2020
907: Implement OpenGL Backend For Unix Platforms r=kvark a=zicklag

**Connections**
Requires: gfx-rs/gfx#3340 ( merged )
Should be merged first: #910
Works towards: #450

**Description**
Integrates the GFX GL backend for Unix platforms

**Testing**
Runs the basic cube and triangle examples  for wgpu-rs, but not without rendering artifacts on the cube.
<!--
Non-trivial functional changes would need to be tested through:
  - [wgpu-rs](https://github.com/gfx-rs/wgpu-rs) - test the examples.
  - [wgpu-native](https://github.com/gfx-rs/wgpu-native/) - check the generated C header for sanity.

Ideally, a PR needs to link to the draft PRs in these projects with relevant modifications.
See #666 for an example.
If you can add a unit/integration test here in `wgpu`, that would be best.
-->


Co-authored-by: Zicklag <zicklag@katharostech.com>
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.

2 participants