Add Python binding for UsdValidatorMetadata
#3232
Conversation
|
@nvmkuruc for vis. |
roggiezhang-nv
force-pushed
the
add_bind_for_usdvalidator_meta
branch
from
c98e8e4
to
fc17b6e
Compare
|
Filed as internal issue #USD-9985 |
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
pxr/usd/usd/wrapValidator.cpp
Outdated
| .add_property("name", &_GetMetadataName, &_SetMetadataName) | ||
| .add_property("plugin", &_GetMetadataPlugin, &_SetMetadataPlugin) | ||
| .add_property("keywords", make_function( | ||
| &_GetMetadataKeywords, return_value_policy<TfPySequenceToList>()), _SetMetadataKeywords) |
There was a problem hiding this comment.
I was having some good discussion with Alex here at Pixar about python APIs and how it might be confusing for a python developer to not be able to do something like: metadata.keywords[0] = "foo". We also discussed and have many examples of this in our code base where we wrap something as a python property if value of the type is immutable in python.
So that gets me to, will it make more sense to provide a GetKeywords/SetKeywords (and similarly for schemaTypes below) which returns a list and takes in a list respectively? (We have example of this here: https://github.com/PixarAnimationStudios/OpenUSD/blob/release/pxr/usd/usd/wrapAttribute.cpp#L123-L124
But now this results in inconsistency with the cpp API, which at present doesn't provide any member functions on the UsdValidationMetadata struct. Do you think it will be possible for you to add these methods also for consistency?
Thanks
There was a problem hiding this comment.
That makes sense to me. I'll change it to Set/Get function instead. Thanks.
There was a problem hiding this comment.
@tallytalwar I have changed it to use getter/setter to access keywords and schemaTypes, but not for cpp struct. One question is that should I keep keywords and schemaTypes as private or add public getter/setter for them only? That adds another inconsistency that it has two set of methods to access the two members if I keep them public. While it breaks back compatibility if I moved them into private.
There was a problem hiding this comment.
What about using a vector_indexing_suite to preserve consistency with the CPP API?
There was a problem hiding this comment.
Rather than use vector_indexing_suite or adding explicit get/set methods, I think I'd actually argue in favor of just making UsdVaidatorMetadata immutable in python. In general, the usage for UsdValidatorMetadata is to just reflect and inspect metadata described in C++ validator plugins, not editing in python.
There's already a keyword argument interface for easy construction for testing. keywords and schemaTypes could use TfPySequenceToTuple.
There was a problem hiding this comment.
Yes, that makes sense to me if we don't need to register validator in python.
There was a problem hiding this comment.
UsdValidatorMetadata is to just reflect and inspect metadata described in C++ validator plugins, not editing in python.
Hmm, I think we should allow registering of validators from python also. So I can see we providing API for this https://github.com/PixarAnimationStudios/OpenUSD/blob/release/pxr/usd/usd/validationRegistry.h#L226 in python.
For this usecase, we will have to have immutable metadata constructs in python, no? @nvmkuruc
There was a problem hiding this comment.
Even when registering validators in python, the keyword arg constructor @roggiezhang-nv provides for validator metadata is probably the most pythonic way to initialize and somewhat symmetrical to aggregate initialization of structs with designated initializers in C++.
There was a problem hiding this comment.
Yup I missed the constructor! Agreed with your statements @nvmkuruc
There was a problem hiding this comment.
I'll make those properties immutable then.
pxr/usd/usd/wrapValidator.cpp
Outdated
| .add_property("keywords", make_function( | ||
| &_GetMetadataKeywords, return_value_policy<TfPySequenceToList>()), _SetMetadataKeywords) | ||
| .def_readwrite("doc", &UsdValidatorMetadata::doc) | ||
| .add_property("schemaTypes", make_function( |
There was a problem hiding this comment.
Same note about schemaTypes also.
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Thanks @roggiezhang-nv , syncing this in now. |
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Description of Change(s)
Add Python binding for
UsdValidatorMetadata.It is stacked on #3223 for adding Python bindings for validator framework.
Fixes Issue(s)