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

Update to latest Magnum with full skinning support and much more #1968

Merged
merged 3 commits into from
Dec 19, 2022

Conversation

mosra
Copy link
Collaborator

@mosra mosra commented Dec 15, 2022

Motivation and Context

Upgrades Magnum to latest, containing the following:

image

Builtin shader constructors got changed to accept a Configuration class because with skinning the argument count was getting out of hand. While the previous constructor signatures are still available, they're marked as deprecated so I updated the code to use the new APIs.

Skinning support and skinned animation playback in the magnum-player utility and its prebuilt binaries, as I advertised on the call today, will arrive in the next days.

How Has This Been Tested

My CIs pass.

@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Dec 15, 2022
@mosra
Copy link
Collaborator Author

mosra commented Dec 16, 2022

Anybody knows what's up with the CI? Both of the failed jobs end with a cascade of errors that have

AttributeError: module 'torch._C' has no attribute '_cuda_setDevice'

at the top. I was getting this with #1947 and thought the reason was that it was made from a fork, because if I pushed the same changes to a branch on the main repo, the CI passed. But this PR somehow doesn't.

@Skylion007
Copy link
Contributor

@mosra Pull from master that should have been fixed. It's pulling the CPU only version of Torch I think.

@mosra
Copy link
Collaborator Author

mosra commented Dec 16, 2022

But this PR is based off current main, commit fffa543 from a week ago...

@Skylion007
Copy link
Contributor

Oh boy, did it break again? Ping @aclegg3

Copy link
Contributor

@0mdc 0mdc left a comment

Choose a reason for hiding this comment

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

LGTM!

@mosra mosra merged commit 4f5b89c into main Dec 19, 2022
@mosra mosra deleted the update-magnum19 branch December 19, 2022 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Do not delete this pull request or issue due to inactivity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants