Implementation of debug markers #713
Conversation
![monocodus[bot]](https://avatars.githubusercontent.com/in/28526?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thank you!
It's great to see this coming online, much appreciated!
Is it looks idiomatic? I saw only one example of pushing dynamic array and here we have work with text?
Yes
Is naming correct? I mean in wgpu it push and pop debug group, but in gfx it called begin and end debug group (should I rename?)
Yes. see https://gpuweb.github.io/gpuweb/#command-encoder-debug-markers
How can I test it, is it an example repository nearby or should I create an example by myself? Or may be we can create an example folder here and add "triangle" and other?
In an ideal situation, a PR links to the draft PRs in https://github.com/gfx-rs/wgpu-rs and https://github.com/gfx-rs/wgpu-native, which you made locally by overriding the dependency to your work. Then you'd be able to test it on any of the wgpu-rs examples, also ensure that API tracing/replay works. Of course requiring this for all the PRs is a bit too much, so we apply our best judgement on whether it's required on a case by case basis, as well as do it ourselves sometimes.
We are also thinking of a way to test features within this repository by relying on the API replay infrastructure.
There was a problem hiding this comment.
Thank you for updating!
A few more minor changes are needed.
Also, please try rebasing and squashing the commits. Render bundles were just merged, and they affect render commands. It's totally fine to just panic with unimplemented! for these commands when recording a bundle.
|
When the review will pass, I will add a new example (or just modify existing without committing it to wgpu-rs) in wgpu-rs examples folder with debug markers and after I will run it with wgpu backend and attach results here. |
wgpu-core/src/command/compute.rs
Outdated
| @@ -166,7 +188,7 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> { | |||
| ); | |||
|
|
|||
| if let Some((pipeline_layout_id, follow_ups)) = | |||
| binder.provide_entry(index as usize, bind_group_id, bind_group, offsets) | |||
| binder.provide_entry(index as usize, bind_group_id, bind_group, offsets) | |||
There was a problem hiding this comment.
Eh, such stuff was modified via rustfmt
wgpu-core/src/command/bundle.rs
Outdated
| @@ -104,6 +104,7 @@ pub struct RenderBundle { | |||
| } | |||
|
|
|||
| unsafe impl Send for RenderBundle {} | |||
|
|
|||
There was a problem hiding this comment.
hmm, another rustfmt thing? We are running it locally all the time, so I'm surprised you see changes.
My guess is you are using another (some weird?) version of rustfmt. We use the stable one that comes with rustup comoponent add rustfmt. Please double-check that, there shouldn't be formatting changes outside of your code - it's all already formatted.
wgpu-core/src/command/compute.rs
Outdated
| @@ -166,7 +188,7 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> { | |||
| ); | |||
|
|
|||
| if let Some((pipeline_layout_id, follow_ups)) = | |||
| binder.provide_entry(index as usize, bind_group_id, bind_group, offsets) | |||
| binder.provide_entry(index as usize, bind_group_id, bind_group, offsets) | |||
wgpu-core/src/command/render.rs
Outdated
| raw.begin_debug_marker(str::from_utf8(label).unwrap(), color) | ||
| }, | ||
| RenderCommand::PopDebugGroup => unsafe { | ||
| assert_eq!(state.debug_scope_depth, 0, "Can't pop debug group, because number of pushed debug groups is zero!"); |
wgpu-core/src/command/render.rs
Outdated
| pub extern "C" fn wgpu_render_pass_pop_debug_group(_pass: &mut RawPass) { | ||
| //TODO | ||
| pub unsafe extern "C" fn wgpu_render_pass_pop_debug_group(pass: &mut RawPass) { | ||
| pass.encode(&RenderCommand::PopDebugGroup {}); |
There was a problem hiding this comment.
I'm surprised this compiles? should be just PopDebugGroup without {}
|
@kvark Can't we squash the commits when we merging into master? |
we use bors on this project, and it doesn't squash |
wgpu-core/src/command/compute.rs
Outdated
| @@ -111,6 +130,11 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> { | |||
|
|
|||
| let mut pipeline_state = PipelineState::Required; | |||
|
|
|||
| let mut state = State { | |||
| binder: Binder::new(cmb.limits.max_bind_groups), | |||
There was a problem hiding this comment.
you forgot to remove the old binder (on line 122)
719: Implement debug marker support r=kvark a=krupitskas **Connections** Closes #697 **Description** Looks like because I've once pushed forward reset branch to my master, previous pull request #713 show that there no commits and it automatically was closed :/ **Testing** Not tested yet Co-authored-by: Nikita Krupitskas <[email protected]>
713: Expose texture format feature query (and update to latest wgpu) r=kvark a=Wumpf Exposes texture format feature query from gfx-rs#1112 Updating wgpu-core led to breaking change of moving get_swap_chain_preferred_format from device to adapter. Co-authored-by: Andreas Reich <[email protected]>
Connections
Closes #697
Description
Hey!
It's not ready yet (not compute support here and I have some questions).
Questions:
Testing
I will test after some questions will be clarified, I will record some debug markers and provide renderdoc capture.