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

Add material with specializations for debugging meshes #7390

Closed

Conversation

torsteingrindvik
Copy link
Contributor

@torsteingrindvik torsteingrindvik commented Jan 28, 2023

Objective

Allows users to debug meshes by looking at UVs, and world {position, normals, tangents}.

Solution

Add a material which allows per-mesh viewing of the above attributes.

Changelog

Added

  • Added DebugMeshMaterial with pipeline specializations for attributes: UVs, world position, world normals, world tangents.
  • Added debug_mesh example to show usage of DebugMeshMaterials on a simple cube and on the flight helmet.

Discussion

Correctness

UVs (2nd debug helmet front-to-back)

debug_mesh.mp4

Compare the above Bevy video to the glTF Sample Viewer (see here):

bilde

This generally looks correct to me although the web viewer has darker colors.

Features

Perhaps local position, normals, tangents would be good to have.
Let me know. I haven't looked into if I have all the functions/data I need in the frag shader for that to be a simple calculation or if a different vertex shader would be needed then.

// This is a more useful default than to saturate/clamp.
return vec4<f32>(uv.x % 1.0, uv.y % 1.0, 0.0, 1.0);
#else
return vec4<f32>(0.0, 0.0, 0.0, 1.0);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is a sane default when a user wants to debug UVs (or other attributes) but there are none?
I opted for just showing black.

Copy link
Member

Choose a reason for hiding this comment

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

I would expect something garish and visibly different: I would go with hot pink as that's pretty common for "something has gone wrong".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using @superdump's shader now, which does indeed use pink.

@alice-i-cecile alice-i-cecile added C-Enhancement A new feature A-Rendering Drawing game state to the screen labels Jan 28, 2023
@@ -13,7 +13,11 @@ repository = "https://github.com/bevyengine/bevy"
rust-version = "1.67.0"

