Skip to content
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

Merged
merged 7 commits into from
Oct 17, 2024

Conversation

bpapaspyros
Copy link
Member

Description

This PR solves the issue by making get_state_variable and set_state_variable public, and also adding two support functions to convert CartesianStateVariables from/to string.

Review guidelines

Estimated Time of Review: 5 minutes

Checklist before merging:

  • Confirm that the relevant changelog(s) are up-to-date in case of any user-facing changes

@bpapaspyros bpapaspyros self-assigned this Oct 16, 2024
@bpapaspyros bpapaspyros requested a review from domire8 October 16, 2024 12:27
@bpapaspyros bpapaspyros force-pushed the feat/cartesian_state_utilities branch from 4a9cc3c to f345baf Compare October 16, 2024 13:18
@bpapaspyros bpapaspyros marked this pull request as ready for review October 16, 2024 13:21
@bpapaspyros bpapaspyros requested a review from eeberhard October 16, 2024 13:21
Copy link
Member

@domire8 domire8 left a 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

@domire8
Copy link
Member

domire8 commented Oct 17, 2024

Okay first part is done, now you also need to update the python bindings. Let me know if you need help with this

@bpapaspyros bpapaspyros force-pushed the feat/cartesian_state_utilities branch from c7f4b06 to 7a47229 Compare October 17, 2024 09:07
Copy link
Member

@domire8 domire8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice

python/source/state_representation/bind_exceptions.cpp Outdated Show resolved Hide resolved
@bpapaspyros
Copy link
Member Author

Nice

I noticed we didn't make the move to C++20 in control-libraries. Should we do that (in general)? Here or separate PR?

@domire8
Copy link
Member

domire8 commented Oct 17, 2024

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

Copy link
Member

@domire8 domire8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me

@bpapaspyros
Copy link
Member Author

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?

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)

@bpapaspyros bpapaspyros merged commit 3dd80ce into main Oct 17, 2024
5 checks passed
@bpapaspyros bpapaspyros deleted the feat/cartesian_state_utilities branch October 17, 2024 10:46
@github-actions github-actions bot locked and limited conversation to collaborators Oct 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants