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

Fix testrender sphere tangents, per MaterialX suggestion #1465

Merged
merged 2 commits into from
Feb 19, 2022

Conversation

lgritz
Copy link
Collaborator

@lgritz lgritz commented Feb 16, 2022

Needed to update two reference output images.

Signed-off-by: Larry Gritz [email protected]

@fpsunflower
Copy link
Contributor

Actually both current and proposed code look wrong, there is at the minimum a missing factor of the sphere radius that should be in there.

Beyond that - there are some choices to be made about handedness. Does MaterialX impose a convention here?

dPdu is intended to be the derivative of P with respect to u. In fact, strictly speaking dPdu and dPdv are both redundant since they can be computed from P,u,v directly with the Dx and Dy ops and the chain rule.

For instance, depending on if you use the v or the 1-v convention for texture lookups, you may want to negate dPdv.

When this code is updated, I think we should beef up the comments to explain these details at the minimum, showing the implied surface parameterization more directly.

@lgritz
Copy link
Collaborator Author

lgritz commented Feb 16, 2022

Does somebody else want to take a whack at this one who has thought about the sphere parameterization problem and what convention it is that we are trying to follow? I feel like maybe I should not be making random changes here based on a partial overhearing of an external conversation that I've only been vaguely following.

@lgritz
Copy link
Collaborator Author

lgritz commented Feb 17, 2022

I've updated the PR with what I believe are the correct sphere derivatives (dPdu & dPdv), derived from first principles.
I created a new test to be sure that the spheres and their texture coords are oriented the way we think they are.
The old code was definitely wrong (everybody agrees that dPdu.y had better be 0, because as you move in u, your latitude doesn't change).

I don't swear that the sphere parametric derivatives match the method you were using in the MaterialX examples for computing a tangent frame give the normal -- I haven't tried to work out the math to prove that they are or aren't the same result. Assuming nobody finds a flaw in my math here (totally possible!), then if they still don't match, I think the solution is just to realize that they're computing different directions, and that's fine -- you should alter the OSL shaders you are using to compute the tangents the same way you do for GLSL.

@jstone-lucasfilm @bernardkwok

Add render-uv test to check

Signed-off-by: Larry Gritz <[email protected]>
@lgritz
Copy link
Collaborator Author

lgritz commented Feb 17, 2022

Updated to fix sign flip problem that Jonathan pointed out on Slack.

Copy link
Member

@jstone-lucasfilm jstone-lucasfilm left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Copy link

@bernardkwok bernardkwok left a comment

Choose a reason for hiding this comment

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

Me as well.

Signed-off-by: Larry Gritz <[email protected]>
Copy link
Contributor

@fpsunflower fpsunflower left a comment

Choose a reason for hiding this comment

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

LGTM

@lgritz lgritz merged commit 89207ef into AcademySoftwareFoundation:main Feb 19, 2022
@lgritz lgritz deleted the lg-testrender branch February 19, 2022 18:19
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.

4 participants