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

Fix UDIM path resolution for scene index plugins #3494

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

marktucker
Copy link
Contributor

Description of Change(s)

Implement a specialization of UsdImagingDataSourceAttribute
that properly resolves UDIM relative paths.

Note that I have not created a unit test, or run the unit tests yet. I'm posting this PR early because there is an obvious issue that I want to discuss, namely the correct "location" for this code change. This change, as is, will affect all SdfAssetPath attributes read from USD and accessed by hydra. It is not clear to me if this is a good thing or a bad thing.

If it's a bad thing, I can take this UsdImagingDataSourceAttribute sepcialization and move it to dataSourceMaterial.cpp instead. Then in _UsdImagingDataSourceShadingNodeParameters::Get, I can conditionally create this data source for SdfAssetPath attributes instead of calling UsdImagingDataSourceAttributeNew to get the "standard" SdfAssetPath handler.

Alternatively, UsdImagingDataSourceAssetPathAttribute could be defined and implemented in dataSourceAttribute.h/cpp, but the standard factory would still return a vanilla UsdImagingDataSourceAttribute. But in dataSourceMaterial.cpp I could introduce the condition to use UsdImagingDataSourceAssetPathAttribute for SdfAssetPath attributes. This has the nice feature that if any other scene index code wants to support UDIM translation, the specialized subclass can be shared.

I personally think the universality might be a good thing as if some renderer supports UDIMs on a procedural or other custom prim type, they won't have to jump through hoops or create a custom data source to handle that case. The downside is every time hydra accesses an SdfAssetPath attribute, we will scan that string looking for "". To me this seems like a small price, but happy to accept Pixar's judgement on this and alter this PR as needed.

Fixes Issue(s)

#3492

Checklist

[ X ] I have created this PR based on the dev branch

[ X ] I have followed the coding conventions

[ ] I have added unit tests that exercise this functionality (Reference:
testing guidelines)

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

[ X ] I have submitted a signed Contributor License Agreement (Reference:
Contributor License Agreement instructions)

…ath>

that properly resolves UDIM relative paths.
@jesschimein
Copy link
Collaborator

Filed as internal issue #USD-10594

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

@jesschimein
Copy link
Collaborator

/AzurePipelines run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

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

Successfully merging this pull request may close these issues.

2 participants