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

Explicitly case Ar path string to ArResolvedPath #1261

Conversation

dj-mcg
Copy link
Collaborator

@dj-mcg dj-mcg commented Mar 16, 2021

Passing a path string to ArResolver::OpenAsset throws an error when built against the latest version of USD. This parameter is explicitly cast to ArResolvedPath elsewhere in the USD code, so updating this callsite to follow that pattern.

dj-mcg added 2 commits March 16, 2021 13:22
This was throwing errors when built against the latest version of USD.
File path strings are explicitly cast elsewhere in the USD code,
updating this one to follow suit.
This throws an error when built against the latest USD. File path
strings are being explicitly cast to ArResolvedPath objects when passed
to ArResolver::OpenAsset elsewhere. Updating this call to follow this
pattern.
@dj-mcg dj-mcg added the build Related to building maya-usd repository label Mar 16, 2021
@kxl-adsk
Copy link

FYI @ysiewappl

Copy link
Collaborator

@JGamache-autodesk JGamache-autodesk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@JGamache-autodesk JGamache-autodesk added the ready-for-merge Development process is finished, PR is ready for merge label Mar 16, 2021
@kxl-adsk kxl-adsk removed the ready-for-merge Development process is finished, PR is ready for merge label Mar 17, 2021
@@ -266,7 +266,7 @@ bool PxrMayaUsdUVTexture_Reader::Read(UsdMayaPrimReaderContext* context)
&& !filePath.empty() && ArIsPackageRelativePath(filePath)) {
// NOTE: (yliangsiew) Package-relatve path means that we are inside of a USDZ file.
ArResolver& arResolver = ArGetResolver(); // NOTE: (yliangsiew) This is cached.
std::shared_ptr<ArAsset> assetPtr = arResolver.OpenAsset(filePath);
std::shared_ptr<ArAsset> assetPtr = arResolver.OpenAsset(ArResolvedPath(filePath));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dj-mcg Please check PF results - https://github.com/Autodesk/maya-usd/pull/1261/checks?check_run_id=2125284262. Looks like this wasn't existing in USD 20.11. We currently support USD versions down to 20.02.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @kxl-adsk - put in a PXR_VERSION check and running preflight now

ArResolver::OpenAsset takes an ArResolvedPath in versions after 20.11.
Add a version check when calling this function and passing an
std::string and explicitly cast to an ArResolvedPath instead.
@kxl-adsk
Copy link

We had some slow network issues...that slowed down the PF enough to cause the timeout on GitHub Action side. The internal job (job/ecg-mayausd-branch-preflight-master-public/1128/) finished with success. No need to re-trigger the PF.

@kxl-adsk kxl-adsk added the ready-for-merge Development process is finished, PR is ready for merge label Mar 18, 2021
@kxl-adsk kxl-adsk merged commit 0e00e1d into Autodesk:dev Mar 18, 2021
@dj-mcg dj-mcg deleted the pr/Explicitly_cast_Ar_path_string_to_ArResolvedPath branch March 18, 2021 23:47
@ysiewappl
Copy link
Contributor

Sorry, something was bugging me and I just double-checked with @dgovil: I am already building against USD 21.02 here and the code builds fine without this check.

From what I can see:

https://graphics.pixar.com/usd/docs/668045551.html#AssetResolution(Ar)2.0-RolloutandTransition

Ar 2.0 will substantially modify the ArResolver interface and other related classes. To maintain backwards compatibility with existing ArResolver implementations and client code sites, users will be able to specify whether they want to enable Ar 2.0 at build time. If enabled, the AR_VERSION flag will be defined and to 2 and the Ar 2.0 changes will be built and installed, causing old code to break if not updated to accommodate those changes. If disabled, AR_VERSION will be set to 1 and Ar will revert back to the code that exists today, ensuring no change in behavior or build breakages.

For its initial release, Ar 2.0 will be disabled by default – the current Ar implementation will remain the default for backwards compatibility, but will be deprecated. Users will be able to enable Ar 2.0 using the build flag to begin testing and transitioning their resolver implementations and client code to the new interface. Ar 2.0 will be enabled by default in the subsequent release, and the old Ar implementation will be removed at that time.

Should the macro be changed to check against this instead rather than the USD version itself, assuming people want to use the old resolver and haven't built their libUSD with this flag enabled?

@kxl-adsk
Copy link

I'm not sure about the connection to Ar 2.0 here. We are internally building without Ar 2.0 which means PF is validating against an artifact that doesn't have it. The problem was that this change originally only worked for USD 21.02 and above. It was not compiling with older versions (like 20.11 down to 20.02).

Based on that, why should the check be changed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Related to building maya-usd repository ready-for-merge Development process is finished, PR is ready for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants