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

Default Normal Shader: transform tangents with modelViewMatrix #18107

Merged
merged 1 commit into from Dec 10, 2019

Conversation

WestLangley
Copy link
Collaborator

resolves #18005.

As mentioned in #18005, an alternate approach is:

vec3 transformedTangent = transformDirection( objectTangent, modelViewMatrix ); // normalized

which we can implement later if needed.

@WestLangley WestLangley added this to the r112 milestone Dec 9, 2019
@mrdoob mrdoob merged commit a451341 into mrdoob:dev Dec 10, 2019
@mrdoob
Copy link
Owner

mrdoob commented Dec 10, 2019

Thanks!

@WestLangley
Copy link
Collaborator Author

@mrdoob It appears from your earlier commits that you are aware that this is not correct, either:

#ifdef USE_INSTANCING

	transformedNormal = mat3( instanceMatrix ) * transformedNormal;

#endif

I am not sure of a performant fix to this. One option is to say instancing does not support non-uniform scale in the instance matrices. That is not very appealing to me, though...

@WestLangley WestLangley deleted the dev_shader_transform_tangent branch December 10, 2019 01:36
@mrdoob
Copy link
Owner

mrdoob commented Dec 10, 2019

@mrdoob It appears from your earlier commits that you are aware that this is not correct

Hmm, maybe I was aware at some point, but I sure forgot 😅

@WestLangley
Copy link
Collaborator Author

You need a normal matrix for each instance.

@WestLangley
Copy link
Collaborator Author

WestLangley commented Dec 10, 2019

What you are doing is OK as long as there is no non-uniform scaling. But if one were to instance a grove of trees at varying heights (for example), non-uniform scaling support would be required.

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.

The normal matrix should not be used to transform tangents
2 participants