[WIP] Merge window creation between backends #2136

goddessfreya · 2018-06-10T23:40:06Z

Signed-off-by: Hal Gentz zegentzy@protonmail.com

Fixes #1619
PR checklist:

kvark · 2018-06-14T15:59:56Z

src/hal/src/lib.rs

@@ -375,6 +375,8 @@ pub trait Backend: 'static + Sized + Eq + Clone + Hash + fmt::Debug + Any + Send
    type Fence:               fmt::Debug + Any + Send + Sync;
    type Semaphore:           fmt::Debug + Any + Send + Sync;
    type QueryPool:           fmt::Debug + Any + Send + Sync;
+
+    type Window:                           Any + Send + Sync;


This shouldn't be here, ideally.
Could you explain the thought process that leaded you to this decision? GL should not affect our HAL API.

Well, the gl backend and the non gl backends use a different type for their windows, so I wanted to avoid having to define a type in the examples based on which backend is in use. But then I relized that we won't have a type to use for people not using either winit or glutin, so I'm already aware that it's going to need to get stripped out.

goddessfreya · 2018-06-25T04:57:29Z

Latest version was tested against rust-windowing/winit@c873c2d and goddessfreya/glutin@cdad4a9 FYI

goddessfreya · 2018-06-25T07:01:35Z

src/backend/gl/src/command.rs

@@ -886,9 +886,13 @@ impl command::RawCommandBuffer<Backend> for RawCommandBuffer {
            for new_binding in &*desc_set.bindings.lock().unwrap() {
                match new_binding {
                    n::DescSetBindings::Buffer {ty: btype, binding, buffer, offset, size} => {
+                        let btype = match btype {


This is not strictly necessary and could be separated into a separate PR.

goddessfreya · 2018-06-25T07:33:08Z

A couple notes:

While the changes here haven't been tested with multiwindow setups w/ the opengl backend, it should make supporting them easier.
Cargo.tomls will be returned to there prior state
The changes here limit users of gfx to one events loop per instance (and given they will likely only have one instance, that's basically one events loop per program). I think this is 100% OK because after talking with Franseska I discovered that they don't officially support more than 1 event loop per application. Nor do they support event loops not on the main thread on macOS.
The gl code isn't the most simplistic and I'd be up for ideas at simplifying it.
Insuring we are always on the main context (unless when we need to not be) is a possible source of bugs which might be hard to track down. Up for ideas on how to eliminate their possibility. (Maybe accessing a share should make it's context current? What if they try to access multiple contexts simultaneously?)
Now respecting the user's swap chain config is a couple changes away. I opted to not include it in this PR because I consider it out of this PRs scope (and more importantly, I'm lazy.)
We do open separate devices per window, but those are "handled" internally.)
Currently only works on linux (every backend) & windows (wgl), see Context sharing not implemented for many platforms rust-windowing/glutin#899 and Adds context sharing support for the X11 & Wayland backends rust-windowing/glutin#1031
Once this is in, hopefully we can do the portability cts stuff with the opengl backend (yay!)
Some changes here are to support glutin's (or was it winit?) new hidpi support model. (Don't remember which changes tho :( )

Anyways, I quick summary of this PR off the top of my head is that we:

Have instances make a windowed headless context.
We delay window making till swapchain creation.
When recreating a swapchain we don't recreate the window (comment in code for why)
When creating a swapchain the first time we make an vao, some shaders, a vbo with stuff for rendering a quad which we share between all instances.
When creating a swapchain, we open them a new share + window + context + image. This is stuff we can't share
When recreating a swapchain, we only recreate the image.
When the surface's compatibilities are queried for and the window has been already made, we give it the window's settings (cause we can't change them). If it hasn't we just give them everything because it doesn't matter since we ignore it anyways.
When doing rendering stuff, we render to the image belonging to the swapchain they want to render to.
When presenting, we switch to that swapchain's context, bind the texture & then render a quad (with that texture) to that window's framebuffer.

Edit: I've typed this big block of text at 1:34 AM so forgive me for any errors (and for the block of text).

kvark

I skimmed through it, need to re-read your comments and have another look.

kvark · 2018-06-25T15:39:14Z

examples/quad/main.rs

    let (window, _instance, mut adapters, mut surface) = {
-        let window = wb.build(&events_loop).unwrap();
-        let instance = back::Instance::create("gfx-rs quad", 1);
+        let instance = back::Instance::create("gfx-rs quad", 1, Arc::clone(&events_loop));


how would this map to gfx-portability? We don't have the event loop when we are creating an instance there.

I have no clue, I'd have to look into it.

kvark · 2018-06-25T15:42:25Z

src/backend/gl/src/device.rs

@@ -424,6 +428,74 @@ impl Device {
            }
        }
    }
+
+    pub fn create_shader_program<'a>(


are these changes a part of the windowing PR? or just refactoring accidentally got mixed in?

I wanted to avoid re implementing create_shader_programs when making the shader for the swapchain quad stuff.

why do you need a shader?

kvark · 2018-06-25T15:45:42Z

src/backend/gl/src/lib.rs

+pub struct Instance {
+    el: Arc<Mutex<glutin::EventsLoop>>,
+    context: glutin::Context,
+    pub(crate) quadvbo: Arc<Mutex<Option<u32>>>,


nit: you can insert _ between words

kvark · 2018-06-25T15:46:36Z

src/backend/gl/src/lib.rs

+        Arc::new(Instance {
+            el,
+            context,
+            quadvbo: Arc::new(Mutex::new(None)),


why aren't we initializing those here?

We haven't loaded the gl pointers yet nor done the stuff which is done when opening the device. I'm not actually sure what the opening the device stuff is done for so I avoided changing that code and just made the stuff get initialized when making a swap chain the first time.

I guess we could make the physical device & open the device once in the instance then just return those. I think that's what I'll change it to do, and then we can avoid reloading the gl ptrs in drop.

kvark · 2018-06-25T15:47:14Z

src/backend/gl/src/lib.rs

+impl Drop for Instance {
+    fn drop(&mut self) {
+        unsafe {
+            let gl = gl::Gl::load_with(|s| self.context.get_proc_address(s) as *const _);


why are we loading GL pointers here?

Because the instance doesn't have a copy of the share or the gl ptrs, so it's easier (and still correct) to just load them in again.

kvark · 2018-06-26T00:57:20Z

@zegentzy what do you think about my earlier plan to go with the "pure" GL context (for the Instance) and sharing for all the others (created per window/swapchain)? Would it still be compatible with your approach if we get rust-windowing/glutin#942 merged eventually?

goddessfreya · 2018-06-26T02:22:47Z

I don't see why it wouldn't work, instead of sharing everything with the instance's context, we share them with each other. Then we need to make current the context for the window we are using.

Which method is preferable is up to debate, and I personally don't have an opinion on this.

goddessfreya · 2018-06-27T03:36:04Z

We now all the shared stuff in the instance. We now automatically change contexts only when necessary.
Tested against rust-windowing/winit@c873c2d and goddessfreya/glutin@cdad4a9 FYI

goddessfreya · 2018-06-27T20:52:44Z

We just blit fbo's now as discussed on gitter with @kvark .
Tested against rust-windowing/winit@c873c2d and goddessfreya/glutin@cdad4a9 FYI

Signed-off-by: Hal Gentz <zegentzy@protonmail.com>

goddessfreya · 2018-07-07T09:28:05Z

Honestly, this just makes the code a lot uglier, and after a five second glance at gfx-portability I relied that this wouldn't work. Shame, because the only reason I tried this was to get the GL backend to be added to gfx-portability.

I'll sleep on this and think of a different solution.

2211: Latest winit, dpi bugfix and remap descriptors fix. r=kvark a=ZeGentzy Some minor fixes I did while working on #2136 PR checklist: - [ ] `make` succeeds (on *nix) - [ ] `make reftests` succeeds - [x] tested examples with the following backends: gl + vulkan on archlinux Co-authored-by: Hal Gentz <zegentzy@protonmail.com>

goddessfreya force-pushed the merge-window-creation branch from 77af818 to c98b8b8 Compare June 12, 2018 02:11

kvark reviewed Jun 14, 2018

View reviewed changes

goddessfreya force-pushed the merge-window-creation branch from 07fee6c to 15d9dc4 Compare June 25, 2018 04:45

goddessfreya commented Jun 25, 2018

View reviewed changes

goddessfreya force-pushed the merge-window-creation branch from 15d9dc4 to cd1a044 Compare June 25, 2018 07:15

kvark reviewed Jun 25, 2018

View reviewed changes

goddessfreya force-pushed the merge-window-creation branch 2 times, most recently from 964ff92 to 48c51b7 Compare June 27, 2018 00:22

goddessfreya force-pushed the merge-window-creation branch from 48c51b7 to 291833c Compare June 27, 2018 20:51

goddessfreya force-pushed the merge-window-creation branch 2 times, most recently from 073ed46 to cacdb1f Compare June 27, 2018 22:47

goddessfreya added 4 commits July 7, 2018 01:20

Window creation stuff

Unverified

This user has not yet uploaded their public signing key.

GPG key ID: AEFF36671C5657F8

Learn about vigilant mode

aa72e1b

Signed-off-by: Hal Gentz <zegentzy@protonmail.com>

Fixes

Unverified

This user has not yet uploaded their public signing key.

GPG key ID: AEFF36671C5657F8

Learn about vigilant mode

5399982

Signed-off-by: Hal Gentz <zegentzy@protonmail.com>

Update comment

Unverified

This user has not yet uploaded their public signing key.

GPG key ID: AEFF36671C5657F8

Learn about vigilant mode

c9028d9

Signed-off-by: Hal Gentz <zegentzy@protonmail.com>

Fixes

Unverified

This user has not yet uploaded their public signing key.

GPG key ID: AEFF36671C5657F8

Learn about vigilant mode

d6be479

Signed-off-by: Hal Gentz <zegentzy@protonmail.com>

goddessfreya force-pushed the merge-window-creation branch from cacdb1f to d6be479 Compare July 7, 2018 07:51

goddessfreya mentioned this pull request Jul 7, 2018

Latest winit, dpi bugfix and remap descriptors fix. #2211

Merged

3 tasks

goddessfreya closed this Jul 7, 2018

goddessfreya mentioned this pull request Aug 26, 2018

[NOMERGE] Merge window creation, attempt 2 #2347

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Merge window creation between backends #2136

[WIP] Merge window creation between backends #2136

goddessfreya commented Jun 10, 2018 •

edited

Loading

kvark Jun 14, 2018

goddessfreya Jun 14, 2018

goddessfreya commented Jun 25, 2018

goddessfreya Jun 25, 2018

goddessfreya commented Jun 25, 2018 •

edited

Loading

kvark left a comment

kvark Jun 25, 2018

goddessfreya Jun 25, 2018

kvark Jun 25, 2018

goddessfreya Jun 25, 2018

kvark Jun 25, 2018

kvark Jun 25, 2018

kvark Jun 25, 2018

goddessfreya Jun 25, 2018

goddessfreya Jun 25, 2018 •

edited

Loading

kvark Jun 25, 2018

goddessfreya Jun 25, 2018

kvark commented Jun 26, 2018

goddessfreya commented Jun 26, 2018

goddessfreya commented Jun 27, 2018

goddessfreya commented Jun 27, 2018

goddessfreya commented Jul 7, 2018

[WIP] Merge window creation between backends #2136

[WIP] Merge window creation between backends #2136

Conversation

goddessfreya commented Jun 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goddessfreya commented Jun 25, 2018

Choose a reason for hiding this comment

goddessfreya commented Jun 25, 2018 • edited Loading

kvark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goddessfreya Jun 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kvark commented Jun 26, 2018

goddessfreya commented Jun 26, 2018

goddessfreya commented Jun 27, 2018

goddessfreya commented Jun 27, 2018

goddessfreya commented Jul 7, 2018

goddessfreya commented Jun 10, 2018 •

edited

Loading

goddessfreya commented Jun 25, 2018 •

edited

Loading

goddessfreya Jun 25, 2018 •

edited

Loading