Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add friction direction unit vector and frame parameters #1427
Add friction direction unit vector and frame parameters #1427
Changes from 4 commits
ed3a488
d1d8073
0195b7a
843b40b
ed85373
1962ef3
a932d87
5fe40b7
cddef7a
b3438cb
691fa12
d2b9e0d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
It's not the part of this change, but I would like to negate the direction of
tangent
so thatT.col(0).cross(T.col(1)) == n
(currently, it'sT.col(1).cross(T.col(0)) == n
) because I believe it'd be more intuitive. This change wouldn't affect the behavior (but just flipping the sign of impulse for the second tangential friction).It can be done by changing the order of all the cross products in this function. For example, line 813 should be changed to:
Eigen::Vector3d tangent = n.cross(mFirstFrictionalDirection);
or simply negating the final
tangent
. I'd prefer changing the order of the cross products though.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.
I'm not opposed to that, but I'll bring @azeey into the conversation since he made this specific change
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.
Negating the direction of
tangent
sounds fine to me if it doesn't affect the behavior. I think the main change here is thatT.col(0)
has to be parallel to the first frictional direction.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.
reversed in 5fe40b7
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.
Currently, the first friction direction will be set to z-direction in world coordinates (unless it's parallel to the contact normal). Maybe we want to set the default direction to non-zero direction (e.g.,
Eigen::Vector3d::UnitZ()
) so that the default behavior is more consistent in terms of the friction direction?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.
my intent was that an unset vector with zero length would use the global friction direction, and that only friction directions that are explicitly set to have a nonzero length would use their custom direction and frame. See the following logic in ContactConstraint.cpp
dart/dart/constraint/ContactConstraint.cpp
Lines 148 to 172 in a932d87
does that make sense?
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.
Yeah, I read the logic but was suggesting changing the default behavior to using local friction direction (by setting
mFirstFrictionDirection
to non-zero vector intentionally) because first/second friction coefficients would make more sense when they're aligned with local directions (if they have different values).Downside of it is that it changes the default simulation behavior (from using global friction direction to local friction direction) though. I'm okay to leave as it is as long as the logic is documented.
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.
a tricky aspect of the friction directions is choosing which to use when custom directions are set for both bodies. I've added logic in this PR to choose the direction from the body with the smaller primary friction coefficient, since that would be the dominant parameter, which makes sense to me, but I haven't tested it, and I'd be wary of making a big behavior change like that without testing.
Is the logic sufficiently documented? If not, I'll add the necessary documentation.
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.
Agreed to not changing the default behavior.
It's well documented in the implementation, but it'd be great if it's also documented in the header as well (maybe on the constructor?).
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.
how does cddef7a look to you?
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.
Looks awesome! Thank you for adding it.