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

Implement OpenGL Backend For Unix Platforms #907

Merged
merged 3 commits into from
Nov 5, 2020

Conversation

zicklag
Copy link
Contributor

@zicklag zicklag commented Aug 29, 2020

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.

Copy link
Contributor

@monocodus monocodus bot left a comment

Choose a reason for hiding this comment

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

This is an autogenerated code review.New suggestions: 8

wgpu-core/build.rs Outdated Show resolved Hide resolved
wgpu-core/build.rs Outdated Show resolved Hide resolved
wgpu-core/src/hub.rs Outdated Show resolved Hide resolved
wgpu-core/src/hub.rs Outdated Show resolved Hide resolved
wgpu-core/src/hub.rs Outdated Show resolved Hide resolved
wgpu-core/src/hub.rs Outdated Show resolved Hide resolved
wgpu-core/src/hub.rs Outdated Show resolved Hide resolved
wgpu-core/src/swap_chain.rs Outdated Show resolved Hide resolved
@zicklag zicklag mentioned this pull request Aug 29, 2020
@zicklag zicklag force-pushed the gl-final-showdown branch 6 times, most recently from fa65a99 to aef60ca Compare August 30, 2020 00:10
@zicklag zicklag marked this pull request as ready for review August 30, 2020 00:14
@zicklag
Copy link
Contributor Author

zicklag commented Aug 30, 2020

OK, I haven't nailed down the dependency versions yet, but the rest of the code should be ready for review.

The most controversial organizational change that I've included in this ( sorry, maybe should have been a separate PR ) was I used the cfg_aliases crate to replace the backends! macro throughout the code. I think it is much more readable to use plain Rust cfg checks instead of a macro in all of the cases that we previously used backends! and now you can use the same cfg syntax anywhere that the backends! macro could not handle such as in struct field definitions.

@kvark
Copy link
Member

kvark commented Aug 30, 2020

Note that wgpu always works with released gfx-rs versions. So we can't merge this until we get something usable in gfx-rs 0.6 branch and publish the GL backend.

Copy link
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.

I agree the configuration change should land independently prior to GL stuff

wgpu-core/build.rs Show resolved Hide resolved
wgpu-core/build.rs Outdated Show resolved Hide resolved
@zicklag
Copy link
Contributor Author

zicklag commented Aug 30, 2020

I agree the configuration change should land independently prior to GL stuff

OK, I'll extract it out to a separate PR that should be merged before this one.

@zicklag
Copy link
Contributor Author

zicklag commented Aug 30, 2020

Note that wgpu always works with released gfx-rs versions. So we can't merge this until we get something usable in gfx-rs 0.6 branch and publish the GL backend.

Yeah, I figured. I'll put the GFX git commit hashes in there to fix the deps just so we can verify that CI will pass.

This was referenced Aug 30, 2020
@zicklag zicklag force-pushed the gl-final-showdown branch 3 times, most recently from 0c0899e to 351babf Compare August 30, 2020 16:01
@zicklag
Copy link
Contributor Author

zicklag commented Aug 30, 2020

OK, the two commits have been separated. Once #910 is merged this PR will only have the second commit in it.

After separating the commits you can see that this was actually really simple from the WGPU side thanks to the abstraction provided by GFX, which is really awesome.

@zicklag zicklag force-pushed the gl-final-showdown branch 2 times, most recently from 719d79e to 73d0b0b Compare August 30, 2020 17:48
@zicklag
Copy link
Contributor Author

zicklag commented Aug 30, 2020

Just found that the port to the newer version of GFX broke the Vulkan backend on Linux at least. I'm working on fixing that. It's some panic on the unwrap for get_surface_mut.

Edit: Figured it out: I had left out the x11 feature on the Vulkan backend.

@zicklag zicklag force-pushed the gl-final-showdown branch 2 times, most recently from 1ade7a9 to b4ea52c Compare August 30, 2020 22:01
Copy link
Contributor Author

@zicklag zicklag left a comment

Choose a reason for hiding this comment

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

Couple of implementation questions.

wgpu-core/Cargo.toml Outdated Show resolved Hide resolved
wgpu-core/src/instance.rs Outdated Show resolved Hide resolved
bors bot added a commit that referenced this pull request Aug 31, 2020
910: Replace backends! Macro with CFG Aliases r=kvark a=zicklag

**Connections**
Needs to be merged before: #907

**Description**
This change makes it easier to conditionally compile code based on graphics backends by adding `#[cfg]` aliases for the backends such as `vulkan`, `metal`, etc. This makes the code easier to read and maintain.

**Testing**
Tested the WGPU-rs cube and boids examples and they work as normal on a Linux machine with Vulkan.

Co-authored-by: Zicklag <zicklag@katharostech.com>
@kvark
Copy link
Member

kvark commented Nov 3, 2020

