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 FixedJoints: restoring spring-damped-torques, initial rotation offset #135

Merged

Conversation

mstoelzle
Copy link
Contributor

@mstoelzle mstoelzle commented Jul 14, 2022

Fixes #131 for the FixedJoint class:

  • Now any arbitrary torques applied to rods or rigid bodies connected by a FixedJoint are compensated (e.g. more general implementation)
  • Rotation frame deviation is computed using rotation vectors
  • Rotational spring-damper system is used to restrict the rotational mode
  • Spring: spring constant kt scales the rotational frame deviation between systems one and two
  • Damper: damping constant nut scales the difference between angular velocities of system one and two
  • Introduce functionality of static rotation offset between systems one and two: records the initial relative rotation between the two systems and enforces this static rotation throughout the entire simulation. If use_static_rotation==False, the static rotation is set to an identity matrix and restoring torques are applied to fully align the local frames of both systems at the join.
  • Example examples/JointCases/fixed_joint_torsion.py illustrates how this implementation generates torsional / twisting torques to prevent torsional deviations
  • Use common diagnostic callback for all joint cases examples
    The attached plots demonstrates the results of the examples/JointCases/fixed_joint_torsion.py example:
    Orientation_of_last_node_of_rod_1
    Orientation_of_last_node_of_rod_2

…otation errors using rotational spring-damper systems
@mstoelzle
Copy link
Contributor Author

@xzhan139 What is the reason that the damping force for the SphericalJoint constraints is projected onto the normalized distance error?

relative_velocity = (
            rod_two.velocity_collection[..., index_two]
            - rod_one.velocity_collection[..., index_one]
        )
        normal_relative_velocity = (
            np.dot(relative_velocity, normalized_end_distance_vector)
            * normalized_end_distance_vector
        )
        damping_force = -self.nu * normal_relative_velocity

When adding damping to the torque compensation for the FixedJoint, do we need this projection as well?

@codecov-commenter
Copy link

codecov-commenter commented Jul 14, 2022

Codecov Report

Merging #135 (d84f5f1) into update-0.3.0 (735fe00) will decrease coverage by 0.06%.
The diff coverage is 84.21%.

@@               Coverage Diff                @@
##           update-0.3.0     #135      +/-   ##
================================================
- Coverage         87.30%   87.24%   -0.07%     
================================================
  Files                44       44              
  Lines              2695     2705      +10     
  Branches            360      360              
