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

[usdMaya] Adding an attribute converter for UsdGeom purpose. #215

Merged

Conversation

sirpalee
Copy link
Contributor

@sirpalee sirpalee commented May 26, 2017

Description of Change(s)

The change adds an attribute converter for the purpose attribute on UsdGeomImageable. This makes setting up proxies for usdview / katana way simpler, and users don't have to implement it for themselves.

@sirpalee sirpalee changed the title [usdMaya] Setting up an attribute converter for UsdGeom purpose. [usdMaya] Adding an attribute converter for UsdGeom purpose. May 26, 2017
@jtran56
Copy link

jtran56 commented Jun 2, 2017

Filed as internal issue #147140.

@pmolodo pmolodo force-pushed the pr/convert_purpose_attribute branch from 25e7a00 to 6d49352 Compare December 7, 2017 01:37
Copy link
Contributor

@sunyab sunyab left a comment

Choose a reason for hiding this comment

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

Passing along some notes from our sets team. Thanks!

@@ -0,0 +1,84 @@
//
// Copyright 2016 Pixar
Copy link
Contributor

Choose a reason for hiding this comment

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

2018 :)


TF_DEFINE_PRIVATE_TOKENS(
_tokens,
(USDGeom_purpose)
Copy link
Contributor

Choose a reason for hiding this comment

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

We think naming the attribute just "USD_purpose" would be preferable, since it'd be more consistent with the other attributes the importer and exporter look for. Is it important that we indicate that this is a UsdGeom schema thing in the name of this token?

@@ -0,0 +1,83 @@
#!/pxrpythonsubst
#
# Copyright 2016 Pixar
Copy link
Contributor

Choose a reason for hiding this comment

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

2018


def testImport(self):
"""
Tests that the built-in metadata attribute converters can import
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we update the docstring here as well?

from pxr import Usd, UsdGeom


class testUsdMetadataAttributeConverters(unittest.TestCase):
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we update the test fixture name to match the test name?


# Testing that there is no authored attribute
geom4 = UsdGeom.Imageable(newUsdStage.GetPrimAtPath('/World/pCube4'))
self.assertEqual(geom4.GetPurposeAttr().HasAuthoredValueOpinion(), False)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above, this could be: self.assertFalse(geom4.GetPurposeAttr().HasAuthoredValueOpinion()

self.assertEqual(cmds.getAttr('pCube3.USDGeom_purpose'), 'proxy')

# pCube4 does not have a purpose attribute
self.assertEqual(cmds.objExists('pCube4.USDGeom_purpose'), False)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we change this to use self.assertFalse(...) instead?

Signed-off-by: Pal Mezei <palm@lumapictures.com>
@sirpalee sirpalee force-pushed the pr/convert_purpose_attribute branch from 6d49352 to 64948e5 Compare January 12, 2018 04:01
@sirpalee
Copy link
Contributor Author

@sunyab thanks for the feedback! I addressed all the concerns, please let me know if you need anything else change.

@pixar-oss pixar-oss merged commit 64948e5 into PixarAnimationStudios:dev Jan 18, 2018
pixar-oss added a commit that referenced this pull request Jan 18, 2018
[usdMaya] Adding an attribute converter for UsdGeom purpose.
@sirpalee sirpalee deleted the pr/convert_purpose_attribute branch January 22, 2018 06:03
@timthehoff
Copy link

This is great! I just ran into this feature by accident. Could it be added to the documentation?

@spiffmon
Copy link
Member

spiffmon commented Mar 1, 2018 via email

@timthehoff
Copy link

timthehoff commented Mar 1, 2018 via email

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.

6 participants