gfx-rs GL backend is now published. Time to revive this!

@zicklag
Copy link
Contributor Author

zicklag commented Nov 3, 2020

Yay! I'll see about getting this back up to date tomorrow probably. 😃

@zicklag
Copy link
Contributor Author

zicklag commented Nov 3, 2020

There you go! It looks like surfman is failing to compile on iOS, unfortunately. Not sure why we hadn't hit that until now.

Also the cube example doesn't run:

thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', /home/zicklag/.cargo/registry/src/github.com-1ecc6299db9ec823/gfx-backend-gl-0.6.0/src/command.rs:1174:30
stack backtrace:
   0: rust_begin_unwind
             at /rustc/ffa2e7ae8fbf9badc035740db949b9dae271c29f/library/std/src/panicking.rs:483:5
   1: core::panicking::panic_fmt
             at /rustc/ffa2e7ae8fbf9badc035740db949b9dae271c29f/library/core/src/panicking.rs:85:14
   2: core::panicking::panic
             at /rustc/ffa2e7ae8fbf9badc035740db949b9dae271c29f/library/core/src/panicking.rs:50:5
   3: core::option::Option<T>::unwrap
             at /home/zicklag/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/option.rs:386:21
   4: <gfx_backend_gl::command::CommandBuffer as gfx_hal::command::CommandBuffer<gfx_backend_gl::Backend>>::bind_graphics_descriptor_sets
             at /home/zicklag/.cargo/registry/src/github.com-1ecc6299db9ec823/gfx-backend-gl-0.6.0/src/command.rs:1172:40
   5: wgpu_core::command::render::<impl wgpu_core::hub::Global<G>>::command_encoder_run_render_pass_impl
             at /home/zicklag/git/zicklag/wgpu-workspace/wgpu/wgpu-core/src/command/render.rs:1010:29
   6: wgpu_core::command::render::<impl wgpu_core::hub::Global<G>>::command_encoder_run_render_pass
             at /home/zicklag/git/zicklag/wgpu-workspace/wgpu/wgpu-core/src/command/render.rs:414:9
   7: <wgpu::backend::direct::Context as wgpu::Context>::command_encoder_end_render_pass
             at ./src/backend/direct.rs:1417:9
   8: <wgpu::RenderPass as core::ops::drop::Drop>::drop
             at ./src/lib.rs:2340:13
   9: core::ptr::drop_in_place
             at /home/zicklag/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ptr/mod.rs:175:1
  10: <cube::Example as cube::framework::Example>::render
             at ./examples/cube/main.rs:412:9
  11: cube::framework::start::{{closure}}
             at ./examples/cube/../framework.rs:297:17
  12: winit::platform_impl::platform::sticky_exit_callback
             at /home/zicklag/.cargo/registry/src/github.com-1ecc6299db9ec823/winit-0.22.2/src/platform_impl/linux/mod.rs:698:5
  13: winit::platform_impl::platform::x11::EventLoop<T>::run_return
             at /home/zicklag/.cargo/registry/src/github.com-1ecc6299db9ec823/winit-0.22.2/src/platform_impl/linux/x11/mod.rs:312:21
  14: winit::platform_impl::platform::x11::EventLoop<T>::run
             at /home/zicklag/.cargo/registry/src/github.com-1ecc6299db9ec823/winit-0.22.2/src/platform_impl/linux/x11/mod.rs:390:9
  15: winit::platform_impl::platform::EventLoop<T>::run
             at /home/zicklag/.cargo/registry/src/github.com-1ecc6299db9ec823/winit-0.22.2/src/platform_impl/linux/mod.rs:645:35
  16: winit::event_loop::EventLoop<T>::run
             at /home/zicklag/.cargo/registry/src/github.com-1ecc6299db9ec823/winit-0.22.2/src/event_loop.rs:149:9
  17: cube::framework::start
             at ./examples/cube/../framework.rs:230:5
  18: cube::framework::run
             at ./examples/cube/../framework.rs:307:5
  19: cube::main
             at ./examples/cube/main.rs:419:5
  20: core::ops::function::FnOnce::call_once
             at /home/zicklag/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:227:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
[0.222763 ERROR]()(no module): Heaps not completely freed before drop. Utilization: MemoryHeapUtilization { utilization: MemoryUtilization { used: 917504, effective: 263424 }, size: 18446744073709551615 }
[0.222821 ERROR]()(no module): Heaps not completely freed before drop. Utilization: MemoryHeapUtilization { utilization: MemoryUtilization { used: 16777216, effective: 262996 }, size: 18446744073709551615 }
[0.222881 ERROR]()(no module): Not all allocations from LinearAllocator were freed
[0.222901 ERROR]()(no module): Memory leak: SizeEntry(768) is still used
[0.222924 ERROR]()(no module): Memory leak: SizeEntry(6144) is still used
[0.222942 ERROR]()(no module): Memory leak: SizeEntry(49152) is still used
[0.222959 ERROR]()(no module): Memory leak: SizeEntry(256) is still used
[0.222992 ERROR]()(no module): Memory leak: SizeEntry(262144) is still used
[0.223013 ERROR]()(no module): Memory leak: SizeEntry(65536) is still used
[0.223039 ERROR]()(no module): DescriptorAllocator is dropped

