Skip to content

Direct delta mush python binding#153

Merged
alecjacobson merged 12 commits intolibigl:masterfrom
kishoreVen:direct-delta-mush-python
Feb 1, 2023
Merged

Direct delta mush python binding#153
alecjacobson merged 12 commits intolibigl:masterfrom
kishoreVen:direct-delta-mush-python

Conversation

@kishoreVen
Copy link
Copy Markdown
Contributor

Expose Direct Delta Mush Skinning to Python

Libigl-python currently doesn't support direct delta mush skinning. The set of commits in this branch allows for this support. Both the precomputation and delta mush skinning are exposed through the direct_delta_mush.cpp binding source and are tested in test_direct_delta_mush function of the tests/test_basic.py file.

Additionally I fixed a few tests (830880d) by pointing the tests to the correct numpy datatypes. These checks seemed to be failing the tests and I wanted to have a good starting point before adding the direct delta mush skinning test.

Platform Notes

I tested all my changes on the following configuration

OS: Ubuntu 22.04.1 LTS
Type: 64 Bit
Processor: Intel® Core™ i7-9750H CPU @ 2.60GHz × 12
Memory: 16 GB
Graphics: NVIDIA Corporation TU116M [GeForce GTX 1660 Ti Mobile]
Graphics Memory: 6 GB
Driver Version: 515.86.01
CUDA Version: 11.7

@kishoreVen
Copy link
Copy Markdown
Contributor Author

Just realized these type checks are platform dependent issues. I will revert (830880d) that commit and leave the rest as is. Shouldn't impact rest of the branch.

@kishoreVen
Copy link
Copy Markdown
Contributor Author

Some Images after the most recent fix

Left (Green) is Rigid Skinning, Right (Cyan) is Direct Delta Mush Skinning

Screenshot from 2023-01-05 18-19-10

Screenshot from 2023-01-05 18-20-57

Screenshot from 2023-01-05 18-21-16

@jiangzhongshi
Copy link
Copy Markdown
Contributor

This is fantastic! Could you do a rebase against the latest master?

kishoreVen and others added 3 commits January 6, 2023 18:09
* Update README.md

* deprecate np.float

* deprecate np.int

* updated deprecated package in setup

* fixed types in tests

* added default dtype

Co-authored-by: Teseo Schneider <teseo@bluewin.ch>
Comment thread src/direct_delta_mush.cpp Outdated
Comment thread src/direct_delta_mush.cpp
npe_function(direct_delta_mush)
npe_doc(ds_direct_delta_mush)

npe_arg(v, dense_double)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

why only double?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If the question is about why direct_delta_mush.cpp (https://github.com/libigl/libigl/blob/main/include/igl/direct_delta_mush.cpp#L267) is only double, I think it's because DDM has some artifacts when using single precision.

If the question is why double for our python bindings, it is because I felt that it best reflected that DDM only works on double precision. And that work needs to be done for single precision. IMaybe I can let you pick an option here

Option 1. I can leave this as a comment in the file.
Option 2. Take in float as well and cast it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

yeah, it looks like the upstream libigl function has Affine3d hard coded. I doubt that double precision is needed, just seems like the libigl function should be better templated. I think Option 1 is fine until libigl changes (happy to merge such a PR there).

Comment thread src/direct_delta_mush.cpp Outdated
Comment thread src/direct_delta_mush.cpp
npe_function(direct_delta_mush_precomputation)
npe_doc(ds_direct_delta_mush_precomp)

npe_arg(v, dense_double)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

why only double?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

same options as the question above since the reason is exactly the same.

Comment thread tests/test_basic.py
self.assertTrue(U.shape[0] == V.shape[0])
self.assertTrue(U.shape[1] == 3)
self.assertTrue(U.dtype == V.dtype)
self.assertFalse(np.isnan(U).any())
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

add checks for dtypes

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I am assuming you meant dtype checks for omega and specific check for U. But if you have something else in mind please let me know

Comment thread src/direct_delta_mush.cpp
Copy link
Copy Markdown
Contributor Author

@kishoreVen kishoreVen left a comment

Choose a reason for hiding this comment

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

Addressed comments.

@alecjacobson alecjacobson merged commit 3668b6c into libigl:master Feb 1, 2023
@kishoreVen kishoreVen deleted the direct-delta-mush-python branch February 26, 2023 02:37
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.

4 participants