Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SdfAssetPath values retrieved via ComposeAttributeDefaultValue() are not resolved #3488

Open
fhechtAdobe opened this issue Jan 22, 2025 · 3 comments

Comments

@fhechtAdobe
Copy link

fhechtAdobe commented Jan 22, 2025

Context

Dynamic file format plugins can determine their file format args based on the default values of attributes on the prim that contains the payload arc.

Access to those default values is available in the PcpDynamicFileFormatInterface::ComposeFieldsForFileFormatArguments() virtual function. In said function the file format plugin can call PcpDynamicFileFormatContext::ComposeAttributeDefaultValue() to retrieve a composed default value.

These values usually match what one would get, if the default value was retrieved, for a usual UsdAttribute via the normal USD APIs. But there is a subtle difference for asset typed attributes, which return a SdfAssetPath value.

The normal code paths on the UsdStage return not just the authored value (GetAssetPath()), but also the resolved path (GetResolvedPath()). This resolution happens roughly here: https://github.com/PixarAnimationStudios/OpenUSD/blob/v25.02/pxr/usd/usd/stage.cpp#L514

In case of retrieving the default value via the PcpDynamicFileFormatContext the retrieval of the value happens here: https://github.com/PixarAnimationStudios/OpenUSD/blob/v25.02/pxr/usd/pcp/dynamicFileFormatContext.cpp#L85

But this is the raw default value that is authored on a layer. No special treatment is there for SdfAssetPath typed values, nor any of the other special steps that are taken when querying default values via the normal USD APIs.

Problem

When a file format gets a default value of type SdfAssetPath via the ComposeAttributeDefaultValue() method, the path is unresolved. And in case of a layer relative path, this can't be resolved anymore, since the information about which layer the value was composed from is not available. Similar to the normal USD API query for default values, this has to be resolved at a point in the code, where the layer context is available.

Solution?

I experimented with adding a straight forward resolution step after the code here:https://github.com/PixarAnimationStudios/OpenUSD/blob/v25.02/pxr/usd/pcp/dynamicFileFormatContext.cpp#L85

if (value.IsHolding<SdfAssetPath>()) {
    SdfAssetPath assetPath = value.UncheckedGet<SdfAssetPath>();
    assetPath = SdfAssetPath(
        assetPath.GetAssetPath(),
        _ResolveAssetPathRelativeToLayer(
            layer, assetPath.GetAssetPath()));
    value = VtValue(assetPath);
}

_ResolveAssetPathRelativeToLayer() was copied over from the code in usd/stage.cpp. This seems to handle the specific case of layer relative paths, but I'm not certain this is all there is to consider.

I saw the variable expression support in the UsdStage code. Is this something that needs to be handled? What about asset[] typed attributes? The above snippet only handles the single path case.

I think there is a gap between normal default value resolution on a UsdStage and what the specialized API for dynamic file formats does. I think that gap should be narrowed to the extent possible, especially for paths. But I do realize the that one API is part of the lower level pcp module and the other is part of the higher level usd module.

@jesschimein
Copy link
Collaborator

Filed as internal issue #USD-10588

(This is an automated message. See here for more information.)

@spiffmon
Copy link
Member

Thanks for filing, @fhechtAdobe . While fully acknowledging the issue you're raising, we're going to need to discuss a bit how to go about addressing it. I think the code you pointed at assumes that expression variables have already been evaluated within the assetPaths, and that highlights that the value resolution logic that UsdStage applies is non-trivial, and we need to consider how to make it available to the dynamic payload situation without too much code duplication is concepts-crossing-software-layers. Going to prioritize this, but it may still take a beat or two.

@fhechtAdobe
Copy link
Author

No worries. We're not immediately blocked by this, but we did notice this resolution issue and wanted to flag it. And we understand there are some very fundamental things to figure out between the software layers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants