Larger resolutions fail to render

[BlackHoleFox]

I've been trying to use Pixels in a personal project, but I've hit what seems to be a bug with resolution handling. It seems that if you have a larger winit window, therefore a large surface texture, the first attempt at calling Pixels.render() crashes with a wgpu validation error. I think its related to the aspect ratio it ends up using,

[2021-08-26T01:52:49Z ERROR wgpu::backend::direct] wgpu error: Validation Error

    Caused by:
        In a RenderPass
          note: encoder = `<CommandBuffer-(0, 1, Vulkan)>`
        In a set_scissor_rect command
        Invalid ScissorRect parameters

I dug through just about the entire issue tracker but I wasn't able to find anything related. The closest thing seems to be this open PR about aspect ratios. Interesting, the minimal example on this branch doesn't panic rendering at 2K. I'm not sure how intentional this is though or if there's something else needed for the "ideal" result.

Steps to reproduce:

1. git clone https://github.com/parasyte/pixels.git
2. cd pixels/examples/minimal-winit
3. sed -i "s/320/2560/; s/240/1440/" src/main.rs
4. cargo run --release

OS: Windows 10 or Linux (X11 vs Wayland doesn't matter)

[parasyte]

That definitely doesn't have anything to do with pixel aspect ratio. The scissor was added with the update to wgpu 0.9, and the change is detailed in #180. The specific error is raised by wgpu here: https://github.com/gfx-rs/wgpu/blob/v0.9/wgpu-core/src/command/render.rs#L1505-L1511

It definitely should not be creating a scissor rect larger than the texture extent. (But I'll have to look into it this weekend at the earliest.)

Also FYI, #170 has some ideas for improving performance which will be necessary for large pixel buffers. (Especially if you plan to update it frequently.)

[BlackHoleFox]

Thanks for confirming my sanity that I wasn’t just doing something super wrong :)

Also FYI, #170 has some ideas for improving performance

That does sound rather appealing for this. I subbed to the issue so I can follow along.

[parasyte]

I found the bug! winit will only create windows that can fit on your desktop. If you choose a pixel buffer size that is larger than what the winit window can display, then you get this error.

[examples\minimal-winit\src\main.rs:38] window.inner_size() = PhysicalSize {
    width: 240,
    height: 1421,
}
[C:\Users\jay\other-projects\wgpu\wgpu-core\src\command\render.rs:1228] rect = Rect {
    x: 0,
    y: 0,
    w: 240,
    h: 1440,
}
[C:\Users\jay\other-projects\wgpu\wgpu-core\src\command\render.rs:1230] info.extent = Extent3d {
    width: 240,
    height: 1421,
    depth_or_array_layers: 1,
}

This shows the condition failing when the window size is smaller than the pixel buffer.

[BlackHoleFox]

winit will only create windows that can fit on your desktop.

Is this true all the time? winit seems to have no issue creating a 4K window that doesn't fit on my screen. It ends up taking up multiple monitors trying to get the most of it in view. But, it still doesn't fit by any means

[parasyte]

In my case, I requested a vertical screen height of 1440 on a monitor which only has 1440 vertical pixels. winit can't give me a window with that exact size, since the window includes decorations that take space (namely the title bar). The result of the call to window.inner_size() shows the actual dimensions available to the surface, in the copy-pasta above.

My initial reaction is that this is kind of outside of the scope of pixels, but there might still be something that can be done to improve the situation.

This is where the issue occurs when your patch is applied:

pixels/examples/minimal-winit/src/main.rs

Lines 28 to 43 in 480764e

	let window = {
	let size = LogicalSize::new(WIDTH as f64, HEIGHT as f64);
	WindowBuilder::new()
	.with_title("Hello Pixels")
	.with_inner_size(size)
	.with_min_inner_size(size)
	.build(&event_loop)
	.unwrap()
	};
	let mut pixels = {
	let window_size = window.inner_size();
	let surface_texture = SurfaceTexture::new(window_size.width, window_size.height, &window);
	Pixels::new(WIDTH, HEIGHT, surface_texture)?
	};
	let mut world = World::new();

HEIGHT is 1440 and window_size.height is 1421 (on my 1440p monitor, with window title bar etc.) This means that the SurfaceTexture is smaller than the pixel buffer. An easy thing for us to do here is ensure that the SurfaceTexture is always greater or equally sized to the pixel buffer, or return an error from Pixels::new().

[BlackHoleFox]

Thanks for clarifying more, though it still seems odd to me. If you don't mind, why does #151's branch work fine then? EDIT: Its cutoff a bit towards the top.

It doesn't seem to have an issue creating a window that goes beyond my monitors scale. In the example screenshoted, the window goes quite a ways more downwards past the taskbar:

In this case, the resolution is 1440x2960 (not swapped). These dimensions are being passed to with_inner_size() for the winit window, SurfaceTexture::new(), and Pixels::new() without issue.

[parasyte]

That branch works because it predates the scissor addition. There is no scissor rect, so it isn't possible to get the ScissorRectError from wgpu.

Arguably, I think it's still a bug to allow users to create pixel buffers that don't fit on the surface. pixels doesn't want to scale the buffer down to fit on the surface, because that would break pixel-perfect rendering.

Check the size of the window that winit actually creates. I'll bet it isn't 1440x2960 like you think it is. Just add dbg!(window.inner_size()); somewhere before entering the event loop.

[BlackHoleFox]

That branch works because it predates the scissor addition.

Ahh, that makes a lot more sense now, thanks for clarifying.

Arguably, I think it's still a bug to allow users to create pixel buffers that don't fit on the surface

I'd agree there since it leads to slightly more confusing errors and scaling can be taken care of by the user.

Looks like you're also right about the winit window deceiving me, I feel dumb now :)

[src\main.rs:57] window.inner_size() = PhysicalSize {
    width: 1440,
    height: 1421,
}

[parasyte]

I'm still thinking the right thing to do in this case is just return an error from Pixels::new() (and PixelsBuilder::build()) when the pixel buffer will not fit on the provided SurfaceTexture.

It might help to know why on earth you are trying to do this in the first place? But at least an error with an obvious reason would be better than just letting this happen until it hits some other issue at a deeper level.

[parasyte]

This bug also happens (as you might imagine) when resizing the window so the inner size is smaller than the pixel buffer size. This is easy with, e.g. a patch like this:

diff --git a/examples/minimal-winit/src/main.rs b/examples/minimal-winit/src/main.rs
index a48e7d0..8b599bb 100644
--- a/examples/minimal-winit/src/main.rs
+++ b/examples/minimal-winit/src/main.rs
@@ -30,7 +30,6 @@ fn main() -> Result<(), Error> {
         WindowBuilder::new()
             .with_title("Hello Pixels")
             .with_inner_size(size)
-            .with_min_inner_size(size)
             .build(&event_loop)
             .unwrap()
     };

With no minimum window constraint, dragging the window handle to a tiny size immediately triggers the same scissor error. To handle this, I'm going to panic with a friendly message in the resize methods when an invalid state is created like this.

[parasyte]

Still testing things out. I can make it not panic in Pixels::resize_surface() and Pixels::resize_buffer() if I come up with good scissor rect dimensions for any screen size smaller than the pixel buffer. Which means I can make the crate generally support pixel buffers that are larger than the surface just by clipping things appropriately. That would mean no new errors and no new panics.

[BlackHoleFox]

It might help to know why on earth you are trying to do this in the first place?

Great question :P Originally, I ran into this bug due to not handling resizing logic in my code correctly. However, in the time since then, I think that the possibility of a full-size/jumbo Pixels surface isn't fully out of the question for a kind of "enlarged"/fullscreen feature down the line for the project I'm working on.