-
Notifications
You must be signed in to change notification settings - Fork 0
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
feat(state-representation): add utilities for CartesianStateVariable #196
Conversation
4a9cc3c
to
f345baf
Compare
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.
Looks good, just one nitpick
source/state_representation/include/state_representation/space/cartesian/CartesianState.hpp
Outdated
Show resolved
Hide resolved
...ate_representation/include/state_representation/exceptions/InvalidStateVariableException.hpp
Outdated
Show resolved
Hide resolved
Okay first part is done, now you also need to update the python bindings. Let me know if you need help with this |
c7f4b06
to
7a47229
Compare
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.
Nice
Co-authored-by: Dominic Reber <[email protected]>
I noticed we didn't make the move to C++20 in control-libraries. Should we do that (in general)? Here or separate PR? |
Not sure what that would imply. What would we gain from it? You could also open a backlog issue to do the equivalent changes in JointState |
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.
Works for me
at the moment nothing, it's not blocking. It's more for future proofing and to gain access to some more modern features that are always nice to have. (definitely not an immediate need) |
Description
This PR solves the issue by making
get_state_variable
andset_state_variable
public, and also adding two support functions to convertCartesianStateVariable
s from/to string.Review guidelines
Estimated Time of Review: 5 minutes
Checklist before merging: