-
Notifications
You must be signed in to change notification settings - Fork 202
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
[al] ColorSet import/export: support USD displayOpacity primvar (#1015) #250
[al] ColorSet import/export: support USD displayOpacity primvar (#1015) #250
Conversation
…1015) * ColorSet import/export: support display Opacity * refactor to separate out import of UVs and coloursets * better test scene
{ | ||
|
||
const UsdGeomPrimvar& primvar = *it; | ||
TfToken name, interpolation; |
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.
const std::vector<UsdGeomPrimvar> primvars = mesh.GetPrimvars();
for (auto it = primvars.begin(), end = primvars.end(); it != end; ++it)
{
const UsdGeomPrimvar& primvar = *it;
}
Above code can be refactored out to something a bit nicer:
for (const UsdGeomPrimvar& primvar : mesh.GetPrimvars())
{
}
or even better with auto
for (const auto& primvar : mesh.GetPrimvars())
{
}
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.
Really this is just sugar IMO....in this case I was refactoring existing code and therefore adhering to the existing style.
If it's covered by our upcoming style guide, perfectly happy to adhere to it, but for now I think we should avoid too much stylistic quibbling.(There is debate about whether the use of auto makes things more readable or not - not that I want to enter into it)
//---------------------------------------------------------------------------------------------------------------------- | ||
void MeshImportContext::applyPrimVars(bool createUvs, bool createColours) | ||
void MeshImportContext::applyUVs() | ||
{ | ||
const TfToken prefToken("pref"); | ||
const std::vector<UsdGeomPrimvar> primvars = mesh.GetPrimvars(); |
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.
const std::vector<UsdGeomPrimvar> primvars = mesh.GetPrimvars();
for (auto it = primvars.begin(), end = primvars.end(); it != end; ++it)
{
const UsdGeomPrimvar& primvar = *it;
}
Above code can be refactored out to something a bit nicer:
for (const UsdGeomPrimvar& primvar : mesh.GetPrimvars())
{
}
or even better with auto
for (const auto& primvar : mesh.GetPrimvars())
{
}
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.
Great, thank you @murphyeoin for the changes. LGTM.
Just a side note, there is a comment in line 276 that says Maya fails when the surface normal
is computed using the left handed rule. I don't have time to look into this further right now but would be nice to have an issue ticket on github to keep track of it.
Thanks @HamedSabri-adsk - we will take a look at the surface normal problem. We've made some small additions/improvements to this PR internally which we were just about to push - will create a separate PR for those |
UsdImaging uses displayOpacity in tandem with displayColor to set trasparency on objects.
In AL_USDMaya, the colorset export supported either:
This PR adds export of the "displayOpacity" attribute to the second case above, if we're exporting RGBA data
Output (manually simpliifed) now looks like this