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

Fix planar joint Jacobian bug #3439

Merged
merged 6 commits into from May 28, 2023

Conversation

ivatavuk
Copy link
Contributor

Description

While working on this I think I found a bug in Jacobian calculation for planar joints.

The fix is simple, in lines performing joint_axis calculation, instead of joint_transform, joint_transform.linear() should be used, like for the other joint types.

I've written a simple test, I don't think it needs to be merged, but it demonstrates the bug so you can check it.
For the example in the test, current getJacobian implementation calculates the joint_axis of the first joint to be [1, 0, 0]^T when q[0] = 0, and [11, 0, 0]^T when q[0] = 10, which results in different Jacobian values.

@welcome
Copy link

welcome bot commented May 27, 2023

Thanks for helping in improving MoveIt and open source robotics!

@ivatavuk ivatavuk changed the title Planar joint Jacobian bug fix Fix planar joint Jacobian bug May 27, 2023
@codecov
Copy link

codecov bot commented May 27, 2023

Codecov Report

Patch coverage: 64.04% and no project coverage change.

Comparison is base (ba04810) 61.82% compared to head (9475dfb) 61.81%.

Additional details and impacted files
@@               Coverage Diff                @@
##           noetic-devel    #3439      +/-   ##
================================================
- Coverage         61.82%   61.81%   -0.00%     
================================================
  Files               380      385       +5     
  Lines             33610    34064     +454     
================================================
+ Hits              20775    21054     +279     
- Misses            12835    13010     +175     
Impacted Files Coverage Δ
...lude/moveit/collision_detection/collision_matrix.h 100.00% <ø> (ø)
...bot_model/include/moveit/robot_model/joint_model.h 89.34% <ø> (ø)
...bot_model/include/moveit/robot_model/robot_model.h 84.00% <ø> (ø)
...bot_state/include/moveit/robot_state/robot_state.h 85.12% <ø> (+0.76%) ⬆️
moveit_core/utils/src/xmlrpc_casts.cpp 0.00% <0.00%> (ø)
...moveit/move_group_interface/move_group_interface.h 36.85% <ø> (ø)
.../moveit/setup_assistant/tools/moveit_config_data.h 44.45% <ø> (ø)
...tup_assistant/src/tools/collision_linear_model.cpp 0.00% <0.00%> (ø)
...t_setup_assistant/src/tools/moveit_config_data.cpp 14.34% <0.00%> (-0.76%) ⬇️
moveit_core/robot_model/src/joint_model.cpp 52.76% <12.50%> (-2.70%) ⬇️
... and 14 more

... and 9 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@rhaschke rhaschke left a comment

Choose a reason for hiding this comment

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

Thanks for this fix.

@rhaschke rhaschke merged commit 18a6160 into moveit:noetic-devel May 28, 2023
7 checks passed
@welcome
Copy link

welcome bot commented May 28, 2023

Congrats on getting your first MoveIt pull request merged and improving open source robotics!

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.

None yet

2 participants