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

Convert UsdZip to C++ #2101

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from

Conversation

dgovil
Copy link
Collaborator

@dgovil dgovil commented Nov 18, 2022

This PR depends on #2090 , and follows up by converting usdzip to C++.
The files that are changed from that PR are:

  • pxr/usd/bin/usdzip/CMakeLists.txt
  • pxr/usd/bin/usdzip/usdzip.cpp

additionally a new GetFileSize function was added to the pxr/usd/bin/common.h/cpp files.

Some notes:

  • Compliance Checking results in an error being surfaced to the user since those have yet to be ported to C++. I was torn on whether to leave the argument in place or not, but either way an error seems best for now.
  • testUsdZipInputFiles2 fails because the order of the files listed from the UDSZ are in a different order. I'm not sure what to do here. I'm not sure what causes the files to be listed in a different order, but I don't see anything obvious. I'm getting the filelist in the same order as the UsdZipFile provides it, but it looks like UsdZipFile gives it to me in a different order? specifically src/sub/c.png and src/sub/d.txt are returned in flipped positions.
  • I have verified that all unit tests pass with the proposed changes with one minor exception
  • I have submitted a signed Contributor License Agreement

pxr/usd/bin/common.cpp Outdated Show resolved Hide resolved
@sunyab
Copy link
Contributor

sunyab commented Nov 19, 2022

Filed as internal issue #USD-7784

@dgovil
Copy link
Collaborator Author

dgovil commented Nov 28, 2022

Refactored to base on #2107
I adjusted the failing recursive test to pass since I don't think it's actually an issue, just a difference in order of how the Python zipfile works compared to the USD zip reader, but both are correct.

@dgovil
Copy link
Collaborator Author

dgovil commented Aug 31, 2023

@meshula I wanted to revisit this PR and see what you think would be a good approach to get it in.

  1. I can update it to be built on top of the latest dev, and following the conventions of the latest USD dev commit. That's not an issue.
  2. Compliance checker is only available in Python which gates the --checkCompliance flag. I'm not sure on the ETA of a C++ rewrite of the compliance checks, but I was wondering what you thought about calling the Python compliance checker if compiled with Python, and otherwise having the flag raise an error if used. When the C++ rewrite of the checks lands, we can swap that out fairly easily.

Anyway would love your thoughts since I'd love to have usdzip in my non-Python builds of USD for selfish reasons.

@meshula
Copy link
Member

meshula commented Aug 31, 2023

Other non-python-centric groups such as gamedev are keen to have non-pythonic versions of the tools, so you're not the only one looking for C++ versions of these tools. Since you tagged me I feel compelled to have a cunning thought.

What if you make an intermediate stage for your project, that doesn't lose functionality?

name the c++ tool something else, like I dunno, usdziptool
usdzip.py checks for --complianceCheck and does the thing. If it passes, it passes all the arguments to usdziptool to actually do the work of creating the archive.

now you can run usdziptool in a python-less environment, and the existing functionality of usdzip is unaffected if one has a python-present environment.

Maybe it's practical to have usdzip and usdzip.py live side by side, so maybe the subterfuge of a renamed c++ variant is not needed. Not sure.

@dgovil
Copy link
Collaborator Author

dgovil commented Aug 31, 2023

Hmm I can give that a try and see what feels better, though I'm a little hesitant to introduce an extra file to the mix. The last round of C++ rewrites I did, the Pixar folks had said to go with a hard switchover for simplicit.

Anyway, I'll try your suggestion out in addition to mine. The changes are minimal either way, so should be quick to try out.

@meshula
Copy link
Member

meshula commented Aug 31, 2023

Re-reading your #2 and your response, I now understand your option #2 (I'd read it wrong the first time). That sounds roughly equivalent to what I suggested but with one less published file, so that suggestion sounds reasonable to me.

@dgovil
Copy link
Collaborator Author

dgovil commented Aug 31, 2023

Okay great, that simplifies things

@dgovil
Copy link
Collaborator Author

dgovil commented Aug 31, 2023

@meshula okay donesies. This is rebased to the latest dev commit as of this morning.
It will either fail the compliance checker function if compiled without Python, or it uses CPython to call the Python code.

I didn't end up using the Tf Python functions because I was getting random segfaults when trying to use it, that I couldn't unwind. Since this is a leaf utility, I ended up just using standard CPython functions.

Let me know if there's any changes y'all recommend. Hopefully it's in a state that's acceptably mergeable.

@dgovil
Copy link
Collaborator Author

dgovil commented Sep 1, 2023

Excellent, all the tests passed. I always forget msvc doesn't support all the C keywords that clang and gcc do.

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