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

[wgpu] Add depth stencil initialization to Painter #2316

Merged
merged 3 commits into from
Nov 21, 2022

Conversation

lsh
Copy link
Contributor

@lsh lsh commented Nov 17, 2022

I was getting panics in the egui_demo_app when using a depth buffer. To add the depth buffer I had modified:

// egui_demo_app/src/main.rs
let options = eframe::NativeOptions {
    drag_and_drop_support: true,

    initial_window_size: Some([1280.0, 1024.0].into()),

    #[cfg(feature = "wgpu")]
    renderer: eframe::Renderer::Wgpu,
    depth_buffer: 1, // <-- enable depth buffer

    ..Default::default()
};

and

// egui_demo_app/src/apps/custom3d_wgpu.rs
// in create_render_pipeline
depth_stencil: Some(wgpu::DepthStencilState {
    format: wgpu::TextureFormat::Depth32Float,
    depth_write_enabled: true,
    depth_compare: wgpu::CompareFunction::Less,
    stencil: wgpu::StencilState::default(),
    bias: wgpu::DepthBiasState::default(),
}),

I realized that because the first frame can render before the window is set, it is possible for depth_buffer to exist, while depth_texture_view to be None. This check stopped the crash for me, but if there is an issue with this solution I would be happy to refactor.

@emilk emilk requested a review from Wumpf November 17, 2022 13:17
Copy link
Collaborator

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

putting this into paint_and_update_textures doesn't really make sense as we don't check for a configured surface every frame either. Also don't quite understand why you're doing map().iter().for_each on a single element - you can just map to the correct thing right away :).

Anyways, it would be best if set_window would do the same on Some(window) as on_window_resized does on self.surface_state.is_some().

I.e. best to put

            self.configure_surface(width_in_pixels, height_in_pixels);
            let device = &self.render_state.as_ref().unwrap().device;
            self.depth_texture_view = self.depth_format.map(|depth_format| {
                device
                    .create_texture(&wgpu::TextureDescriptor {
                        label: Some("egui_depth_texture"),
                        size: wgpu::Extent3d {
                            width: width_in_pixels,
                            height: height_in_pixels,
                            depth_or_array_layers: 1,
                        },
                        mip_level_count: 1,
                        sample_count: 1,
                        dimension: wgpu::TextureDimension::D2,
                        format: depth_format,
                        usage: wgpu::TextureUsages::RENDER_ATTACHMENT
                            | wgpu::TextureUsages::TEXTURE_BINDING,
                    })
                    .create_view(&wgpu::TextureViewDescriptor::default())
            });

into a function and call it at both locations. If that's unclear I can help out!

…ture_view`, and call it in `set_window` and `on_window_resized`
@lsh lsh requested a review from Wumpf November 19, 2022 19:23
@lsh
Copy link
Contributor Author

lsh commented Nov 19, 2022

@Wumpf thanks for explaining! Also feel free to suggest a better name for the texture view generator than resize_and_generate_depth_texture_view.

Copy link
Collaborator

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks great and thanks for contributing!

I added a change-log entry myself since I forgot to mention that earlier (:

@Wumpf Wumpf merged commit f4d8ab9 into emilk:master Nov 21, 2022
@lsh lsh deleted the depth-buffer-fix branch November 21, 2022 16:25
JohannesProgrammiert pushed a commit to JohannesProgrammiert/egui that referenced this pull request Jan 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants