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

Include naga parse error in the error chain #1760

Merged
merged 1 commit into from
Aug 3, 2021
Merged

Conversation

scoopr
Copy link
Contributor

@scoopr scoopr commented Aug 3, 2021

Connections
Probably makes #1424 moot

Description
Wraps the Naga ParseError with NagaParseError that shows the full Naga diagnostic in its Display impl and adding it to the CreateShaderModuleError::Parsing variant, allows for the wgsl diagnostics to be visible in the displayed validation error.

Testing
With modifying examples

Wraps the Naga's `ParseError` with `NagaParseError` type, that uses the
Naga's full error formatting for its `Display` impl, including shader
source.
@scoopr
Copy link
Contributor Author

scoopr commented Aug 3, 2021

Looks a little something like this

[2021-08-03T20:46:40Z ERROR wgpu::backend::direct] wgpu error: Validation Error

    Caused by:
        In Device::create_shader_module
          note: label = `water`
        Failed to parse a shader

    Shader error:
    error: invalid field accessor `f_WaterScreenPos`
        ┌─ wgsl:215:9
        │
    215 │     out.f_WaterScreenPos = (0.5 * gridpos.xy * (1.0 / gridpos.w)) + vec2<f32>(0.5, 0.5);
        │         ^^^^^^^^^^^^^^^^ invalid accessor




thread 'main' panicked at 'Handling wgpu errors as fatal by default', wgpu/src/backend/direct.rs:2101:5

Copy link
Member

@kvark kvark left a comment

Choose a reason for hiding this comment

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

Nice!

@kvark kvark merged commit 5d9c276 into gfx-rs:master Aug 3, 2021
@scoopr scoopr deleted the parse-error branch August 3, 2021 21:31
@almarklein
Copy link
Contributor

I'm really grateful for this change, but one issue I'm facing is that newlines are escaped in the result. An example:

λ python examples\geometry_cube.py
Failed to parse WGSL code for Some(""): no definition in scope for identifier: 'atomicExchange'
thread '<unnamed>' panicked at 'Parsing(NagaParseError { shader_source: "\n        [[block]]\n        struct Struct_u_stdinfo {\n            cam_transform: mat4x4<f32>;\n            cam_transform_inv: mat4x4<f32>;\n            projection_transform: mat4x4<f32>;\n            projection_transform_inv: mat4x4<f32>;\n            physical_size: vec2<f32>;\n            logical_size: vec2<f32>;\n        };\n\n        [[group(0), binding(0)]]\n        var u_stdinfo: Struct_u_stdinfo;\n        \n\n        [[block]]\n        struct Struct_u_wobject {\n            world_transform: mat4x4<f32>;\n            id: i32;\n        };\n\n        [[group(0), binding(1)]]\n        var u_wobject: Struct_u_wobject;\n        \n\n        [[block]]\n        struct [... etc.]

Is this intentional or could we change something to fix this? I reckon it should be a minor change, but I don't know enough about Rust strings to know what that change should be.

@kvark
Copy link
Member

kvark commented Aug 24, 2021

Interesting. This is definitely not what we see. Where does this unwrap happens? It's not in wgpu-core, it must be in wgpu-native itself. Should look like this -

fn default_error_handler(err: crate::Error) {

@almarklein
Copy link
Contributor

Would that be where create_shader_module is called? (and in check_error here

@kvark
Copy link
Member

kvark commented Aug 24, 2021

Right. It needs to be panic!("{}", error); instead of panic!("{:?}", error);

@almarklein
Copy link
Contributor

Then I get:

λ cargo build
   Compiling wgpu-native v0.6.0 (C:\dev\rust\wgpu-native)
error[E0277]: `E` doesn't implement `std::fmt::Display`
  --> src\lib.rs:53:22
   |
53 |         panic!("{}", error);
   |                      ^^^^^ `E` cannot be formatted with the default formatter
   |
   = note: in format strings you may be able to use `{:?}` (or {:#?} for pretty-print) instead
   = note: required by `std::fmt::Display::fmt`
   = note: this error originates in the macro `$crate::format_args` (in Nightly builds, run with -Z macro-backtrace for more info)
help: consider further restricting this bound
   |
51 | pub fn check_error<I, E: std::fmt::Debug + std::fmt::Display>(input: (I, Option<E>)) -> I {
   |                                          ^^^^^^^^^^^^^^^^^^^

error: aborting due to previous error

For more information about this error, try `rustc --explain E0277`.
error: could not compile `wgpu-native`

@kvark
Copy link
Member

kvark commented Aug 24, 2021

Could you try E: std::error::Error instead?

@almarklein
Copy link
Contributor

Then I can use panic!("{}", error); but then also the text written is reduced to only the error title (what we had before this pr),

@kvark
Copy link
Member

kvark commented Aug 24, 2021

Oh, I see now. In wgpu-rs, between the wgpu-core call and the user error report, there is also this piece happening -

let error = wgc::error::ContextError {
string,
cause: Box::new(cause),
label: label.unwrap_or_default().to_string(),
label_key,
};
let sink = sink_mutex.lock();
let mut source_opt: Option<&(dyn Error + 'static)> = Some(&error);
while let Some(source) = source_opt {
if let Some(wgc::device::DeviceError::OutOfMemory) =
source.downcast_ref::<wgc::device::DeviceError>()
{
return sink.handle_error(crate::Error::OutOfMemoryError {
source: Box::new(error),
});
}
source_opt = source.source();
}
// Otherwise, it is a validation error
sink.handle_error(crate::Error::ValidationError {
description: self.format_error(&error),
source: Box::new(error),
});

And that's not a part of wgpu-native atm. Would you mind filing an issue to wgpu-native to follow-up on this?

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.

3 participants