-
Notifications
You must be signed in to change notification settings - Fork 91
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 xyz and rpy offsets in fixed joint reduction #500
Conversation
Signed-off-by: Ian Chen <ichen@osrfoundation.org>
Signed-off-by: Ian Chen <ichen@osrfoundation.org>
The fact that it generates xyzOffset: -0.707108, -1.70711, 0 before the updates in this PR confuse me. If it was doing the inverse (i.e, base_link relative to link2), wouldn't it be Aside from that, the changes makes sense to me, but like you said, this is problematic for gazebo_ros_p3d. Particularly, gazebo_ros_p3d.cpp#L295 has
If we merge this PR, we need to update that line to
|
The If I set the |
I worked through the math in The code currently does: but for it to be the inverse of where: Also, I noticed that with the changes in this PR, |
Signed-off-by: Ian Chen <ichen@osrfoundation.org>
thanks for checking the math. I've removed the |
here is the gazebo_ros_pkgs PR for the updating the rotation offset calculation: ros-simulation/gazebo_ros_pkgs#1241 |
What's the status of this PR? It's been approved for a while. |
fix for Ubuntu CI in #626 |
Codecov Report
@@ Coverage Diff @@
## sdf6 #500 +/- ##
==========================================
+ Coverage 65.70% 66.40% +0.69%
==========================================
Files 88 88
Lines 10163 10160 -3
==========================================
+ Hits 6678 6747 +69
+ Misses 3485 3413 -72
Continue to review full report at Codecov.
|
I wonder if we should try to inject a |
Signed-off-by: Ian Chen <ichen@osrfoundation.org>
alright I implemented this suggestion in 536b847. One question, do we need the |
well, since it's in a plugin none of the tags are part of the spec, so a namespace wouldn't be required for that reason, but since we are injecting the element, I feel better about using a namespace so we don't conflict with any preexisting user tags. I think it's unlikely, so we could probably be fine without the namespace, but I have a slight preference for it. If others feel more strongly about not using |
ok sounds good! I'll keep the ignition namespace |
I just noticed that it seems to be inserting two |
src/parser_urdf.cc
Outdated
@@ -3473,14 +3444,21 @@ void ReduceSDFExtensionPluginFrameReplace( | |||
// remove xyzOffset and rpyOffset | |||
(*_blobIt)->RemoveChild(rpyKey); | |||
} | |||
TiXmlNode* correctedOffsetKey = | |||
(*_blobIt)->FirstChild("ignition::corrected_offsets"); | |||
if (!correctedOffsetKey) |
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 think the logic is backwards here. Should the !
be removed?
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.
removed in 2eba874
Signed-off-by: Steven Peters <scpeters@openrobotics.org>
should be fixed by 2eba874 |
thanks! |
There is special handling in the parser_urdf code to update plugin fields when links are merged together during fixed joint reduction. A test for this was added to sdf6 in gazebosim#500. A portion of this test is applied directly to the sdf10 branch to illustrate that it is broken. Signed-off-by: Steve Peters <scpeters@openrobotics.org>
There is special handling in the parser_urdf code to update plugin fields when links are merged together during fixed joint reduction. A test for this was added to sdf6 in #500. A portion of this test is applied directly to the sdf10 branch to illustrate a problem with ReduceSDFExtensionPluginFrameReplace in parser_urdf.cc. The original migration to use tinyxml2 in #264 changed the data structure used in SDFExtension to store XMLDocuments instead of XMLPointers, which requires an extra call to FirstChildElement, but the ReduceSDFExtension*FrameReplace functions did not receive this update. The fix here refactors the function arguments to pass the first child element directly, which simplifies the helper function implementation. Signed-off-by: Steve Peters <scpeters@openrobotics.org>
This pull request has been mentioned on Gazebo Community. There might be relevant details there: https://community.gazebosim.org/t/new-ignition-releases-2022-01-10/1228/1 |
This pull request has been mentioned on Gazebo Community. There might be relevant details there: https://community.gazebosim.org/t/new-ignition-releases-2022-03-25-fortress-edifice-citadel/1343/1 |
Continuation of the fix from pull request #497. This is another report and contribution from external user.
The
xyzOffset
andrpyOffset
computation seems to be incorrect for a model plugin during fixed joint reduction.This diagram describes the urdf configuration that is being tested:
The model has three links connected together by two fixed joints with [0 1 0] pos and [0 0 pi/4] rotation joint origin as shown in the diagram (see
test/integration/fixed_joint_reduction_plugin_frame_extension.urdf
). The model plugin has these relevant params:After fixed joint reduction,
link1
andlink2
are merged intobase_link
and hence thexyzOffset
andrpyOffset
values are also modified to be in thebase_link
frameBefore the changes: the urdf parser produces these offsets:
bodyName
: "base_link"xyzOffset
: -0.707108, -1.70711, 0rpyOffset
: 0, 0, -1.5708Looking at the offsets and also the diagram above, these values do not look correct.
After the changes: the urdf parser produces these offsets:
bodyName
: "base_link"xyzOffset
: -0.707108, 1.70711, 0rpyOffset
: 0, 0, 1.5708These values match what's shown in the diagram.
Note the code has been around for many years so I appreciate a closer look at this. Looking at gazebo_ros_pkgs, the one plugin that could be affected is the gazebo_ros_p3d plugin, which is a model plugin and makes use of these offsets. The issue only happens if the user has this plugin and references a frame that's being reduced due to fixed joints. Here's a related issue in gazebo_ros_pkgs. The comments suggest that people have worked around it by disabling fixed joint lumping or using continuous joints with zero limits.
Signed-off-by: Ian Chen ichen@osrfoundation.org