Attempt to make UsdUtilsModifyAssetPaths more flexible #1282
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
function on all the paths in each list type, remove any duplicates from the new path vector. The remap function doesn't have enoug global context to prevent remapping several input files to the same output file and having them show up as duplicates. If the duplicates are left in, the validation of the array fails and the modifications are not committed to the layer. Added a similar test for duplicates after remapping sublayers from within UsdUtilsModifyAssetPaths (which was the original impetus for the above changes to list editing with a ModifyCallback).
|
Filed as internal issue #USD-6264 |
pixar-oss
pushed a commit
that referenced
this pull request
Nov 1, 2022
This matches SdfListEditorProxy::ModifyOperations and just exposes functionality that already exists on the underlying Sdf_ListEditor object. Fixes #1282 (Internal change: 2252578)
pixar-oss
pushed a commit
that referenced
this pull request
Nov 1, 2022
- Update sublayer handling to use newly-added SdfListProxy::ModifyItemEdits to take advantage of its removal of duplicate values. This is consistent with how references and payloads are handled. - Expand unit test to exercise handling of duplicated asset paths. - Use lambdas instead of std::bind for consistency - Clean up unnecessary boost header and typos. Fixes #1282 (Internal change: 2252579)
pixar-oss
pushed a commit
that referenced
this pull request
Nov 8, 2022
UsdUtilsModifyAssetPaths returned an empty path. UsdUtilsModifyAssetPaths now removes the original asset path if the modification callback returns an empty asset path. Fixes #1282 (Internal change: 2254050)
|
I added a comment to one of the commits here: 2598536#r110511560 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When using a ModifyCallback to edit a listOp, after running the modify
function on all the paths in each list type, remove any duplicates from
the new path vector. The remap function doesn't have enoug global context
to prevent remapping several input files to the same output file and having
them show up as duplicates. If the duplicates are left in, the validation
of the array fails and the modifications are not committed to the layer.
Added a similar test for duplicates after remapping sublayers from within
UsdUtilsModifyAssetPaths (which was the original impetus for the above
changes to list editing with a ModifyCallback).
Description of Change(s)
I know this is unlikely to be accepted, but I'm using this to start a conversation about how to address an issue (that may be very local to our use case). In Houdini, when saving USD from a LOP network we make extensive use of UsdUtilsModifyAssetPaths (as a replacement for UpdateExternalReferences and some custom code to cover cases not addressed by UpdateExternalReferences). The problem is that one of the tricks we pull involves having a sequence of unique USD file references/payloads/sublayers all resolve to the same single USD file. Because of the way the resolve function in UsdUtilsModifyAssetPaths works, we have no global knowledge, and only get to replace one file at a time. So our callback does all the replacements, and generates a bunch of duplicates. Our old code would collapse the duplicates, and just put the single resolved file into the reference/payload/sublayer list. UsdUtilsModifyAssetPaths uses the List Op editing code, which flags this as a coding error and rejects the edit.
This change does the de-duplication before attempting to do the actual edits. It also handles having the resolve function return an empty path to indicate that the reference/payload/sublayer should be removed.
So I think the use case I'm describing here is reasonable, I just don't know the right way to address it. The changes here seems too far-reaching than I intend it to be. Maybe the change in dependencies.cpp could be left as-is (since it's local to UsdUtilsModifyAssetPaths), but I worry about unintentional behavior changes caused by the listOp.cpp change, since it is used in other code paths. Maybe this behavior could be made optional in listOp somehow?