Get joint state by name or index of joint #210
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
eeberhard
reviewed
Sep 16, 2021
| * @brief Getter of the positions attribute | ||
| * @brief Get joint index by the name of the joint, if it exists. | ||
| * @param joint_name The name of the desired joint | ||
| * @return The index of the joint, if it exists |
There was a problem hiding this comment.
Would be good to add a @details tag that it will throw a JointNotFoundException on failure
There was a problem hiding this comment.
This would actually be nice to add on each scalar getter too, just for explicitness. You can use @copydetails JointState::get_joint_index() and it will automatically fill in.
| #include <iostream> | ||
|
|
||
| namespace state_representation::exceptions { | ||
| class JointNotFoundException : public std::logic_error { |
There was a problem hiding this comment.
Exception class should also have a simple docstring with @class and @brief tags
| @@ -228,6 +249,15 @@ TEST(JointStateTest, GetSetData) { | |||
| EXPECT_THROW(js1.set_data(Eigen::Vector2d::Zero()), exceptions::IncompatibleSizeException); | |||
| } | |||
|
|
|||
| TEST(JointStateTest, GetNameByIndex) { | |||
There was a problem hiding this comment.
Suggested change
| TEST(JointStateTest, GetNameByIndex) { | |
| TEST(JointStateTest, GetIndexByName) { |
Comment on lines
83
to
87
| if (joint_index > this->get_size()) { | ||
| throw JointNotFoundException( | ||
| "Index '" + std::to_string(joint_index) + "' is out of range for joint state with size" | ||
| + std::to_string(this->get_size())); | ||
| } |
There was a problem hiding this comment.
there's a bit of code duplication in these four methods. could you define a local static method assert_index_in_range or similar that takes the index and size and does this check?
buschbapti
approved these changes
Sep 16, 2021
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.
As requested, the function
get_joint_indexandget_Xfor a single joint by providing the name or index of the joint. This fails and throws a new exceptionJointNotFoundExceptionin case the joint doesn't exist.Sorry for the large diff in JointState.hpp, I took the liberty to improve the doc a bit, trying to make it more consistent, becuase @eeberhard is always using the generated doc now :)