Enable GL backend via Surfman #3375

zicklag · 2020-09-17T03:32:49Z

( Will, hopefully ) fix issue #3353

PR checklist:

make succeeds (on *nix)
make reftests succeeds
tested examples with the following backends:

kvark

Thank you for trying these ideas out!
Fortunately, I found a few places that would explain why it doesn't work yet :)

src/backend/gl/src/queue.rs

zicklag

Sweet. I'll come back and fix those probably later tonight. Thanks!

src/backend/gl/src/queue.rs

zicklag · 2020-09-18T02:17:33Z

I fixed the issues you pointed out and I'm still getting the jagged edges. 😕

Any other ideas. 🙂

kvark · 2020-09-18T02:47:35Z

src/backend/gl/src/queue.rs


+        // Wait for rendering to finish
+        unsafe {
+            let wait_result = gl.client_wait_sync(


this actually needs to be wait_sync, not client_wait_sync

Interesting, glow doesn't actually have a wait_sync. 😲

https://docs.rs/glow/0.6.0/glow/?search=wait_sync

Should I open an issue for glow on that? Maybe that is just a simple PR to glow I could make?

Yes, I recommend implementing it quickly in a fork, making a PR (it's a non-breaking change), and using your fork for the dependency here in the meantime.

OK, updated glow with the extra binding and submitted a PR and updated this branch with the changes, but still no change in rendering.

looks like this still needs to be done?

127: Add glWaitSync Binding r=grovesNL a=zicklag Needed this for `gfx-backend-gl`: gfx-rs/gfx#3375 (comment). Co-authored-by: Zicklag <[email protected]>

kvark · 2020-09-19T03:11:35Z

What if, after we render to the renderbuffer on the main context (in submit), we bind it for reading to a framebuffer, and actually issue a blit to some other FBO? Just to replicate the same scenario that we have on surface context (in present). This would ensure that the image goes through the necessary memory barriers and layout changes under the hood. Of course we don't want to ship that, but it would confirm the theory that there is a workaround, at least.

zicklag · 2020-09-19T15:33:57Z

What if, after we render to the renderbuffer on the main context (in submit), we bind it for reading to a framebuffer, and actually issue a blit to some other FBO?

I'll try that next, but I just found something interesting. I just made a new commit that gets rid of context sharing completely and I'm still getting the same rendering artifacts.

Also, I'm trying to figure out why we need separate contexts for the surface and the root context. Is that because in multi-threading you cannot use the same context across different threads? Being that we can't share anything across extra threads right now anyway because of Starc and such maybe we shouldn't even be sharing contexts at all?

Not that it is helping in this case it seems.

Edit: I'm not actually sure how to try what you suggested in submit because the framebuffer I would need to bind as the read FBO would be the swapchain FBO which we don't have access to in the submit function. Does the test I already did by getting rid of context sharing cover that test?

kvark · 2020-09-19T19:58:21Z

I just made a new commit that gets rid of context sharing completely and I'm still getting the same rendering artifacts.

That's an interesting experiment! We should double-check if the sharing is actually requested at the low level (by debugging through context creation).

I'm not actually sure how to try what you suggested in submit because the framebuffer I would need to bind as the read FBO would be the swapchain FBO which we don't have access to in the submit function. Does the test I already did by getting rid of context sharing cover that test?

Let's do it in present() then. To clarify: this operation is purely artificial and makes no practical sense, it's just to convince the driver to transition the renderbuffer into "readable" state. So, in present() before switching the current context, you could create an FBO and attach the renderbuffer to it, bind the FBO for reading. Then maybe create another temporary RBO, another temporary FBO, attach them, and bind from drawing. Then do a blit.

After that, we can proceed as usual by switch the contexts and doing a real blit.

kvark · 2020-09-23T23:11:44Z

@zicklag have you tried to make further progress here?

zicklag · 2020-09-24T00:13:27Z

Haven't had the chance to try anything else yet.

kvark · 2020-11-01T02:30:05Z

What platform are you on? Can you reproduce the rendering issue on the quad example?

kvark · 2020-11-01T02:53:02Z

I tested this PR without the last commit on both macOS and Linux with a modified quad example and got no issues...

zicklag · 2020-11-01T14:07:14Z

I'm on Linux, but the quad example did work before this PR: only the WGPU Cube example actually exhibits the problem so far.

For now, because of other priorities this is on the back burner for me probably for a while depending on how things go. Especially with Bevy getting an OpenGL renderer backend which would have been one of my larger use-cases. I'd still love for this to work! I just probably won't have time to spend on it for a while.

kvark · 2020-11-01T17:38:20Z

I think the difference with the cube example is that the latter has a second pass, where it draws edges only, and enables blending. So this is what confusing the driver in our case, and it would be trivial to replicate on gfx's quad example.

Especially with Bevy getting an OpenGL renderer backend which would have been one of my larger use-cases

I'd expect them to happily throw it out if gfx-rs had a working GL backend.
There is also Veloren, which is essentially blocked for wgpu migration on our GL backend.

zicklag · 2020-11-01T17:50:03Z

I think the difference with the cube example is that the latter has a second pass, where it draws edges only, and enables blending. So this is what confusing the driver in our case, and it would be trivial to replicate on gfx's quad example.

Hmm, that would be good to do.

I'd expect them to happily throw it out if gfx-rs had a working GL backend.
There is also Veloren, which is essentially blocked for wgpu migration on our GL backend.

Ahgh, I know. You're going to convince me to keep trying this when I have time. LOL 😃

OK, well I'll try to get back to this eventually, then. Right now all my time is going to getting out an 0.2 release of my Arsenal game engine ( which is built on Bevy ). GL support is definitely something we really want for Arsenal so this will probably take priority at some point, but the first goal is for me to get Arsenal working at all.

kvark · 2020-11-01T19:31:00Z

Sorry, didn't mean to try convincing you. Looks like you got enough on your plate at the moment :) Just wanted to argue/elaborate on why GL backend would still be a strategic goal for us, even if Bevy doesn't use it.

zicklag · 2020-11-01T19:33:00Z

Yeah, no problem man. :D I really do want to get this figured out. We'll figure out it sometime. 👍 It would be amazing to support all of the graphics APIs through one interface.

kvark · 2020-11-02T02:59:05Z

Bummer. I added wireframe rendering to the quad example, and it still doesn't exhibit the issue.

kvark · 2020-11-02T03:41:12Z

Do you see artifacts in the quad example with https://github.com/kvark/gfx/tree/gl-test ?
I tried running wgpu's examples on GL with the fixes of your PRs (except for the last commit in this PR, which is temporary), and it's all fine on Linux. Going to do the same on macOS now...
Would be good to resurrect and land this, at least on master. Are you interested to do this? Otherwise, I can pick it up.

zicklag · 2020-11-02T03:57:05Z

Do you see artifacts in the quad example with https://github.com/kvark/gfx/tree/gl-test ?

Just ran it and it looks fine.

I tried running wgpu's examples on GL with the fixes of your PRs (except for the last commit in this PR, which is temporary), and it's all fine on Linux. Going to do the same on macOS now...

Oh, interesting. I'll try it again.

Would be good to resurrect and land this, at least on master. Are you interested to do this? Otherwise, I can pick it up.

I should be able to get it rebased. I'll see if I can do that now, I've got ~20 mins. It shouldn't be too hard.

src/backend/gl/Cargo.toml

src/backend/gl/src/queue.rs

kvark · 2020-11-02T04:05:52Z

src/backend/gl/src/queue.rs


+        // Wait for rendering to finish
+        unsafe {
+            let wait_result = gl.client_wait_sync(


looks like this still needs to be done?

kvark · 2020-11-02T04:07:44Z

Unable to test the wgpu's cube example on macOS, getting

thread 'main' panicked at 'Error InvalidOperation executing command: CopyBufferToBuffer(2, 1, BufferCopy { src: 0, dst: 0, size: 580 })', /Users/dmalyshau/Code/gfx/src/backend/gl/src/queue.rs:1184:13
note: run with RUST_BACKTRACE=1 environment variable to display a backtrace

This is unrelated to your PR but unfortunate. Will look into it, not a blocker.

src/backend/dx12/src/command.rs

kvark

Alright, looks like everything is in place!
bors r+

zicklag · 2020-11-02T04:13:32Z

looks like this still needs to be done?

Sorry, it won't let me reply to our comment inline. What still needs to be done?

3375: [ Failed ( so far ) ] Attempt to Fix Rendering Artifacts r=kvark a=zicklag ( Will, hopefully ) fix issue #3353 PR checklist: - [ ] `make` succeeds (on *nix) - [ ] `make reftests` succeeds - [ ] tested examples with the following backends: Co-authored-by: Zicklag <[email protected]>

kvark · 2020-11-02T04:14:17Z

looks like this still needs to be done?

Sorry, it won't let me reply to our comment inline. What still needs to be done?

It was using client_wait_sync instead of wait_sync, but it looks like you fixed this now 👍

kvark · 2020-11-02T04:15:31Z

Could you make a similar PR towards hal-0.6 branch?
I'd be comfortable publishing GL backend with this. Remember that we didn't publish it since hal-0.6 got out. Even if it's partially broken, still more useful than no backend at all.

bors · 2020-11-02T04:17:01Z

Build failed:

Windows Nightly

zicklag · 2020-11-02T04:17:33Z

It was using client_wait_sync instead of wait_sync, but it looks like you fixed this now +1

Ah, cool.

Alright, looks like everything is in place!

Sweet! 😃

I haven't tested the WGPU examples yet, but I'll try those out in a sec ( if there's no major conflicts to resolve ). If those don't work for me, it might just be that my driver's goofy. Also it's running the Mesa driver, but I've got a hybrid GPU setup on my machine, so I can choose to run it with the GPU which I didn't realize earlier so I'm not sure if that is the reason for the weirdness. I'll have to try both.

Could you make a similar PR towards hal-0.6 branch?
I'd be comfortable publishing GL backend with this. Remember that we didn't publish it since hal-0.6 got out. Even if it's partially > broken, still more useful than no backend at all.

Sounds good. I'll do that. 👍

zicklag · 2020-11-02T04:18:58Z

Oh, yeah, I forgot about that extra framebuffer was only added for surfman. I'll have to gate the code off for the windows version and do that differently I guess.

kvark · 2020-11-02T04:21:23Z

But we don't need Surfman (and GL backend) on windows at all, since it's not able to support WGL in the long run anyway

zicklag · 2020-11-02T04:22:29Z

Ah, then no reason to pollute the code with extra #[cfg] checks. What do we want to do here? Just disable CI for GL on windows?

kvark · 2020-11-02T04:23:23Z

Yes, rip out GL on windows completely :)

zicklag · 2020-11-02T04:25:52Z

There that should make CI pass now.

kvark · 2020-11-02T04:27:13Z

I was worried that the GL error I'm seeing on macOS is really about something broken with the presentation, and just manifesting itself on the next command. But it turns out to be a genuine error of buffer copying. Not sure what's going on there, but we can proceed.
bors r+

zicklag · 2020-11-02T04:29:12Z

Just about out of time now, I'll try to do the hal-0.6 PR and a test of the WGPU examples tomorrow! 🙂

kvark · 2020-11-02T04:33:51Z

Thank you! I'll do some more cleanup of the GL backend in master in the meantime.

bors · 2020-11-02T04:37:44Z

Build succeeded:

monocodus bot approved these changes Sep 17, 2020

View reviewed changes

zicklag mentioned this pull request Sep 17, 2020

WGPU GL Rendering Artifacts #3353

Open

kvark requested changes Sep 17, 2020

View reviewed changes

src/backend/gl/src/queue.rs Outdated Show resolved Hide resolved

src/backend/gl/src/queue.rs Show resolved Hide resolved

src/backend/gl/src/queue.rs Outdated Show resolved Hide resolved

zicklag commented Sep 17, 2020

View reviewed changes

src/backend/gl/src/queue.rs Show resolved Hide resolved

src/backend/gl/src/queue.rs Outdated Show resolved Hide resolved

src/backend/gl/src/queue.rs Outdated Show resolved Hide resolved

zicklag force-pushed the gl-final-showdown branch from 02e90d1 to 6f77195 Compare September 18, 2020 02:15

kvark reviewed Sep 18, 2020

View reviewed changes

zicklag mentioned this pull request Sep 19, 2020

Add glWaitSync Binding grovesNL/glow#127

Merged

bors bot added a commit to grovesNL/glow that referenced this pull request Sep 19, 2020

Merge #127

b31cce3

127: Add glWaitSync Binding r=grovesNL a=zicklag Needed this for `gfx-backend-gl`: gfx-rs/gfx#3375 (comment). Co-authored-by: Zicklag <[email protected]>

zicklag force-pushed the gl-final-showdown branch from 1ebbe01 to c077a2f Compare September 19, 2020 15:30

zicklag force-pushed the gl-final-showdown branch from c077a2f to bf32c5e Compare November 2, 2020 03:58

kvark reviewed Nov 2, 2020

View reviewed changes

src/backend/gl/Cargo.toml Outdated Show resolved Hide resolved

zicklag force-pushed the gl-final-showdown branch 3 times, most recently from 8845f60 to 98c6da9 Compare November 2, 2020 04:04

kvark reviewed Nov 2, 2020

View reviewed changes

zicklag force-pushed the gl-final-showdown branch from 98c6da9 to ca21330 Compare November 2, 2020 04:10

Attempt to Fix GL Rendering Artifacts

75658d8

zicklag force-pushed the gl-final-showdown branch from ca21330 to 75658d8 Compare November 2, 2020 04:11

kvark reviewed Nov 2, 2020

View reviewed changes

src/backend/dx12/src/command.rs Show resolved Hide resolved

kvark approved these changes Nov 2, 2020

View reviewed changes

kvark changed the title ~~[ Failed ( so far ) ] Attempt to Fix Rendering Artifacts~~ Enable GL backend via Surfman Nov 2, 2020

Exclude GL Backnd From CI on Windows

a2bdf29

bors bot merged commit 26ee948 into gfx-rs:master Nov 2, 2020

This was referenced Nov 6, 2020

Idiomatic OpenGL context directly on EGL #3468

Closed

OpenGL surface via surfman #3133

Closed

zicklag deleted the gl-final-showdown branch November 6, 2020 15:25

Enable GL backend via Surfman #3375

Enable GL backend via Surfman #3375

Uh oh!

Conversation

zicklag commented Sep 17, 2020

Uh oh!

kvark left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zicklag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zicklag commented Sep 18, 2020

Uh oh!

kvark Sep 18, 2020

Choose a reason for hiding this comment

Uh oh!

zicklag Sep 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kvark Sep 18, 2020

Choose a reason for hiding this comment

Uh oh!

zicklag Sep 19, 2020

Choose a reason for hiding this comment

Uh oh!

kvark Nov 2, 2020

Choose a reason for hiding this comment

Uh oh!

kvark commented Sep 19, 2020

Uh oh!

zicklag commented Sep 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kvark commented Sep 19, 2020

Uh oh!

kvark commented Sep 23, 2020

Uh oh!

zicklag commented Sep 24, 2020

Uh oh!

kvark commented Nov 1, 2020

Uh oh!

kvark commented Nov 1, 2020

Uh oh!

zicklag commented Nov 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kvark commented Nov 1, 2020

Uh oh!

zicklag commented Nov 1, 2020

Uh oh!

kvark commented Nov 1, 2020

Uh oh!

zicklag commented Nov 1, 2020

Uh oh!

kvark commented Nov 2, 2020

Uh oh!

kvark commented Nov 2, 2020

Uh oh!

zicklag commented Nov 2, 2020

Uh oh!

Uh oh!

Uh oh!

kvark Nov 2, 2020

Choose a reason for hiding this comment

Uh oh!

kvark commented Nov 2, 2020

Uh oh!

Uh oh!

kvark left a comment

Choose a reason for hiding this comment

Uh oh!

zicklag commented Nov 2, 2020

zicklag Sep 18, 2020 •

edited

Loading

zicklag commented Sep 19, 2020 •

edited

Loading

zicklag commented Nov 1, 2020 •

edited

Loading