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

API design for shader chaining #316

Open
parasyte opened this issue Nov 8, 2022 · 7 comments
Open

API design for shader chaining #316

parasyte opened this issue Nov 8, 2022 · 7 comments
Assignees
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@parasyte
Copy link
Owner

parasyte commented Nov 8, 2022

In the very early days of this crate, we had a trait and heterogenous list of shaders. The idea was to allow shaders to be chained together to produce a complete set of render passes. See: https://docs.rs/pixels/0.0.4/pixels/trait.RenderPass.html

The RenderPass trait was replaced in v0.1 with the Pixels::render_with() method in #95 and #96. This API simplified the design substantially, at the cost of making certain things more difficult, and some things impossible.

This issue tracks the reintroduction of this feature to chain shaders programmatically

Rationale

Shaders that need to operate in pixel buffer space (opposed to screen space) cannot be implemented easily today because the default renderer is hardcoded to accept the pixel buffer texture view as its only input. (See #285) To build these kinds of shaders, one must reimplement the scaling renderer on the user-side and ignore the renderer passed to render_with() via PixelsContext.

Chaining shaders should be a first-class experience. The examples in

let render_result = pixels.render_with(|encoder, render_target, context| {
let noise_texture = noise_renderer.get_texture_view();
context.scaling_renderer.render(encoder, noise_texture);
noise_renderer.update(&context.queue, time);
time += 0.01;
noise_renderer.render(encoder, render_target, context.scaling_renderer.clip_rect());
Ok(())
});
and
let render_result = pixels.render_with(|encoder, render_target, context| {
let fill_texture = fill_renderer.get_texture_view();
context.scaling_renderer.render(encoder, fill_texture);
fill_renderer.render(encoder, render_target);
Ok(())
});
are not ideal; chaining is entirely ad hoc and by convention. Each custom renderer has a bespoke method to create a texture view, which is used to pass the result from the scaling renderer to the custom renderer.

A more unified API would treat these renderers as "of the same class" where the interface itself offers a seamless way to chain each renderer together.

This existing API also forces users to synchronize the inverse scaling matrix to handle mouse coordinates with multiple passes. The scissor rect also needs to be set correctly, etc. See #262.

API Sketch

This is very incomplete pseudocode, but I want to put my thoughts down now. (And maybe it will help someone make progress here if they feel inclined.)

pub trait Renderer {
    fn chain(&mut self, next: Box<dyn Renderer>) -> Option<Box<dyn Renderer>>;

    fn resize(&mut self, width: u32, height: u32);

    fn render(&self, encoder: &mut wgpu::RenderPass);
}

This API will allow a Renderer to consume another Renderer for chaining purposes. In other words, the next arg becomes a child of self after chaining. The method returns an existing child if it needs to be replaced. The render method takes a mutable RenderPass, which each renderer in the chain can recursively render to.

Unresolved Questions

  • How does chaining "connect" the render target of one renderer to the input texture view on the next? the RenderPass only has one color attachment, decided by the caller (which is pixels itself; and it only uses the Surface texture view as the color attachment).

    • Alternatively, we can continue using the "pass &mut wgpu::Encoder and &wgpu::TextureView as the render target" pattern that we have today. This requires renderers to create a new RenderPass for themselves and chaining would be performed by each Renderer with a method like fn update_input(&mut self, texture_view: wgpu::TextureView), e.g. called by resize().
  • Is there anything else we can learn from other users of wgpu?

    • This wiki page has some ideas for how to implement wgpu middleware: https://github.com/gfx-rs/wgpu/wiki/Encapsulating-Graphics-Work
    • glyph_brush doesn't use the middleware pattern as defined in the link above. Instead its draw_queued() method resembles our ScalingRenderer::render() method as it is today.
    • It looks pretty much the same with iced.
@parasyte parasyte added enhancement New feature or request help wanted Extra attention is needed labels Nov 8, 2022
@parasyte parasyte self-assigned this Nov 8, 2022
@parasyte
Copy link
Owner Author

parasyte commented Nov 8, 2022

For more historical context, see #107, #117 and #221.

@wiz21b
Copy link

wiz21b commented Nov 28, 2022

Continuing from a previous post... Not quite a discussion of a new API, just experience sharing. So in order to make an NTSC emulation, my first step was to be able to put a shader between the pixel surface rendering and the scaling renderer. What I have done is simply to copy/paste the ScalingRenderer code and modify it so that it doesn't use a SurfaceSize which is private (a really minor modification). Then I wired things like this:

  • my own shader which takes as input the pixels.context().texture and writes to an intermediate texture of mine.
  • the scaling renderer which takes as input the intermediate texture and outputs to the render_target provided by render_with()

The hardest part was to understand WSGL a bit (I've never done a single line of WGSL before but bit of GLSL).

As I said earlier, this is a very simple architecture. If I had a simple example and a reusable ScalingRenderer (i.e. wihout the SurfaceSize input) then I would have been up and running pretty fast. My point is: maybe I don't need a full scale API just for that; a simple example could be enough. But maybe I'll add some post processing, which means either I'll modify the ScalingRenderer shader either I'll need to chain another renderer behind it. Dunno.

But (there's but) doing things like this seems to be hard on pixels: it's no longer rendering at 60fps... It's strange because I leverage render_with() with just one more shader which is part of the same CommandEncoder...

@wiz21b
Copy link

wiz21b commented Dec 7, 2022

Now I have a prototype that mostly works. I'm stille trying to figuring out how the HiDPI influences scaling.

I think the interesting point for @parasyte here is that I have been able to reuse the ScalingRenderer bit but to do so I had to change it (and so wasn't unable to reuse it as is). The change consists in replacing:

surface_size: &SurfaceSize,

with

        surface_width: u32,
        surface_height: u32,

in order to avoid to use the Surfacesize type which is private. I guess I could have just changed the visibility of SurfaceSize instead.

So, with that very small change and using render_with I was able to hook my own shader right before the scaling one. It's not exactly a brand new shader chaining API, but that's a useful step nonetheless !

@parasyte
Copy link
Owner Author

parasyte commented Dec 7, 2022

Either of those changes seem appropriate if we are going to keep the existing design. It's also a low friction change and could be done as part of the next major release.

I suspect it would also need a method to update its texture view and bindings like the two custom shader examples allow. It would avoid recreating the entire pipeline when handling resize_buffer() calls. Likewise, changing anything about the texture view including using a different texture view from a different pipeline's render target.

@wiz21b
Copy link

wiz21b commented Dec 8, 2022

Hmmm... Hold on. Although I have some kind of prototype running, it doesn't work well on HiDpi screens: there are some discrepancies between the window size and the buffer size which ends up to be very ugly or simply not working. I didn't see that before 'cos the PC I'm used to has no HiDpi screen. First investigations show that there is some deep flaw in my understanding of Pixels's pipeline. Could you confirm that the rendering goes like this:

  • first the user application fills in a buffer provided by pixels (Pixels.pixels, in RAM)
  • then pixels copies that buffer to the GPU in a texture stored in Pixels.context.texture
  • then pixels renders that Pixels.context.texture onto the surface provided by Pixels.context.surface with the ScalingRender function.

Again, what I'm doing is:

  • first the user application fills in a buffer provided by pixels (Pixels.pixels, in RAM)
  • then pixels copies that buffer to the GPU in a texture stored in Pixels.context.texture
  • using a shader, I transform Pixels.context.texture to an intermediate texture
  • that intermediate texture is then rendered onto the surface provided by Pixels.context.surface with the ScalingRender function.

So I come before ScalingRender. The issue I have is that the physical size of the windows determines the pixels which will be processed by my shader. Right now, I'm trying to make sure every texture and the window do have the very same size, in physical pixels. But it still doesn' t work.

Unfortunately, the more I think about it, the more I think I'm leaving the scenario Pixels has been designed for... But that would be too bad to leave it because there is already a ton of code in it that's really good...

@parasyte
Copy link
Owner Author

parasyte commented Dec 8, 2022

Your understanding of the pipeline sounds correct.

High DPI displays don't really do anything special. For a display like on my MacBook Pro, the logical pixel size is 2x larger than the physical pixel size. I.e., the scaling ratio is 2.0. It is exactly identical to the situation where the window's inner area is 2x larger than the pixel buffer and the logical and physical pixel sizes are the same (scaling ratio = 1.0). This is from a renderer perspective; the logical pixel size plays a role for UX purposes like mouse coordinates and making the window "normal sized" instead of way too small on a high DPI display.

So even if you put your shader before the scaling renderer, you should be getting the same kind of result on the high DPI display as if your window was scaled up by the same ratio on a normal DPI display. Remember that the "backing texture" (Pixels.context.texture) is always the same size as the pixel buffer, and the "render texture" may be scaled up (either because the window is big or because the display has a higher DPI).

All that is to say, the texture your shader samples will always be the pixel buffer size, and the texture it renders to should be whatever size you want to output without taking high DPI or scaling into account. It's the scaling renderer's job to scale the output, not your shader. Another way to think about it is that your shader output size should use logical pixels.

Or alternatively, don't use the scaling renderer at all, and do the scaling directly in your shader. Then you do have to take physical pixel size with high DPI into account.

@wiz21b
Copy link

wiz21b commented Dec 9, 2022

Thanks for your great explanation. I've double checked my code and fixed it. I have not yet implemented the resize operation but it shouldn't be any trouble. So I can confirm that making ScalingRenderer fully accessible by letting the SurfaceSize be public allows for plugging in shaders before pixels' own stuff. That's not a full blown API but it unlocks quite a lot of possibilities. Thanks @parasyte !

Regarding the API proposal, I'm no expert but building separated render pass may be the easiest. Moreover, as I can see with my stuff (single data point !), we actually don't need full blown chaining. Being able to inject a shader before/after pixels' stuff is already quite a lot. And once you've done that, as a pixels user you have already been facing 99% of the complexity which is setting up wgpu (that was, for me, by far the trickiest past).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants