Experimental support for integration with existing wgpu apps.

[parasyte]

• Closes Render-api-v2 further enhancements #107

This has a few unavoidable breaking changes. The first is that the closure provided by Pixels::render_with() no longer accepts a reference to PixelsContext, but an owned copy of one. PixelsContext is now a view of the poorly named PixelsInternalContext (which is what now owns or borrows the top-level wgpu state).

The second breaking change is related to the first: Pixels::context() now returns an owned PixelsContext. This was ultimately the reason that the closure signature had to change, since this method cannot return a reference to a PixelsContext view borrowed from the stack. The closure signature was changed for consistency.

There are now three ways to build a Pixels instance:

• Pixels::new() remains unchanged. Simply calls PixelsBuilder::build().
• PixelsBuilder::build() remains unchanged. Creates a Pixels instance that owns the top-level wgpu state.
• PixelsBuilder::build_with_wgpu() is new, and allows Pixels to borrow existing top-level wgpu state. See Render-api-v2 further enhancements #107 for details.

This PR needs an example before it's ready to merge.

[parasyte]

@JMS55 wdyt? Would this solve your original need? I don't have an example for using the new builder method, but the code seems like it should work as expected.

[JMS55]

I completely forget what my original issue is tbh. Pixels has been working well for me ever since render_with() got added.

I'm also very busy the next few days - I'll review sometime next week (probably).

[parasyte]

Completely understandable! I'm in no rush with this one. Just want to start working toward #117.

Just some additional context that might help your memory: One of the pseudo-goals here is to promote pixels from "libraries that work with wgpu" to "middleware libraries that integrate with your existing wgpu application" on the wgpu wiki. This PR is a big improvement in that direction, but it's not quite middleware, yet. The middleware link has a detailed description of the moving parts.

[JMS55]

Code itself looks good. One nit: instead of separate build methods, perhaps it's better to make those builder methods, and then have only one build function? Not convinced either way. Maybe we just need different names for the functions that are more clear that you have to provide the wgpu stuff yourself, since build_with_wgpu is kind of vague (pixels already uses wgpu even without that function).

---

However I have some concerns with the overall idea. Is there any need to continue to have pixels borrow the wgpu stuff? As far as I see it, there's 3 scenarios:

1. Pixels only - Best to have pixels do as much work and hide as many details as possible (Pixels::new()).
2. Pixels with additional libraries - Best to have pixels do as much work as possible, but expose a step to let the user render stuff like post-processing and UI (Pixels::new() + Pixels::render_with()). Mostly focused around overlays on top of pixels.
3. Pixels as a library/middleware - Expose the scaling renderer and input helpers, but let the user provide wgpu and control the majority of things.

Right now 1. and 2. are in a good state. This PR is for adding support for 3. But I'm not sure it makes sense to continue to have pixels borrow wgpu state and be in charge of things (when it comes to supporting 3). It seems weird that for the case where pixels is a small part of your overall program, you have to give it a permanent reference to your wgpu structs, and structure your code around it, instead of just passing it things as needed.

What do you think of splitting things into a pure (no state) library, and another library that wraps the previous one and sets up all the wgpu[1] state for you? We could call them pixels-middleware and pixels.

[1] Potentially also winit, like we discussed in the webgl PR.

---

An additional concern: Is there any current demand for 3? My original use case was just UI/post-processing I believe, which pixels currently covers well. I'm concerned about making major changes if we have no way to test whether it's a good fit or not.

[parasyte]

Awesome review! Thanks for looking at this.

One nit: instead of separate build methods, perhaps it's better to make those builder methods, and then have only one build function?

I put together the following API from this suggestion. It replaces the second build method with an optional builder method to borrow wgpu state. But it is probably moot, following the other concerns you raised.

Click to expand...

impl<'wgpu, ..> PixelsBuilder<'wgpu, ..> {
    /// Borrow the given `wgpu` state.
    ///
    /// [`Pixels`] owns `wgpu` state by default. This method allows it to borrow state. Use this
    /// when integrating `pixels` into an application that already uses `wgpu`.
    ///
    /// Note that the following builder methods will become no-ops after this method is called:
    ///
    /// - [`PixelsBuilder::request_adapter_options`]
    /// - [`PixelsBuilder::device_descriptor`]
    /// - [`PixelsBuilder::wgpu_backend`]
    pub fn with_borrowed_wgpu(
        mut self,
        surface: &'wgpu wgpu::Surface,
        adapter: &'wgpu wgpu::Adapter,
        device: &'wgpu wgpu::Device,
        queue: &'wgpu wgpu::Queue,
    ) -> Self;

    /// Create a pixel buffer from the options builder.
    ///
    /// # Errors
    ///
    /// Returns an error when a [`wgpu::Adapter`] or [`wgpu::Device`] cannot be found.
    pub fn build(mut self) -> Result<Pixels<'wgpu>, Error>;
}

It wasn't too hard to refactor the code for this. (Gotta love Rust!)

---

It seems weird that for the case where pixels is a small part of your overall program, you have to give it a permanent reference to your wgpu structs, and structure your code around it, instead of just passing it things as needed.

I agree with the overall sentiment. It does seem weird that Pixels needs to borrow wgpu stuff for its whole lifetime.

What do you think of splitting things into a pure (no state) library, and another library that wraps the previous one and sets up all the wgpu[1] state for you? We could call them pixels-middleware and pixels.

Yes! I think this is a much cleaner design. It looks like the "Internal Render Data" middleware API roughly maps to ours. But instead of a prepare method, we have get_frame. It could always be renamed for the PixelsMiddleware struct.

[1] Potentially also winit, like we discussed in the webgl PR.

I'm all for creating another higher-level crate to manage winit. I think one of the strengths of pixels is that it is agnostic to the windowing environment. It works with fltk and sdl ❤️.

Is there any current demand for 3?

None at the moment. One of my other projects is a level editor for an old SNES game, so pixels would be a nice fit. Except I also need a rather complex GUI around it. I ended up just writing a basic wgpu app with egui and uploading texture data the same way pixels does, without the complexity of the scaling renderer. I thought that the middleware approach would make it easier to integrate pixels with this editor app, but I might just be able to use the pixel buffer as an egui image!

I also received a question today asking how to account for the size of UI elements like the menubar in the egui example (which partially covers the pixel buffer on the window). If this egui image idea works without much trouble, that will address both of those requirements without solving for scenario 3. And if that's the case, then there is indeed no demand for scenario 3 so I'd be happy to close this and the two tracking tickets #107 and #117 until some need comes up to revisit the middleware pattern.

[parasyte]

Just tried the egui image thing, and it works perfectly. Here's a patch for the egui example that demonstrates: 0.7.0...egui-menubar-size-v2

Closing all of these for now.