[Merged by Bors] - Add PBR textures by mtsr · Pull Request #1632 · bevyengine/bevy

mtsr · 2021-03-12T10:13:38Z

This PR adds normal maps on top of PBR #1554. Once that PR lands, the changes should look simpler.

Edit: Turned out to be so little extra work, I added metallic/roughness texture too. And occlusion and emissive.

mtsr · 2021-03-12T10:16:46Z

crates/bevy_render/src/shader/shader_reflect.rs

                for input_variable in module.enumerate_input_variables(None).unwrap() {
                    if input_variable.name == GL_VERTEX_INDEX
                        || input_variable.name == GL_INSTANCE_INDEX
+                        || input_variable.name == GL_FRONT_FACING
                    {
                        continue;
                    }


I might misunderstand the point of this condition, if so I'd love to learn. But wouldn't a more long-term solution be (snipped some use changes for clarity):

diff --git a/crates/bevy_render/src/shader/shader_reflect.rs b/crates/bevy_render/src/shader/shader_reflect.rs index 238da1ef..95761fed 100644 --- a/crates/bevy_render/src/shader/shader_reflect.rs +++ b/crates/bevy_render/src/shader/shader_reflect.rs @@ -31,8 +32,9 @@ impl ShaderLayout { // obtain attribute descriptors from reflection let mut vertex_attributes = Vec::new(); for input_variable in module.enumerate_input_variables(None).unwrap() { - if input_variable.name == GL_VERTEX_INDEX - || input_variable.name == GL_INSTANCE_INDEX + if input_variable + .decoration_flags + .contains(ReflectDecorationFlags::BUILT_IN) { continue; }

Oooh that does seem like a win / appears to do what we want it to.

mtsr · 2021-03-12T12:12:52Z

2021-03-12_11-37-48.mp4

mtsr · 2021-03-13T20:10:16Z

Rebased on pbr.

mtsr · 2021-03-19T22:44:21Z

Rebased on the latest pbr.

mtsr · 2021-03-20T08:52:15Z

And rebased onto main now that #1554 has been merged.

crates/bevy_render/src/mesh/mesh.rs

crates/bevy_pbr/src/render_graph/pbr_pipeline/pbr.frag

jakobhellermann · 2021-03-24T17:01:29Z

Should the standard shapes like cube or sphere provide the tangent vertex attribute?

crates/bevy_pbr/src/material.rs

alice-i-cecile · 2021-03-24T18:24:12Z

crates/bevy_pbr/src/render_graph/pbr_pipeline/pbr.frag

    vec3 specular_ambient = EnvBRDFApprox(F0, perceptual_roughness, NdotV);

-    output_color.rgb = light_accum + (diffuse_ambient + specular_ambient) * AmbientColor;
+    output_color.rgb = light_accum;


Tiny comments here would make it easier for folks new to rendering to figure out exactly what's happening here.

See the // tone_mapping comment just below.

examples/3d/pbr_textures.rs

examples/README.md

pkupper · 2021-03-24T21:17:34Z

Looking at the image below, there is clearly something wrong with the normal mapping in the example.

One possible mistake I found is that in the fragment shader the sample taken from the normal map is not converted into the -1.0 to 1.0 range before being transformed to world space. But that alone doesn't seem to fix the issue.

alice-i-cecile · 2021-03-24T22:00:16Z

Transferring @pkupper's comment from Discord for discoverability:

Another thing that's missing is calculating normals and tangents if they are not present in the gltf. Tried a normal map sample gltf for easier debugging (https://github.com/KhronosGroup/glTF-Sample-Models) and it didn't load due to missing tangents. The gltf spec defines how both should be calculated (https://github.com/KhronosGroup/glTF/tree/master/specification/2.0#meshes). But that is probably something for another PR.

mtsr · 2021-03-25T21:01:01Z

Should the standard shapes like cube or sphere provide the tangent vertex attribute?

Possibly. Do we want to pull that into this PR? (neutral question) It has both pros and cons, I think, but I guess it does make sense to make this more fit for general use, rather than having to add that in a separate PR.

Transferring @pkupper's comment from Discord for discoverability:

Another thing that's missing is calculating normals and tangents if they are not present in the gltf.

This is actually pretty similar as above, except for externally loaded models. I guess it makes sense too.

Tried a normal map sample gltf for easier debugging (https://github.com/KhronosGroup/glTF-Sample-Models) and it didn't load due to missing tangents. The gltf spec defines how both should be calculated (https://github.com/KhronosGroup/glTF/tree/master/specification/2.0#meshes). But that is probably something for another PR.

After @pkupper pointed out the other samples, I found there's actually a really good sample for testing this PR, which is NormalTangentMirror. I considered adding an example with these models, but it would add quite a bit of extra size, which I'm not sure is good. Maybe in a separate repo? Later, anyway.

mtsr · 2021-03-25T21:26:49Z

We're hitting a minor snag, where GltfLoader passes off the textures to be loaded as a Vec<AssetPath>, without any texture information. Specifically, we cannot tell AssetServer to create Linear textures from these, rather than Srgb.

The effect of this is that Normal, Metallic, Roughness etc textures get gamma-incorrected during sampling, basically doing a Srgb->linear conversion on already linear textures.

I don't really know where to start fixing this. If somebody is able to help out, so I don't have to also dig into the Asset system, please do. Otherwise I may look into it this weekend or next week.

jakobhellermann · 2021-03-25T21:52:02Z

I think a second PR is fine for adding the missing tangent vertex attributes.

mtsr · 2021-03-26T09:18:39Z

Rebase on #1762 in progress...

Load textures from gltf as linear when needed. This is for #1632, but can be done independently and won't have any visible impact before. * during iteration over materials, register textures that need to be loaded as linear * during iteration over textures * directly load bytes from external files instead of adding them as dependencies in the load context * configure the texture the same way for buffered and external textures * if the texture is linear rgb, set as linear rgb

cart · 2021-03-26T21:00:19Z

bors r+

This PR adds normal maps on top of PBR #1554. Once that PR lands, the changes should look simpler. Edit: Turned out to be so little extra work, I added metallic/roughness texture too. And occlusion and emissive. Co-authored-by: Carter Anderson <[email protected]>

bors · 2021-03-26T21:17:54Z

Pull request successfully merged into main.

Build succeeded:

mtsr commented Mar 12, 2021

View reviewed changes

mtsr changed the title ~~Add normal maps~~ Add normal maps + metallic/roughness texture Mar 12, 2021

mtsr mentioned this pull request Mar 12, 2021

[Merged by Bors] - Rebase of existing PBR work #1554

Closed

Ratysz added C-Feature A new feature, making something new possible A-Rendering Drawing game state to the screen labels Mar 12, 2021

inodentry mentioned this pull request Mar 13, 2021

Is shadow or global illumination on 0.5? #1637

Closed

mtsr force-pushed the pbr-normal-maps branch from 0a6d008 to 6e315e9 Compare March 13, 2021 20:10

mtsr force-pushed the pbr-normal-maps branch from 6e315e9 to 2078f0c Compare March 19, 2021 22:42

mtsr force-pushed the pbr-normal-maps branch from 2078f0c to 9f028dc Compare March 20, 2021 08:51

jakobhellermann reviewed Mar 24, 2021

View reviewed changes

crates/bevy_render/src/mesh/mesh.rs Outdated Show resolved Hide resolved

jakobhellermann reviewed Mar 24, 2021

View reviewed changes

crates/bevy_pbr/src/render_graph/pbr_pipeline/pbr.frag Outdated Show resolved Hide resolved

alice-i-cecile added the help wanted label Mar 24, 2021

alice-i-cecile reviewed Mar 24, 2021

View reviewed changes

crates/bevy_pbr/src/material.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed Mar 24, 2021

View reviewed changes

examples/3d/pbr_textures.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed Mar 24, 2021

View reviewed changes

examples/README.md Outdated Show resolved Hide resolved

alice-i-cecile reviewed Mar 24, 2021

View reviewed changes

examples/README.md Outdated Show resolved Hide resolved

mtsr changed the title ~~Add normal maps + metallic/roughness texture~~ Add PBR textures Mar 25, 2021

mockersf mentioned this pull request Mar 25, 2021

[Merged by Bors] - gltf: load normal and occlusion as linear textures #1762

Closed

mtsr force-pushed the pbr-normal-maps branch from 368a9d3 to eb8d788 Compare March 26, 2021 11:02

mtsr and others added 14 commits March 26, 2021 13:37

Add normal maps to pbr + example

d9f3e7b

Deal with doublesided materials

d7d3da9

Add metallic/roughness texture

589c4af

Add occlusion

b82a10e

Add emissive

c8aeaf5

Add pbr_textures example to README

8f0d0f7

Add ATTRIBUTE_TANGENT doc comment

be39bed

Fix emissive texture use

c2e0ac2

Improve emissive alpha channel comment

71ea60c

Rename Rotator component Rotates

ffee787

Improve example README

ed834fb

TxN => NxT and convert texture_normal to [-1,1]

3983fc1

standardize todos

ba5f765

merge pbr_textures and load_gltf examples

00b03a7

cart force-pushed the pbr-normal-maps branch from 78d4b64 to 00b03a7 Compare March 26, 2021 20:37

bors bot changed the title ~~Add PBR textures~~ [Merged by Bors] - Add PBR textures Mar 26, 2021

bors bot closed this Mar 26, 2021

Uh oh!

Conversation

mtsr commented Mar 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mtsr Mar 12, 2021

Choose a reason for hiding this comment

Uh oh!

cart Mar 13, 2021

Choose a reason for hiding this comment

Uh oh!

mtsr commented Mar 12, 2021

Uh oh!

mtsr commented Mar 13, 2021

Uh oh!

mtsr commented Mar 19, 2021

Uh oh!

mtsr commented Mar 20, 2021

Uh oh!

Uh oh!

Uh oh!

jakobhellermann commented Mar 24, 2021

Uh oh!

Uh oh!

alice-i-cecile Mar 24, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pkupper commented Mar 24, 2021

Uh oh!

alice-i-cecile commented Mar 24, 2021

Uh oh!

mtsr commented Mar 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mtsr commented Mar 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jakobhellermann commented Mar 25, 2021

Uh oh!

mtsr commented Mar 26, 2021

Uh oh!

cart commented Mar 26, 2021

Uh oh!

bors bot commented Mar 26, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mtsr commented Mar 12, 2021 •

edited

Loading

mtsr commented Mar 25, 2021 •

edited

Loading

mtsr commented Mar 25, 2021 •

edited

Loading