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

Investigate lowering limits that graphics backend needs to provide #908

Closed
hannobraun opened this issue Aug 3, 2022 · 2 comments · Fixed by #1273
Closed

Investigate lowering limits that graphics backend needs to provide #908

hannobraun opened this issue Aug 3, 2022 · 2 comments · Fixed by #1273
Labels
good first issue Good for newcomers topic: display Displaying Fornjot models type: development Work to ease development or maintenance, without direct effect on features or bugs

Comments

@hannobraun
Copy link
Owner

To initialize wgpu-rs, we need to pass it a set of limits that we require the hardware to fulfill (see wgpu::Limits).

We're currently using wgpu::Limits::default(), which is only guaranteed to work on modern backends. Since web support is a goal, and WebGPU seems unlikely to be widely available soon (I might be wrong), it might be required to use wgpu::Limits::downlevel_webgl2_defaults() instead, which be guaranteed to work with WebGL.

When I try to do that, however, I'm getting this error:

cargo run
    Finished dev [unoptimized + debuginfo] target(s) in 0.09s
     Running `target/debug/fj-app`
  2022-08-03T13:46:15.997237Z ERROR wgpu::backend::direct: Handling wgpu errors as fatal by default
    at /home/hanno/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/wgpu-0.12.0/src/backend/direct.rs:2272

thread 'main' panicked at 'wgpu error: Validation Error

Caused by:
    In Device::create_texture
    Dimension 3840 value 3840 exceeds the limit of 2048

', /home/hanno/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/wgpu-0.12.0/src/backend/direct.rs:2273:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread '<unnamed>' panicked at 'Channel is disconnected: SendError { .. }', crates/fj-host/src/lib.rs:195:44

It would be good to investigate whether the higher limit is actually required, and how to lower it.

The limits are requested here: https://github.com/hannobraun/Fornjot/blob/0a6f2598b79a24bb431f5abb89cd0617c0d6245a/crates/fj-viewer/src/graphics/renderer.rs#L151

I don't know where that error originates, but I think we're not using Device::create_texture anywhere in our own code, so it's probably used by egui.

Labeling as https://github.com/hannobraun/Fornjot/labels/good%20first%20issue, since this problem is limited in scope (graphics code) and doesn't require much knowledge of Fornjot itself. Experience with wgpu-rs and possibly egui would be beneficial, of course.

@hannobraun hannobraun added good first issue Good for newcomers topic: display Displaying Fornjot models type: development Work to ease development or maintenance, without direct effect on features or bugs labels Aug 3, 2022
@hannobraun hannobraun added this to the Initial Web Support milestone Aug 3, 2022
@hekno25
Copy link
Contributor

hekno25 commented Aug 3, 2022

Is it possible that this only occurs on 4k monitors with Fornjot in fullscreen (or any screen size with a dimension greater than 2048 pixels)?

I found one usage of create_texture() in this associated function:
https://github.com/hannobraun/Fornjot/blob/b68b6c1a32eb4b1aed571eabcf066f5188f863af/crates/fj-viewer/src/graphics/renderer.rs#L591-L610

This function is then used when constructing the renderer:
https://github.com/hannobraun/Fornjot/blob/b68b6c1a32eb4b1aed571eabcf066f5188f863af/crates/fj-viewer/src/graphics/renderer.rs#L178

The provided surface_config is constructed just before the depth buffer with a size field based on the size of the screen:
https://github.com/hannobraun/Fornjot/blob/b68b6c1a32eb4b1aed571eabcf066f5188f863af/crates/fj-viewer/src/graphics/renderer.rs#L168-L175

This all happens again on resize:
https://github.com/hannobraun/Fornjot/blob/b68b6c1a32eb4b1aed571eabcf066f5188f863af/crates/fj-viewer/src/graphics/renderer.rs#L290-L299

I'm not experienced in this kind of thing but if this is the problem, my guess is that, for WebGL builds, the size of the surface should be limited to 2048 in each dimension and then upscaled if necessary.

@hannobraun
Copy link
Owner Author

Thanks for looking into this @hekno25! I'm not super-knowledgeable about that stuff myself, but everything you say sounds reasonable.

Is it possible that this only occurs on 4k monitors with Fornjot in fullscreen (or any screen size with a dimension greater than 2048 pixels)?

I do have a 4k monitor, so that tracks. That 3840 from the error message is exactly the width of my display.

I found one usage of create_texture() in this associated function:

Ah, good catch! I forgot about the depth buffer.

I'm not experienced in this kind of thing but if this is the problem, my guess is that, for WebGL builds, the size of the surface should be limited to 2048 in each dimension and then upscaled if necessary.

That makes sense.

Here's an alternative idea:

  1. Get the WebGL2 defaults: wgpu::Limits::downlevel_webgl2_defaults
  2. Get the actually supported limits: wgpu::Adapter::limits
  3. Check that the screen resolution is not larger than those limits. Return a nice error that we can handle in a user-friendly way, if not. (I think we'll have to repeat that check on every resize.)
  4. Override the resolution of the WebGL2 limits with the actually supported limits: wgpu::Limits::using_resolution
  5. Initialize the device using those overridden limits.

Could it be possible, that we never run into this error if we do this? I mean, if WebGL2 only supports 2048 pixels, maybe we'll never get a bigger screen size in the browser? Maybe it already takes care of that upscaling for us? No idea, if this is true.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers topic: display Displaying Fornjot models type: development Work to ease development or maintenance, without direct effect on features or bugs
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants