-
Notifications
You must be signed in to change notification settings - Fork 1
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
Allowing annotation to work with any GLB (e.g. non objaverse) file and some other minor improvements. #5
Conversation
LGTM some things in |
I created a PR to fix the merge conflicts with main, but closed it because it would add all the changes from main to the branch (which I guess it's not something @Lucaweihs might want at this time): https://github.com/allenai/objathor/tree/merge-conflicts-newVHACD, apart from the changes in the closed PR, now also includes passing tests and moved all |
# Add symlink if it doesn't already exist | ||
print(f"Symlink from {asset_directory} to {build_target_dir}") | ||
os.symlink( | ||
os.path.abspath(asset_directory), os.path.abspath(build_target_dir) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was also cleaned up, fixed and tested in main
so would keep that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be the same as on main except I have removed the not asset_symlink
option as I don't think we ever want it. I also made sure we're using abs paths in the os.symlink
call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AlvaroHG - Let me know if you're ok with removing the not asset_symlink
branch of the if statement or if it's important for some reason I don't 100% understand.
After merging PR #12 I would check merge conflicts and use comments as a guidance, most things seem good to keep; minor changes like removing relative imports, better help descriptions, variables. |
@AlvaroHG @jordis-ai2 - I've updated this PR to resolve Alvaro's comments and also add in some additional annotation improvements. I've processed a few hundred GLBs locally and it seems to work well. |
Just noticed tests were failing: #13. If that seems a reasonable fix, and we merge it here, I'd say we're good to merge into main. |
Passed conversion tests
I was getting errors when trying to run the object consolidator code so I fixed them one by one until it worked for me. In the process, I changed the VHACD binary to use the latest 4.1 version from https://github.com/kmammou/v-hacd (required changing some parameters).