-
Notifications
You must be signed in to change notification settings - Fork 203
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
MAYA-104987 Allows converting preview surface to Maya native shaders #707
Conversation
Also allows converting displayColor to other Maya shader nodes.
float scale(blinnFn.specularRollOff()); | ||
color /= scale; | ||
blinnFn.setSpecularColor(color); | ||
blinnFn.setSpecularRollOff(1.0f); |
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.
On export we scale the specular color by the rollOff. So on import we must set the rollOff to 1.
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 mentioned this in usdMaterialReader.cpp
where _OnReadAttribute()
gets called, but this code actually looks kind of odd until you realize that this is a "prep" step before we apply the values from USD, since we wouldn't have read a "rolloff" attribute, for example.
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.
Lgtm! Thank you. I did a minimal round trip tests with some of my assets and it is looking good so far. Nice to see the progress in there.
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.
Looks good to me! Just a bunch of mostly cosmetic notes.
Prior to PR Autodesk#707, the un-linearized transparency value was being used to compute a transparency "average", which was then used to author displayOpacity on the bound gprim if it did not already have an authored displayOpacity, as well as a displayOpacity input on the shader prim. It looks like an unintended side effect of Autodesk#707 was that it was made to use the linearized transparency for this purpose instead. Both of these behaviors seem somewhat dubious. It doesn't really seem like the shading mode exporter should be authoring displayOpacity of the bound gprim, since that is more a property of the geometry than the material. It also doesn't appear as if the displayOpacity input being authored on the shader prim is used for anything. Ignoring those two oddities for now, the changes here should address the "regression" introduced by Autodesk#707. We can revisit the behavior of the displayColors shading mode (or better yet work towards removing it!) in follow-up commits.
const float transparencyAvg = (mayaTransparency[0] + | ||
mayaTransparency[1] + | ||
mayaTransparency[2]) / 3.0f; | ||
const float transparencyAvg = (transparency[0] + | ||
transparency[1] + | ||
transparency[2]) / 3.0f; |
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.
Just wanted to flag here that this particular part of the change introduced a subtle "regression" from the old behavior. This is now using the linearized transparency
from the lambert shading node rather than the previously un-linearized mayaTransparency
.
I opened #751 which restores the previous behavior, but as I note there, this shading mode uses this transparencyAvg
value in a somewhat bizarre way. I don't expect it would have any practical impact on anyone exporting materials using the displayColors
shading mode.
Also allows converting displayColor to other Maya shader nodes.
Also fixes #693 by adding export of Maya standardSurface to displayColors.