Skip to content

feat(state-representation): add utilities for CartesianStateVariable#196

Merged
bpapaspyros merged 7 commits into
mainfrom
feat/cartesian_state_utilities
Oct 17, 2024
Merged

feat(state-representation): add utilities for CartesianStateVariable#196
bpapaspyros merged 7 commits into
mainfrom
feat/cartesian_state_utilities

Conversation

@bpapaspyros

@bpapaspyros bpapaspyros commented Oct 16, 2024

Copy link
Copy Markdown

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
domire8
domire8 reviewed Oct 17, 2024
View reviewed changes

@domire8 domire8 left a comment

Copy link
Copy Markdown
Member

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

Comment thread source/state_representation/include/state_representation/space/cartesian/CartesianState.hpp Outdated
@domire8

domire8 commented Oct 17, 2024

Copy link
Copy Markdown
Member

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
domire8
domire8 reviewed Oct 17, 2024
View reviewed changes

@domire8 domire8 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice

Comment thread python/source/state_representation/bind_exceptions.cpp Outdated
@bpapaspyros @domire8
Apply suggestions from code review
6724ccc 
Co-authored-by: Dominic Reber <71256590+domire8@users.noreply.github.com>
@bpapaspyros

bpapaspyros commented Oct 17, 2024

Copy link
Copy Markdown
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

domire8 commented Oct 17, 2024

Copy link
Copy Markdown
Member

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

domire8
domire8 approved these changes Oct 17, 2024
View reviewed changes

@domire8 domire8 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Works for me

@bpapaspyros

bpapaspyros commented Oct 17, 2024

Copy link
Copy Markdown
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)

This was referenced Oct 17, 2024
Closed
Merged
@bpapaspyros bpapaspyros merged commit 3dd80ce into main Oct 17, 2024
@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.

Reviewers

@domire8 domire8 domire8 approved these changes

@eeberhard eeberhard Awaiting requested review from eeberhard eeberhard is a code owner

Assignees

@bpapaspyros bpapaspyros

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@bpapaspyros @domire8