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

Re-normalizes interpolated normals and deals with non-uniform scale #65

Closed
wants to merge 2 commits into from

Conversation

ziriax
Copy link
Contributor

@ziriax ziriax commented Feb 3, 2018

When porting your shader to our Maya2glTF project, we noticed two problems that this pull request should address:

  • when no normal map is present, the shading is wrong. This is best noticed when choosing the InterpolatedNormalsTest from the models menu, and disabling IBL. Without this PR, when rotating the camera, you'll notice the specular highlight to unevenly brighten the edges of the triangles, as if the model is more or less flat shaded. The reason was that the normals are linearly interpolated by the GPU, and hence do not have unit length anymore. So the pixel-shader must re-normalize the normals. This was only correctly done when a normal map was present.

  • when non-uniform scaling was applied, the normals were not computed correctly. Again this can be easily seen by choosing the NonUniformScalingTest from the models menu, and disabling IBL. Without this PR you'll notice a very weird reflection. I tried to document the math behind this, because this is so tricky and weird, I hope the comments can be of some educational value.

Most likely I made some mistakes, I'll be happy to address them.

Thanks for any feedback,
Peter Verswyvelen

@ziriax ziriax changed the title Re-normalizes interpolated normals and deals with non-uniformly scale Re-normalizes interpolated normals and deals with non-uniform scale Feb 3, 2018
…ents (slightly smoothed in Maya to reduce vertex splits), to get correct normal mapping and reflections
@@ -1,204 +1,235 @@
{
"accessors" : [
"asset": {
"generator": "Maya2glTF",
Copy link
Member

Choose a reason for hiding this comment

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

The DamagedHelmet model here comes from the sample model repository, and prior to that it was downloaded from SketchFab back before glTF 2.0 was even published. It does need fixing, and possibly it should be simply re-exported to glTF using SketchFab's current glTF exporter.

But in any case, I don't want this copy of DamagedHelmet to be different from the one on glTF-Sample-Models, regardless of where we get the fixes from.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad, it seems I forgot to make a patch-request branch. This new helmet export was never intended to be part of this patch, mea culpa

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see, this is off your master branch. You may want to close this whole PR and open a new one from a patch branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do so now.

@emackey
Copy link
Member

emackey commented Feb 21, 2018

@snagy Do you have any time to look at the proposed changes to the normal matrix math here?

@ziriax Thanks for the contribution!

@ziriax
Copy link
Contributor Author

ziriax commented Feb 21, 2018

I will created a dedicated PR branch for this issue.

@ziriax ziriax closed this Feb 21, 2018
emackey pushed a commit that referenced this pull request Mar 3, 2021
Fix sync issue inside each individual animation (GSVN-157)
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.

None yet

2 participants