PixarAnimationStudios / OpenUSD

Universal Scene Description
http://www.openusd.org
Other
6.17k stars 1.23k forks source link

Physics parsing utilities #3347

Open AlesBorovicka opened 1 month ago

AlesBorovicka commented 1 month ago

Description of Change(s)

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

Fixes Issue(s)

-

jesschimein commented 1 month ago

Filed as internal issue #USD-10246

:heavy_exclamation_mark: Please make sure that a signed CLA has been submitted!

jesschimein commented 1 month ago

/AzurePipelines run

azure-pipelines[bot] commented 1 month ago
Azure Pipelines successfully started running 1 pipeline(s).
asluk commented 1 month ago

Filed as internal issue #USD-10246

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

yep Ales is on our corporate CLA 👍🏼

jesschimein commented 1 month ago

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

jesschimein commented 1 month ago

/AzurePipelines run

azure-pipelines[bot] commented 1 month ago
Azure Pipelines successfully started running 1 pipeline(s).
jesschimein commented 1 month ago

/AzurePipelines run

azure-pipelines[bot] commented 1 month ago
Azure Pipelines successfully started running 1 pipeline(s).
AlesBorovicka commented 4 weeks ago

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

jesschimein commented 4 weeks ago

/AzurePipelines run

azure-pipelines[bot] commented 4 weeks ago
Azure Pipelines successfully started running 1 pipeline(s).
AlesBorovicka commented 3 weeks ago

Thanks for the review, updated the code accordingly.

AlesBorovicka commented 3 weeks ago

No worries, its a simple change, updated. Thanks

jesschimein commented 3 weeks ago

/AzurePipelines run

azure-pipelines[bot] commented 3 weeks ago
Azure Pipelines successfully started running 1 pipeline(s).
jesschimein commented 6 days ago

/AzurePipelines run

azure-pipelines[bot] commented 6 days ago
Azure Pipelines successfully started running 1 pipeline(s).
marktucker commented 5 days ago

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 commented 1 day ago

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

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 commented 20 hours ago

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 commented 7 hours ago

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.