================================================
+ Hits               2353     2360       +7     
- Misses              324      326       +2     
- Partials             18       19       +1     
Impacted Files Coverage Δ
elastica/joint.py 23.68% <84.21%> (+1.57%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 09babf1...d84f5f1. Read the comment docs.

@bhosale2 bhosale2 linked an issue Jul 14, 2022 that may be closed by this pull request
@bhosale2 bhosale2 added the enhancement New feature or request label Jul 14, 2022
@bhosale2 bhosale2 added this to the Version 0.3 milestone Jul 14, 2022
@bhosale2
Copy link
Collaborator

bhosale2 commented Jul 14, 2022

@mstoelzle after discussing your question with @xzhan139, the conclusion is that there is no need to project it onto the normal vector. You can use directly the relative velocity field. In reality, contact damping is optional, since the internal dissipation of the rod helps in stabilizing the simulations.

@xzhan139 What is the reason that the damping force for the SphericalJoint constraints is projected onto the normalized distance error?

relative_velocity = (
            rod_two.velocity_collection[..., index_two]
            - rod_one.velocity_collection[..., index_one]
        )
        normal_relative_velocity = (
            np.dot(relative_velocity, normalized_end_distance_vector)
            * normalized_end_distance_vector
        )
        damping_force = -self.nu * normal_relative_velocity

When adding damping to the torque compensation for the FixedJoint, do we need this projection as well?

After you have done all necessary changes to the PR, please let us know or remove the WIP keyword from the PR name to open it for review.

@bhosale2 bhosale2 changed the title Fix #131 for FixedJoints by applying restoring torques on rotation errors [WIP] Fix #131 for FixedJoints by applying restoring torques on rotation errors Jul 14, 2022
- remove static rotation from test
- Improve checking of nut damping
- Fix sign error for computing restoring rotational damping torque
@mstoelzle mstoelzle changed the title [WIP] Fix #131 for FixedJoints by applying restoring torques on rotation errors Fix #131 for FixedJoints by applying restoring torques on rotation errors and add functionality to enforce static rotation offset between systems Jul 15, 2022
@mstoelzle
Copy link
Contributor Author

@mstoelzle after discussing your question with @xzhan139, the conclusion is that there is no need to project it onto the normal vector. You can use directly the relative velocity field. In reality, contact damping is optional, since the internal dissipation of the rod helps in stabilizing the simulations.

@xzhan139 What is the reason that the damping force for the SphericalJoint constraints is projected onto the normalized distance error?

relative_velocity = (
            rod_two.velocity_collection[..., index_two]
            - rod_one.velocity_collection[..., index_one]
        )
        normal_relative_velocity = (
            np.dot(relative_velocity, normalized_end_distance_vector)
            * normalized_end_distance_vector
        )
        damping_force = -self.nu * normal_relative_velocity

When adding damping to the torque compensation for the FixedJoint, do we need this projection as well?

After you have done all necessary changes to the PR, please let us know or remove the WIP keyword from the PR name to open it for review.

@bhosale2 This PR is now ready for review

Copy link
Collaborator

@skim0119 skim0119 left a comment

Choose a reason for hiding this comment

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

I only have minor comments. I think the implementation is not wrong but not so clear. @bhosale2 Please take a look.

I would suggest avoiding any branching, and make FixedJoint with rotational offset as a default behavior. @armantekinalp
If we need branching (static_rotation), we should have separate test for each cases.

examples/JointCases/joint_cases_callback.py Outdated Show resolved Hide resolved
elastica/joint.py Outdated Show resolved Hide resolved
elastica/joint.py Outdated Show resolved Hide resolved
elastica/joint.py Outdated Show resolved Hide resolved
elastica/joint.py Outdated Show resolved Hide resolved
elastica/joint.py Outdated Show resolved Hide resolved
elastica/joint.py Outdated Show resolved Hide resolved
elastica/joint.py Outdated Show resolved Hide resolved
elastica/joint.py Outdated Show resolved Hide resolved
@skim0119 skim0119 changed the title Fix #131 for FixedJoints by applying restoring torques on rotation errors and add functionality to enforce static rotation offset between systems Update FixedJoints: restoring spring-damped-torques, initial rotation offset Jul 15, 2022
Copy link
Collaborator

@bhosale2 bhosale2 left a comment

Choose a reason for hiding this comment

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

good job overall @mstoelzle, some critical comments to be addressed.

elastica/joint.py Outdated Show resolved Hide resolved
elastica/joint.py Outdated Show resolved Hide resolved
elastica/joint.py Outdated Show resolved Hide resolved
elastica/joint.py Outdated Show resolved Hide resolved
elastica/joint.py Outdated Show resolved Hide resolved
elastica/joint.py Show resolved Hide resolved
elastica/joint.py Outdated Show resolved Hide resolved
elastica/joint.py Outdated Show resolved Hide resolved
elastica/joint.py Outdated Show resolved Hide resolved
elastica/joint.py Outdated Show resolved Hide resolved
Copy link
Contributor

@armantekinalp armantekinalp left a comment

Choose a reason for hiding this comment

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

Good job @mstoelzle minor changes.

elastica/joint.py Outdated Show resolved Hide resolved
elastica/joint.py Show resolved Hide resolved
mstoelzle and others added 2 commits July 16, 2022 19:54
Co-authored-by: Arman Tekinalp <53585636+armantekinalp@users.noreply.github.com>
Co-authored-by: Arman Tekinalp <53585636+armantekinalp@users.noreply.github.com>
elastica/joint.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@bhosale2 bhosale2 left a comment

Choose a reason for hiding this comment

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

LGTM. @skim0119 and @armantekinalp will follow up.

@armantekinalp
Copy link
Contributor

@mstoelzle some CI tests are failing can you merge changes from update-0.3.0 to your branch.

@mstoelzle
Copy link
Contributor Author

@mstoelzle some CI tests are failing can you merge changes from update-0.3.0 to your branch.

Thanks. I will do that. There is also still some test issues I want to have a look at.

Copy link
Collaborator

@skim0119 skim0119 left a comment

Choose a reason for hiding this comment

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

It is probably the last set of changes. After you fix them, I'll run the testings and JointCases code, and approve.

examples/JointCases/joint_cases_callback.py Outdated Show resolved Hide resolved
elastica/joint.py Outdated Show resolved Hide resolved
tests/test_joint.py Outdated Show resolved Hide resolved
elastica/joint.py Show resolved Hide resolved
elastica/joint.py Show resolved Hide resolved
tests/test_joint.py Outdated Show resolved Hide resolved
@mstoelzle
Copy link
Contributor Author

Thanks everybody for your feedback. I have addressed all the requests for changes (in case I didn't miss anything) and this PR should be ready for merging now.

Copy link
Contributor

@armantekinalp armantekinalp left a comment

Choose a reason for hiding this comment

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

LGTM, nice work @mstoelzle

@skim0119 skim0119 merged commit c3f4c5a into GazzolaLab:update-0.3.0 Jul 19, 2022
@mstoelzle mstoelzle deleted the bugfix/fixed-joint-torsional-torque branch July 19, 2022 06:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Twisting torque is not constrained by FixedJoint and HingeJoint
5 participants