Refactor joint properties data structure (#110) #111
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
scchow
force-pushed
the
dev/joint-property-refactor
branch
2 times, most recently
from
c0267fa to
2d121c0
Compare
- Addresses moveit#110 - Refactored joint_properties to use a map of maps for streamlined access to joint properties - Removed JointProperty struct as information contained is redundant. - Updated tests to account for new getJointProperties signature
scchow
force-pushed
the
dev/joint-property-refactor
branch
from
2d121c0 to
06ccb85
Compare
rhaschke
approved these changes
Mar 20, 2023
include/srdfdom/model.h
Outdated
| // Empty joint property vector | ||
| static const std::vector<JointProperty> empty_vector_; | ||
| // joint name -> (property name -> property value) | ||
| std::map<std::string, std::map<std::string, std::string>> joint_properties_; |
There was a problem hiding this comment.
I prefer introducing descriptive alias names for such types.
include/srdfdom/model.h
Outdated
| std::map<std::string, std::map<std::string, std::string>> joint_properties_; | ||
|
|
||
| // Empty joint property map | ||
| static const std::map<std::string, std::string> empty_map_; |
There was a problem hiding this comment.
I suggest to move this into the function requiring it.
- Use type aliases - Move static empty_map into the function it is used within
|
Please adapt your MoveIt PR accordingly. |
|
Thanks for cleaning this up! |
|
Thanks for the helpful comments! Good call on using the descriptive aliases. I'll put together a similar patch for the ROS2 branch if I can find some time this/next week. |
scchow
added a commit
to scchow/moveit
that referenced
this pull request
Apr 10, 2023
- Due to moveit/srdfdom#111, accessing Joint Properties has changed since properties now use a map instead of a vector. - Fixed tests/checking of joint properties to reflect this change
scchow
added a commit
to scchow/moveit
that referenced
this pull request
Apr 13, 2023
- Due to moveit/srdfdom#111, accessing Joint Properties has changed since properties now use a map instead of a vector. - Fixed tests/checking of joint properties to reflect this change
rhaschke
added a commit
to moveit/moveit
that referenced
this pull request
Apr 14, 2023
backport from moveit/moveit2#390 with several improvements: - accessing joint properties via map of maps (moveit/srdfdom#111) - fixing joint properties parsing, using `moveit::utils::toDouble` instead of `std::stod` - cleanup parsing of joint properties Co-authored-by: Robert Haschke <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR addresses #110 for Noetic:
getJointProperties().After doing incorporating these changes, I realized that this would significantly change functionality in the following ways:
getJointProperties()JointPropertystructAs a result, if this PR gets merged, I will need to update my PR in MoveIt! (moveit/moveit#3359) to account for the modified data structures here.
Since the backport that I did for ROS1 is relatively new, merging this shouldn't cause many problems, as I doubt anyone else has started using the features I backported 2 weeks ago.
However, we should be careful about doing the same for ROS2, since it will likely break things in MoveIt2 (such as the same test in MoveIt2 here and as well as anyone else who were relying on the original joint properties data structure/function signatures).
I can definitely work on a similar PR for ROS2, but wanted to get your thoughts first.
Cheers!