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

Remove assumption in UsdUtils_FileAnalyzer that USDZ files are fully self contained #2637

Conversation

dgovil
Copy link
Collaborator

@dgovil dgovil commented Sep 1, 2023

Description of Change(s)

UsdUtils_FileAnalyzer assumed that USDZ files are fully self contained.
I'm removing this assumption because:

  1. According to prior discussion, USDZ files may perhaps refer to external files, even if that's not ideal
  2. This was skipping validation in files that did have missing references, thereby not catching incorrect files

Fixes Issue(s)

2636

  • I have verified that all unit tests pass with the proposed changes
  • I have submitted a signed Contributor License Agreement

@meshula
Copy link
Member

meshula commented Sep 1, 2023

Irrespective of point 1,

USDZ files may perhaps refer to external files

the test you removed is making a pretty big assumption that the referenced package itself is valid. USDZ files do have the requirement that they are reproducibly resolvable, so removing the "early out" from the test should be both safe to do, and increase confidence about the overall analysis. My follow up question would be whether the analyzer checks for the "anchored" restriction, or if there might be further work required?

For Reproducible Results, Encapsulate Using Anchored Asset Paths

To insure uniform consumption of assets shipped to clients via usdz, we feel it
is prudent to curtail the power of USD's asset resolution system, so that the
asset references within a package resolve uniformly on any consuming system,
regardless of how that system's USD ArResolver is configured. Rather than make
restrictions within the FileFormat itself, we propose that such "encapsulation"
of packages be achieved by restricting the content to only use anchored
paths (paths that begin with "./" or "../", both of which are interpreted in the
virtual filesystem described by the package's internal layout.

@dgovil
Copy link
Collaborator Author

dgovil commented Sep 1, 2023

I'm not sure on that front. I'd have to go code spelunking, but at a cursory glance I don't believe it does.
However there is ARKitPackageEncapsulationChecker at the UseChecker level that does check for encapsulation, but only for assets that actually resolve which was why nothing was flagging it.

@jesschimein
Copy link
Collaborator

Filed as internal issue #USD-8641

@pixar-oss pixar-oss closed this in dc01ba9 Apr 19, 2024
@meshula meshula reopened this Apr 20, 2024
@meshula
Copy link
Member

meshula commented Apr 20, 2024

Reopened, as the noted commit only partially addressed the PR.

@dgovil
Copy link
Collaborator Author

dgovil commented Apr 22, 2024

@meshula It looks like this was fixed somewhere along the way in the last few USD versions, even though I can't find a commit that intentionally did so. I'll (re)close this one out.

@dgovil dgovil closed this Apr 22, 2024
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.

3 participants