[usdAbc] Pass Sdf arguments to Alembic plugin. #1099
Conversation
|
This sounds interesting, @marsupial, and will try to review soon. But up front, can I ask you to add new tests and test files for the new behaviors? |
|
I knew that was coming, so yeah, on its way |
|
Filed as internal issue #USD-5822 |
There was a problem hiding this comment.
Thank you, @marsupial ! There are a few changes we'd like to request - please let us know when it's ready for re-review?
Cheers,
--spiff
pxr/usd/plugin/usdAbc/alembicData.h
Outdated
| @@ -43,7 +44,7 @@ class UsdAbc_AlembicData : public SdfAbstractData { | |||
| /// Returns a new \c UsdAbc_AlembicData object. Outside a successful | |||
| /// \c Open() and \c Close() pairing, the data acts as if it contains | |||
| /// a pseudo-root prim spec at the absolute root path. | |||
| static UsdAbc_AlembicDataRefPtr New(); | |||
| static UsdAbc_AlembicDataRefPtr New(const SdfFileFormat::FileFormatArguments* = nullptr); | |||
There was a problem hiding this comment.
It introduces a couple of layers of extra complexity to change the higher-level pattern of passing FileFormatArguments by const-ref to by pointer, and I'm curious why we'd want to do it? You're already paying for heap-allocating a copy of the std::map (which is generally going to be empty or very small), so why not eliminate all of that and the checks-for-nullptr, and just plumb the const-ref all the way down and let the member-var hold a copy?
There was a problem hiding this comment.
No idea why I chose that initially other than a quick way to enable disable the custom behavior.
Easy enough to change.
| const auto abcLayers = args->find("abcLayers"); | ||
| if (abcLayers != args->end()) { | ||
| for (auto&& l : TfStringSplit(abcLayers->second, ",")) { | ||
| layeredABC.emplace_back(std::move(l)); |
There was a problem hiding this comment.
I know Alembic doesn't do any asset resolution, by-design... but would it not be valuable in the USD universe to attempt to anchor (to "filePath") and resolve each of these paths using the installed ArAssetResolver before passing the paths down to Alembic?
There was a problem hiding this comment.
I've looked into this, but it's currently a bit problematic on the Alembic side to pass layers as relative (particularly in a multi-threaded env). Second, there may be cases where the layer is in a totally separate location, unrelated to the -base layer-.
Or are you suggesting to call the resolver within usdAbc.so ?
There was a problem hiding this comment.
I wasn't suggesting that you pass relative paths to Alembic. Instead, use ArGetResolver().Resolve() (etc) to resolve the paths to full paths in USD-land before passing them on to Alembic.
But if that seems undesirable, then just please document in overview.dox that you must only supply full paths in the URI, which is a bit limiting for fancy resolver setups, but probably OK since this is for Alembic backwards compatibility.
Thanks!
There was a problem hiding this comment.
Ah OK, then yes (i.e. this is currently already being done on the resolver side here), I'll add the note.
| IArchive archive; | ||
| std::vector<std::string> layeredABC; | ||
| if (args) { | ||
| const auto abcLayers = args->find("abcLayers"); |
There was a problem hiding this comment.
Although you could, I won't ask you to add the string-literals for the arg-strings into alembicUtils.h as Tokens (both because you only use each once, and because, since this is a plugin, clients can't really use the header files anyways).
But, please do add a new section to overview.dox that both names the new args, and gives example authored asset paths that use them. And for bonus points, refer people to SdfLayer::CreateIdentifier() as a helper for building up such asset paths. That will show up here.
| if (args) { | ||
| const auto reRoot = args->find("abcReRoot"); | ||
| if (reRoot != args->end()) | ||
| abcReRoot = TfToken(reRoot->second); |
There was a problem hiding this comment.
You should verify TfIsValidIdentifier() on abcReRoot, and discard and warn if not.
Add ability to read the entire hierarchy in under a single parent. Add ability to load layered Alembic.
|
Think I got to all the reviews minus the HDF5 threading issue, but if I missed any let me know |
Description of Change(s)
And adds two arguments to the reader:
abcReRoot Read the entire hierarchy in under a single parent held in argument's value
abcLayers Comma separated list of secondary files to load as a layered Alembic.
Fixes Issue(s)
<defaultPrim>is a bit cumbersome for Alembics prepared without any concept of it