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

Centering joints #9

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ZhengyiLuo
Copy link

Centering the joints before applying to user-supplied translation.

Reference issue: #8 (comment)

Thank you so much!

Copy link

@PerryXDeng PerryXDeng left a comment

Choose a reason for hiding this comment

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

This seems to be a rather important change that can prevent unexpected bugs
👍. Since the centering condition does not depend on the existence of translation, it looks like they can be taken out of the larger if-else block entirely

@@ -149,6 +149,11 @@ def forward(self,
th_jtr = th_jtr - center_joint
th_verts = th_verts - center_joint
else:
if self.center_idx is not None:

Choose a reason for hiding this comment

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

Seems like this if block (and the one above in line 147-150) is independent of the surrounding clauses.

@hassony2
Copy link

Hi @ZhengyiLuo,

Thank you for pointing out this behavior !
Indeed, currently centering is done only if translation is 0 or none, which might not be the intuitive behavior.

However, accepting such a pull request can cause unexpected changes for people who are unknowingly using the current code, e.g. who have set a center_idx which is not used because they have provided a translation (th_trans)
If they upgrade with the added pull request suddenly their translations won't be the same anymore, and existing code might break.

I think a solution at this point would be the following:
Highlight the current behavior in the documentation (something like "Note that providing a translation with prevent the centering on a joint in the forward pass, so center_idx will be ignored if th_trans is provided")

What do you think?

If both centering and translation is needed for your specific use-case, maybe a solution could be to manually apply the translation on the returned joint and vertex values from the layer ?

Cheers !

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