Physics parsing utilities

[AlesBorovicka]

Description of Change(s)

Provides utility function for UsdPhysics parsing, see parsingUtils.dox file for more details.

Fixes Issue(s)

• I have verified that all unit tests pass with the proposed changes

• I have submitted a signed Contributor License Agreement

[jesschimein]

Filed as internal issue #USD-10246

❗ Please make sure that a signed CLA has been submitted!

[jesschimein]

/AzurePipelines run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[asluk]

Filed as internal issue #USD-10246

❗ Please make sure that a signed CLA has been submitted!

yep Ales is on our corporate CLA 👍🏼

[jesschimein]

Thank you! I've got him on record -- sorry for the confusion 🙇‍♀️

[jesschimein]

/AzurePipelines run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[jesschimein]

/AzurePipelines run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[AlesBorovicka]

@jesschimein I did updated the code to latest dev and added some performance optimizations. The code should be ready for a review.

[jesschimein]

/AzurePipelines run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[tallytalwar]

Hi @AlesBorovicka

Thanks for this PR. I took a very basic initial look at this, before I could internalize it in Pixar code base. Could you please address the notes mentioned here? With strict build turned on the PR fails to build internally and the notes here will help build and test it internally.

Thanks and kindly let me know if you have any questions or concerns.

Thanks

[tallytalwar]

Looks like a copy paste typo. Can you fix the test name here? I think you meant this to be: "testUsdPhysicsParsing"?

[AlesBorovicka]

Thanks for the review, updated the code accordingly.

[AlesBorovicka]

No worries, its a simple change, updated. Thanks

[jesschimein]

/AzurePipelines run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[jesschimein]

/AzurePipelines run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[tallytalwar]

Leaving a set of internal feedback from a few of us at Pixar. There are still some pending reviews internally.

Additionally there are some discussion points (we can discuss these when we meet I think), listed below:

• From @meshula:

This PR effectively duplicates all the existing schema classes into "friendly" structs; see for example PhysicsRevoluteJoint in schema.usda verse UsdPhysicsRevoluteJointDesc in parseDesc.h. Assuming that this is done to save repeatedly querying tokens and attrs, and to not have to have piles of boilerplate checking for authored values vs defaults and so on, it begs some questions:

If it's helpful to have a helper that converts a PhysicsRevoluteJoint to a UsdPhysicsRevoluteJointDesc, why not provide a bunch of utilities that given a prim conformant to the schema returns one of these helpers? Or have these helpers have as a constructor, a prim?
the bulk of parseUtils.cpp is effectively a super fancy prim range generator. Why not create a compute prim range function that takes a filter, or a vector of filters and returns a corresponding vector of PrimRanges? Not "friendlies", just a plain ol' PrimRange?
then a second function or an application could take one of those vectors and return the vector of "friendly" structs?
wouldn't the filtered prim range computing function be more sensible on usdPhysics/scene.h?

Summary:
If the filter was on scene, and returned a usd native prim range, then what's left would be utility functions that extract boiler plate from prims into "friendly" structs and that should be enough, and more integrated with what we already have as opposed to a sidecar thing.

• I feel a lot of the parsing utility code is doing scene validation, which shouldn't be the responsibility of the parser, as it could lead to unnecessary slowness. Ideally UsdPhysicsValidator should be able to handle all these validation checks which makes sure the physics scene and its concepts are conforming to the schema defined rules.

[tallytalwar]

Spiff: I feel like much of what's in parsingUtils.dox belongs in the class dox of the classes it is describing, leaving the dox file to provide the overview and glue, such as enumerating the various types of descriptors, and the theory of operation.

Now, there is an argument in favor of having everything in one place... if that argument wins, then at minimum, the class dox of each f the clases should contain a dox ref back to the appropriate section in the parsingUtils.dox document.

[tallytalwar]

Additionally from Dan:
For maintainability reasons, I tend to lean towards authoring class docs with the class code (so that future code/schema changes have a higher likelihood of also getting the appropriate doc updates).

