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

Implement missing PrettyError impls #3066

Merged
merged 2 commits into from
Oct 5, 2022

Conversation

scoopr
Copy link
Contributor

@scoopr scoopr commented Oct 5, 2022

Checklist

  • Run cargo clippy.
  • Run RUSTFLAGS=--cfg=web_sys_unstable_apis cargo clippy --target wasm32-unknown-unknown if applicable.
  • Add change to CHANGELOG.md. See simple instructions inside file.

Connections
Closes #3061

Description
Some of the Error impls that refer to the resource ids were not formatting the labels for the resources

Testing
I must confess that I only compile-tested. Inducing or synthesizing the error conditions are a bit difficult.

@scoopr
Copy link
Contributor Author

scoopr commented Oct 5, 2022

I notice that these are possibly missing label formatting:

  • RequestAdapterError
  • ClearError
  • WaitIdleError
  • QueueSubmitError
    Those seemed less important though.
    Didn't double check the already existing PrettyError impls that they still handle all the correct labels

@scoopr
Copy link
Contributor Author

scoopr commented Oct 5, 2022

I also promote this to a point release if merged :)

Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

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

In the nick of time; changelog pls!

@cwfitzgerald cwfitzgerald merged commit 60fd2a3 into gfx-rs:master Oct 5, 2022
@Wumpf
Copy link
Member

Wumpf commented Oct 6, 2022

@scoopr thank you so much for going after this! But wasn't the bug I encountered a missing pretty print for DrawError which is still not covered?

@cwfitzgerald
Copy link
Member

(if closing the bug was not intended, that is my fault not scoopr's - I edited the body to close)

@Wumpf
Copy link
Member

Wumpf commented Oct 6, 2022

yeah from my understanding the issue is still there, but scoopr's changes are great regardless :)

@cwfitzgerald
Copy link
Member

Reopened!

@scoopr
Copy link
Contributor Author

scoopr commented Oct 6, 2022

This should fix #3061, I believe I misattributed the missing bits initially, when talking about it on Matrix. The #3061 error that is missing label is actually PassErrorScope::Draw, PassErrorScope already implemented PrettyError, but it had been forgotten from format_pretty_any downcasts.

Neither DrawError nor LateMinBufferBindingSizeMismatch actually referenced any Ids to resolve.

It would be great if you tested with the original error condition though, to make sure!

@Wumpf
Copy link
Member

Wumpf commented Oct 6, 2022

ohhh I see that makes sense, double missunderstanding then :). I'll test it today and get back to you!

@Wumpf
Copy link
Member

Wumpf commented Oct 6, 2022

confirmed fixed and suspecting that this was working already:

the error I had was

Caused by:
    In a RenderPass
      note: encoder = `egui_webpainter_paint_and_update_textures`
    In a draw command, indexed:false indirect:false
      note: render pipeline = `<RenderPipeline-(1, 1, Gl)>`
    Buffer is bound with size 16 where the shader expects 32 in group[0] compact index 0

As you can see it prints the encoder name but not the render pipeline name. So turns out I was in release mode the whole time, in debug in prints render pipeline label just fine. Encoder prints its name regardless because it is today the only resource that will still remember its name even if debug assertions are disabled (it has an overwrite here https://github.com/gfx-rs/wgpu/blob/master/wgpu-core/src/command/mod.rs#L237)

Took me an awful long time to realize that, but well I learned quite a bit in the process I'd like to believe :)

@Wumpf
Copy link
Member

Wumpf commented Oct 6, 2022

the fact that encoders keep their label regardless of debug_assertions enabled could be regardless a bug - but it seems it makes the code a lot easier since it needs to pass on that label to commandbuffer, so the tradeoffs are imho in favor of status quo

@scoopr
Copy link
Contributor Author

scoopr commented Oct 6, 2022

Oh alright, that makes more sense, PassErrorScope didn't really need to be handled in format_pretty_any after all, and now I fear that its actually printing the labels doubly, as it gets printed through the outer fmt_pretty impl.

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.

implement PrettyError for DrawError
3 participants