-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 example to test normals and tangents (and fix WebGL) #5924
Add example to test normals and tangents (and fix WebGL) #5924
Conversation
I tried to find a way to fix these issues but I'm not sure where to start. Should this be fixed in the GLB parser or in somewhere else? If anyone can point me in the right direction I can try and fix the issues in this pull request as well. |
Oh that's interesting. I never realised our lighting basis is wrong on the back faces. (We were aware of WebGPU issues though). Thanks for pointing this out! |
@slimbuck if you point me in the right direction, I can try and fix it. Are the normals and tangents calculated at a specific place? I couldn't easily find it as I'm not familiar enough with the source yet. |
I would have expected backfaces to be handled by this bit of code here: https://github.com/playcanvas/engine/blob/main/src/scene/shader-lib/programs/lit-shader.js#L989 The actual tangents and binormals are calculated in this shader chunk: |
I have fixed the calculations for both WebGL and WebGPU. I have also fixed the WebGPU version looking so weird, this was caused by setting |
#ifdef TWO_SIDED_LIGHTING | ||
#ifdef WEBGPU | ||
dTBN = mat3(T * invmax, -B * invmax, gl_FrontFacing ? -normal : normal); | ||
#else | ||
dTBN = mat3(T * invmax, -B * invmax, gl_FrontFacing ? normal : -normal); | ||
#endif | ||
#else | ||
#ifdef WEBGPU | ||
dTBN = mat3(T * invmax, -B * invmax, -normal); | ||
#else | ||
dTBN = mat3(T * invmax, -B * invmax, normal); | ||
#endif | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Webgpu lighting space should not be different to webgl, so this normal negation is terribly confusing.
Are textures inverted (upside down) on webgpu vs webgl @mvaligursky? (If so then tangents and binormals as calculated would be inverted).
I think we need to get to the bottom of why normals needs flipping here and fix it at the source. My suspicion is we have an inverted transform somewhere else in the graphics pipeline under webgpu.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, I also noticed that my solution causes other problems with the lighting. I haven't been able to find a good solution. If at any other place I modify the normals it always messes up other things. My guess right now is that it's not the normals being flipped but the result in dTBN
being used incorrectly somewhere.
For now I have removed this code. I'm not sure if you want to merge this as a fix to the WebGL backfaces and to introduce an example that shows what is wrong in the WebGPU pipeline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I just noticed, looking in lit-shader.js, it seems we are already attempting to handle the normal for double-sided lighting case at https://github.com/playcanvas/engine/blob/main/src/scene/shader-lib/programs/lit-shader.js#L989. This calc must have a 🐛...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, it seems the issue is that it was handled in the wrong place. I have added a negatively scaled mesh to the example and I have gone though all variations of the shader to maker sure they all work correctly now.
2aa2bbd
to
b1b8c14
Compare
I'd be definitely great to have this example as a test for normals / tangents. |
b1b8c14
to
96dddd5
Compare
96dddd5
to
e65d942
Compare
e65d942
to
5f7b0bf
Compare
I have rebased the example on main with the new examples refactor. What needs to happen to get this merged now? |
Thanks for doing that, @erikdubbelboer - we'll take another look ASAP! 😄 |
* @type {import('../../../../types.mjs').ExampleConfig} | ||
*/ | ||
export default { | ||
WEBGPU_ENABLED: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WEBGPU_ENABLED: true, | |
HIDDEN: true, | |
WEBGPU_ENABLED: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm suggesting to hide it from public .. this is more of an internal example for us to validate how things work, and not an example to show the public how to use the engine.
It seems the description only shows 'before' state, but not 'after' state - could you please update it. |
Hi @erikdubbelboer, Looking at the changes, I think it makes sense to handle twoSidedLighting in just one place - after dTBN has been calculated. This is probably best done in a new shader chunk. So I went ahead and implemented this approach in main...slimbuck:erik-normals-and-tangents. Could you take a look and let me know what you think? This approach also means we probably fix the case where If you're happy with this I can push directly to your branch? Thanks! |
This shows that normals and tangents are not used correctly in both WebGL2 and WebGPU.
76b87fd
to
aa3707e
Compare
aa3707e
to
b711709
Compare
@slimbuck great, I have merged your changes. @mvaligursky I think this pull is ready now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is much simpler now, thanks!
This fixes normals and tangents in the WebGL2 shader. WebGPU is still broken, see: #5735
See also https://github.com/KhronosGroup/glTF-Sample-Models/tree/master/2.0/NormalTangentTest
I confirm I have read the contributing guidelines and signed the Contributor License Agreement.