[workspace]
exclude = ["benches", "crates/bevy_ecs_compile_fail_tests", "crates/bevy_reflect_compile_fail_tests"]
exclude = [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just a format-on-save whitespace change.

Cargo.toml Outdated
name = "Debug mesh"
description = "Debug meshes by displaying UVs, world normals, etc."
category = "3D Rendering"
wasm = false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

wasm = false is this an opt-out of having the example on Bevy's homepage?
I haven't tried the example on a wasm build, let me know what to put here.

Copy link
Member

Choose a reason for hiding this comment

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

That's correct. I'd set this to true, it's visually impressive and good to verify.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Set to true now.

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Looks awesome! Can we enable this as a mode in our scene_viewer tool?

@superdump
Copy link
Contributor

I did a PR a long time ago that focused more on screen-wide PBR debug stuff like this. #5398 I'm not sure which I feel is better, or if both are desirable. I feel like a whole-screen solution is useful, just as a per-mesh entity solution is useful. Perhaps they can share some parts or something. Also things have moved on a lot since my PR. It needs updating and there are many more things to debug now. :) Would you perhaps be up for putting together a combined solution?

@SneakyBerry
Copy link
Contributor

Is it technically possible to represent UV map like this?
image

@torsteingrindvik
Copy link
Contributor Author

torsteingrindvik commented Jan 28, 2023

@alice-i-cecile thanks for the review comments, I'll address them after figuring out where to take this.

@superdump Locally now I've merged your PR into this one (so git history should be preserved). Adapted it a bit since there have been a few changes, was not too much work. So now the scene viewer works when cycling all of the enum variants from your PR.

Since yours was a lot more thorough it makes sense to use what you made.

I'm thinking I will try to do something similar to the WireframePlugin where it's possible to both have a global setting and a per-mesh setting.
(EDIT: I'll push to this PR with all of this when I figure that out. If so this PR will combine both of ours. Hopefully that's ok)
(EDIT 2: I realize the WireframePlugin does not replace meshes, it just draws on top. So that method is not applicable I think)

Another thing: If we have the scene viewer, do we still want the example I added or should I remove it?

@torsteingrindvik
Copy link
Contributor Author

torsteingrindvik commented Jan 28, 2023

@alice-i-cecile Merged in @superdump's PR so the scene viewer is included now.

@superdump From your PR the main changes I've made is to update the PBR functions shader slightly since the prepare_normals function was replace, and also I made PbrDebug an optional resource instead of having a PbrDebug::None variant.

To attempt getting the best of both worlds I added PbrDebugMaterial to allow per-mesh choices in the same way I did in this PR originally, but it's all pink now even though the shader defs are added:
bilde

So I'll debug that.
(EDIT: Turned out the wrong frag shader was in use.)

However the scene viewer works:
bilde

Also even though I did a git merge superdump/<branch> directly it did not preserve you as author @superdump 😢
Not sure why. Let me know if there's some git magic I can do here.

Update:
bilde

UVs, depth, interpolated normals, interpolated tangets work.
"TangentSpaceNormalMap", "NormalMappedNormal", "ViewSpaceNormalMappedNormal" are pink.
Others, such as e.g. "BaseColor" crash:

    In Device::create_render_pipeline
      note: label = `opaque_mesh_pipeline`
    error matching FRAGMENT shader requirements against the pipeline
    shader global ResourceBinding { group: 1, binding: 0 } is not available in the layout pipeline layout
    binding is missing from the pipeline layout

Not sure why, and I don't get how it works for scene viewer. Perhaps the cache invalidation is relevant?

@JMS55 JMS55 self-requested a review January 28, 2023 16:19
@superdump
Copy link
Contributor

superdump commented Jan 30, 2023

I'm thinking I will try to do something similar to the WireframePlugin where it's possible to both have a global setting and a per-mesh setting. (EDIT: I'll push to this PR with all of this when I figure that out. If so this PR will combine both of ours. Hopefully that's ok) (EDIT 2: I realize the WireframePlugin does not replace meshes, it just draws on top. So that method is not applicable I think)

That's fine, just retain attribution (i.e. add my stuff in its own commit and make me the author of the commit with git commit --author="Robert Swain <robert.swain@gmail.com>") I see you tried to merge. That should have worked. I'll try to take a look at the git history of the branch.

Another thing: If we have the scene viewer, do we still want the example I added or should I remove it?

I think having an example is good too. The scene_viewer, being a tool, is perhaps not the first place to think to look, whereas the examples are a de facto go to to look up how to do stuff.

@superdump
Copy link
Contributor

This is super cool! The ability to have multiple instances of a mesh side-by-side with different debug views at the same time is surely incredibly useful for hacking away on shaders!

@superdump
Copy link
Contributor

Could you add:

into the PR description?

@torsteingrindvik
Copy link
Contributor Author

@superdump updated the description.
Rebased and amended the merge, look slike the attribution is correct now. Thanks for the tip on discord.

I still need to figure out why only some debug modes work when I'm using materials.

@superdump
Copy link
Contributor

Merging #7431 so you can build on top of that now to simplify the ifdeffery in this one. :)

@torsteingrindvik
Copy link
Contributor Author

Thanks! I'll revisit this in the next couple of days and apply that PR 👍

torsteingrindvik and others added 8 commits February 5, 2023 11:02
Signed-off-by: Torstein Grindvik <torstein.grindvik@nordicsemi.no>
Signed-off-by: Torstein Grindvik <torstein.grindvik@nordicsemi.no>
Co-authored-by: Torstein Grindvik <torstein.grindvik@nordicsemi.no>
Signed-off-by: Torstein Grindvik <torstein.grindvik@nordicsemi.no>
…ommits merged.

Signed-off-by: Torstein Grindvik <torstein.grindvik@nordicsemi.no>
Signed-off-by: Torstein Grindvik <torstein.grindvik@nordicsemi.no>
…. Material only shows pink, have to debug.

Signed-off-by: Torstein Grindvik <torstein.grindvik@nordicsemi.no>
Signed-off-by: Torstein Grindvik <torstein.grindvik@nordicsemi.no>
Signed-off-by: Torstein Grindvik <torstein.grindvik@nordicsemi.no>
Signed-off-by: Torstein Grindvik <torstein.grindvik@nordicsemi.no>
Signed-off-by: Torstein Grindvik <torstein.grindvik@nordicsemi.no>
Signed-off-by: Torstein Grindvik <torstein.grindvik@nordicsemi.no>
@torsteingrindvik
Copy link
Contributor Author

Got rid of a lot of #endifs in the pbr_debug.wgsl shader now, so that's in place.

However I'm a bit stuck on understanding why the material approach does not work (it works only for the few shown in the images).

I'll compare my understanding of the global/resources vs. the material approach:

The global/resource based version checks if the PbrDebug resource changes, if so set this on the mesh pipeline.
Then invalidate the SpecializedMeshPipelines cache for MaterialPipeline<StandardMaterial>. I'm not sure I understand this entirely, but I think it's just because the key must change before a new pipeline is created, and the PbrDebug field is not part of the key.
(Please let me know if it's more nuanced).
And then with the new pipeline in action, the proper shader definitions are added from PbrDebug.

By this logic I created PbrDebugMaterial, and used PbrDebug as the bind_group_data, which lets me specialize this material on which variant of PbrDebug is used.
The shader defs are added in the specialization of the material.
Since now the PbrDebug is part of the key, each variant used should result in a new custom pipeline, and therefore no cache invalidation is necessary here.

So intuitively I thought both approaches should work since the goal is simply to get the right shader defs in place.
Any insight on why this is not the case would be very helpful.

@superdump
Copy link
Contributor

At least at first glance, your description sounds reasonable. I'll take a look at the PR locally.

@superdump
Copy link
Contributor

The problem is that the PbrDebugMaterial is trying to use the main PBR shader without using any of the StandardMaterial data nor shader defs. That was simpler for the way I did it because that I didn't have to deal with materials as I managed the shader defs globally. Hmm, we really need a good way of extending / modifying the StandardMaterial without copying it, modifying it, and then maintaining that.

To be more specific and concrete, TangentSpaceNormalMap for example needs to have vertex tangents, vertex normals, vertex uvs, and a normal map texture. I noticed that STANDARDMATERIAL_NORMAL_MAP was missing and then realised that the PbrDebugMaterial only contains PbrDebug and none of the StandardMaterial bindings.

Hmm, I think to avoid duplication, the material approach will need some more thought. I'm not yet sure how to solve it without just building it into StandardMaterial, which we could do...

@torsteingrindvik
Copy link
Contributor Author

Thanks for looking into that.

Since we currently don't have a clear image of how to solve that, should I perhaps split this PR up?
I could rebase and keep your PR here (PbrDebug and scene viewer stuff), then split the PbrDebugMaterial related stuff into a draft PR until we know more.
Such that we have something to merge before 0.10.

@alice-i-cecile alice-i-cecile removed this from the 0.10 milestone Feb 13, 2023
@alice-i-cecile
Copy link
Member

I don't think this is close enough to shippable to make the 0.10 release, but feel free to prove me wrong ;)

@torsteingrindvik
Copy link
Contributor Author

I see the PBR shaders have changed a lot, seems the PBR pipeline has gotten more structured and extensible in a year which is nice.
But I don't have a good overview of the pipeline now so I'll close- I think a re-do of this PR would likely be a lot better.
(If anyone sees this and would like to continue from a rebase instead of from scratch of course feel free to.)

@alice-i-cecile alice-i-cecile added the S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! label Nov 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Enhancement A new feature S-Adopt-Me The original PR author has no intent to complete this work. Pick me up!
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Add PBR debug render modes
4 participants