If we needed to pull class doc info into parsingUtils.dox, we could maybe utilize the doxygen "snippet" directive (https://www.doxygen.nl/manual/commands.html#cmdsnippet) – I've usually only seen this used to include code snippets, but supposedly it has a "doc" option to include/parse the snippet as doc content. I could try some experiments with this if needed.

[marktucker]

I didn't want to jump I until Pixar had their say, but I would add, as a potential consumer/user of this code, that this is not code SideFX is likely to use. To my eyes it translates and validates an existing (not directly usable) description of a physics sim into another (not directly usable) description, which we will then have to translate into our internal (simulateable) description. We would most likely look at this as "sample code" which we might use to help write our own direct translation from the USD representation to our internal representation.

I don't see any benefit to us that would offset paying the runtime and memory, and additional code complexity cost of doing this intermediate translation. A validator on the other hand (as mentioned in Pixar's comments) would make a lot of sense to us as something that could benefit us and our users.

[AlesBorovicka]

Thanks for the feedback, so based on the discussions we have, I have these bullet points:

• adjust the code to coding rules
• take care of minor points in the review
• remove the custom traversal class, replace with input paths and exclude paths in the parsing parameters
• remove validation into separate code path
• document more

As looking at this as a sample code, this is perfectly fine. However I am not sure one can parse the data directly into the engine implementation, the parsing first translates into these descriptors, but later on works based on these descriptors to compute some values, like articulation roots, joint bodies etc. I am not sure one can just take the USD data as is and directly create data in the physics engine in general. But yeah I am happy if it serves as a sample code so that there is some consistency about how the physics data can be retrieved from the stage.

[meshula]

I am happy if it serves as a sample code

The intent of the suggestions was to keep this code practical, not to demote it to sample status :)

remove the custom traversal class, replace with input paths and exclude paths in the parsing parameters

To be sure we are on the same page, we talked about removing the parsePrimIterator and returning a usd primrange instead, and the exclude paths, which currently are used for pruning in the iterator would happen in the parser instead. I was under the impression this was a minimal change. In particular, I thought classes which are necessary for parsing, but not for end users, would be moved into the parser.cpp file and not publicly exposed.

the parsing first translates into these descriptors, but later on works based on these descriptors to compute some values, like articulation roots, joint bodies etc.

Are you saying that by doing the exclusion in the parsing, instead of the prim iterator, information required by someone trying to map into their own physics engine would become impossible to obtain, or they would have to resort to copying the code as an example? When we discussed it, I had the impression the impact of moving the iterator "under the hood" wouldn't impact the ability to use the parser in a practical way. I thought the computed values were all available in the descriptors as part of the parsing operation.

Sidebar,

Speaking of sample code, having some would make the answers to the above very obvious. I think it wouldn't have to be running sample code, just written documentation with brief code fragments to illustrate the principles and how one is meant to set up a joint and so on.

[AlesBorovicka]

Yes, the parsePrimIterator would be gone, the signature of the parsing function would change to this:
USDPHYSICS_API bool LoadUsdPhysicsFromRange(const UsdStageWeakPtr stage, const std::vector<SdfPath>& includePath, const std::vector<SdfPath>& excludePath, UsdPhysicsReportFn reportFn, void* userData, const CustomUsdPhysicsTokens* customPhysicsTokens = nullptr, const std::vector<SdfPath>* simulationOwners = nullptr);
Then yes the traversals would get created in the parsing code, the returned descriptors would not change.

[nvmkuruc]

For consistency with other globals in OpenUSD, should this be UsdPhysicsSentinelLimit?

Could/should it be constexpr?

[nvmkuruc]

Was this indentation a mistake?

[nvmkuruc]

Was this indentation a mistake?

[nvmkuruc]

Do these need to be prefixed with UsdPhysics since they're in public headers?

[nvmkuruc]

Do these need to be prefixed with UsdPhysics since they're in public headers?

[nvmkuruc]

If these are cpp only, should they be in an anonymous namespace and/or prefixed with _ to align with the coding conventions.
https://openusd.org/release/api/_page__coding__guidelines.html#:~:text=OpenUSD%20Code%20Conventions%20and%20Formatting,single%20line%20in%20their%20bodies.

[jesschimein]

/AzurePipelines run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[jesschimein]

/AzurePipelines run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).