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

Fix Display for pass errors #1068

Closed
wants to merge 2 commits into from
Closed

Fix Display for pass errors #1068

wants to merge 2 commits into from

Conversation

kvark
Copy link
Member

@kvark kvark commented Dec 2, 2020

Connections
Currently printing an error only outputs the scope.

Description
This PR makes it print the whole error.

Testing
Untested

@kvark
Copy link
Member Author

kvark commented Dec 2, 2020

@scoopr I was doing format!("{}", err). Am I missing anything? Could you review this PR?

@scoopr
Copy link
Contributor

scoopr commented Dec 2, 2020

I think @Kimundi did that originally, did I mess it up while doing the other changes?

Shouldn’t the inner be a source error, hence shouldn’t need to be repeated in the outer error message?

@kvark
Copy link
Member Author

kvark commented Dec 2, 2020

Sorry, I might have confused the authorship.

Shouldn’t the inner be a source error, hence shouldn’t need to be repeated in the outer error message?

Well, that sounds logical, but how do I get the full error then?

@scoopr
Copy link
Contributor

scoopr commented Dec 2, 2020

Well, that sounds logical, but how do I get the full error then?

That’s what format_error in wgpu-rs does, are you looking at this from another consumers perspective?

@kvark
Copy link
Member Author

kvark commented Dec 2, 2020

Yeah

@kvark
Copy link
Member Author

kvark commented Dec 2, 2020

I guess another option here is to have format_error at wgpu level, so that non-wgpu-rs clients can take advantage of it

@scoopr
Copy link
Contributor

scoopr commented Dec 3, 2020

I guess another option here is to have format_error at wgpu level, so that non-wgpu-rs clients can take advantage of it

Well yes, you kind of pushed the error reporting to be moved to wgpu-rs level, this includes the id to label resolution, formatting and also some of the top level errors are injected there.

I don't think there is anything in the way of moving all (or some) of that back to wgpu.

Maybe format_error implementation could be moved just by making it accept the Global, or implement it directly on Global, so it can do the label resolution. But it is also doing the label resolution for the ContextError that is defined in wgpu-rs, so either that needs to be moved too or some method of "injecting" more supported types.

@kvark
Copy link
Member Author

kvark commented Dec 3, 2020

Well yes, you kind of pushed the error reporting to be moved to wgpu-rs level, this includes the id to label resolution, formatting and also some of the top level errors are injected there.

Yes, I remember. I did a similar thing to errors in general at first, wanting wgpu-core to just return Result and the clients to deal with the "error IDs". Then I had to rewrite wgpu-core to handle that more transparently for all the clients.

So for format_error, we need something like this for Servo and Gecko, so we might as well move it in wgpu-core now. Sorry about making the wrong call previously!

@kvark
Copy link
Member Author

kvark commented Dec 5, 2020

Is there any harm/downside to landing this? Would it make wgpu-rs errors wrong? cc @Kimundi

@Kimundi
Copy link
Contributor

Kimundi commented Dec 8, 2020

It would change the wgpu-rs error output from something like

Caused by:
    In a RenderPass
      note: encoder = `<CommandBuffer-(0, 1, Vulkan)>`   
    In a set_bind_group command
      note: bind group = `<BindGroup-(0, 1, Vulkan)>`    
    bind group index 9 is greater than the device's requested `max_bind_group` limit 4

To something like this:

Caused by:
    In a RenderPass
      note: encoder = `<CommandBuffer-(0, 1, Vulkan)>`   
    Render pass error In a set_bind_group command: bind group index 9 is greater than the device's requested `max_bind_group` limit 4
      note: bind group = `<BindGroup-(0, 1, Vulkan)>`    
    bind group index 9 is greater than the device's requested `max_bind_group` limit 4

...which does not look quite right :D

@kvark
Copy link
Member Author

kvark commented Dec 8, 2020

Ok, it sounds like we need to get back to the board and try to integrate the pretty errors into wgpu itself.
Is this something @Kimundi feels generous to help with?

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