-
Notifications
You must be signed in to change notification settings - Fork 568
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 bug in GetUrdfService move_group capability #2669
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #2669 +/- ##
==========================================
- Coverage 50.74% 41.08% -9.66%
==========================================
Files 392 690 +298
Lines 32553 56233 +23680
Branches 0 7288 +7288
==========================================
+ Hits 16517 23099 +6582
- Misses 16036 32971 +16935
- Partials 0 163 +163 ☔ View full report in Codecov by Sentry. |
Reading and writing an XML document with substringing and string concatenation seems very fragile. Using something like TinyXML2 would avoid parsing mistakes like this in the future. |
I tried tinyxml2 first but I had a really hard time using it (very little documentation and bad error messaging) and it did not succeed in the end. If we allocate more time for this, I could give it another try but for now I think we should fix this. |
moveit_ros/move_group/src/default_capabilities/get_group_urdf_capability.cpp
Outdated
Show resolved
Hide resolved
Isn't there a way to implement the same thing just using urdfdom? It seems very odd to me that we have to run manual xml string parsing and editing. |
Looks like it is possible to export the parsed model to a TiXmlDoment type (tinyxml, not tinyxml2!) but not to a string :/ I guess creating a submodel from a given URDF is not a common usecase 🤷 |
This PR LGTM since it's fixing a bug in code that is already there. |
Description
The capability did not take into account that it is possible to have link elements that close with "/>". This PR fixes that
Checklist