Is this potentially related to your recent image changes @kvark ? The triangle example works, though.

Otherwise the changes should pretty much be in order I think.

@kvark kvark changed the title WIP Implement OpenGL Backend For Unix Platforms Implement OpenGL Backend For Unix Platforms Nov 4, 2020
Copy link
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.

Thank you for the amazing work!
A few small things here

wgpu-core/src/instance.rs Outdated Show resolved Hide resolved
wgpu-core/src/lib.rs Outdated Show resolved Hide resolved
@kvark
Copy link
Member

kvark commented Nov 4, 2020

Oh, for iOS - we shouldn't be building OpenGL for iOS at all. So if it falls under unix, we should change build.rs to make it so it's unix but not iOS.

@zicklag
Copy link
Contributor Author

zicklag commented Nov 4, 2020

Oh, for iOS - we shouldn't be building OpenGL for iOS at all. So if it falls under unix, we should change build.rs to make it so it's unix but not iOS.

Oh, yeah, of course. Easy fix.

@zicklag
Copy link
Contributor Author

zicklag commented Nov 5, 2020

OK, now I'm a little confused why surfman is being pulled in at all. The only thing that has a dependency on surfman should be gfx-backend-gl and is already excluding the surfman dep on iOS:

[target.'cfg(all(unix, not(target_os = "ios")))'.dependencies]
surfman = { version = "0.3", features = ["sm-raw-window-handle"], optional = true }

Could clippy be being weird about this and including the dependency when it isn't supposed to?

@kvark
Copy link
Member

kvark commented Nov 5, 2020

@zicklag you are looking at the master branch for gfx but using the 0.6 released version, which is a bit different:

[target.'cfg(not(target_arch = "wasm32"))'.dependencies]
surfman = { version = "0.3", features = ["sm-raw-window-handle"] }

Really, we shouldn't even try to build the GL backend on iOS, so this line needs to be changed:

[target.'cfg(unix)'.dependencies]
gfx-backend-gl = { version = "0.6" }

@zicklag
Copy link
Contributor Author

zicklag commented Nov 5, 2020

Oh, yes, that's right. Thank you. I don't know how I missed that. 😆 Fixed it ( assuming CI passes, now ).

@kvark
Copy link
Member

kvark commented Nov 5, 2020

bors r+

bors bot added a commit 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>
@kvark
Copy link
Member

kvark commented Nov 5, 2020

Thank you for all the amazing work!

@bors
Copy link
Contributor

bors bot commented Nov 5, 2020

Build failed:

@zicklag
Copy link
Contributor Author

zicklag commented Nov 5, 2020

Glad I could help!

Unfortunately it looks like we've got to wait for Canonical to fix their servers or something. The apt repos are failing on CI everywhere at the moment.

@kvark
Copy link
Member

kvark commented Nov 5, 2020

bors retry

@bors
Copy link
Contributor

bors bot commented Nov 5, 2020

Build succeeded:

@bors bors bot merged commit 99ff41b into gfx-rs:master Nov 5, 2020
bors bot added a commit to gfx-rs/wgpu-rs that referenced this pull request Nov 5, 2020
606: Enable GL backend r=kvark a=kvark

Replaces #182
Depends on gfx-rs/wgpu#907

Co-authored-by: Dzmitry Malyshau <kvarkus@gmail.com>
VincentJousse pushed a commit to VincentJousse/wgpu that referenced this pull request Dec 18, 2020
bors bot added a commit that referenced this pull request Dec 18, 2020
1097: Fix bad copy/paste in #907 r=grovesNL a=VincentFTS

**Connections**
#907

**Description**
Fix a copy/paste from dx11 code.



Co-authored-by: Vincent Jousse <contact@ftsoftware.fr>
kvark added a commit to kvark/wgpu that referenced this pull request Jun 3, 2021
606: Enable GL backend r=kvark a=kvark

Replaces gfx-rs#182
Depends on gfx-rs#907

Co-authored-by: Dzmitry Malyshau <kvarkus@gmail.com>
kvark pushed a commit to kvark/wgpu that referenced this pull request Jun 3, 2021
907: Add feature request for wgpu-rs r=kvark a=cwfitzgerald

Thanks @JMS55 for bringing this to my attention in gfx-rs#906 

Co-authored-by: Connor Fitzgerald <connorwadefitzgerald@gmail.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