Trajectory classes - #270
Conversation
f3b47f1 to
a59e74b
Compare
| ANALOG_IO_STATE = 19, | ||
| CARTESIAN_TRAJECTORY = 20, | ||
| JOINT_TRAJECTORY = 21, | ||
| #ifdef EXPERIMENTAL_FEATURES |
There was a problem hiding this comment.
Maybe it would be time to remove those
|
NOTE: this is a staging branch. I would invite you to review these changes as a baseline for what is to come next. Once I have approval for this part, I will open PRs for python implementation as well as the clproto updates next. |
a59e74b to
a43c74c
Compare
| /** | ||
| * @brief Delete the last point from trajectory | ||
| */ | ||
| void delete_point(); |
There was a problem hiding this comment.
Sorry, the delete_point by index. But I imagine it's just handy.
| * @param points vector of new points | ||
| * @throw IncompatibleSizeException if points vector is empty | ||
| */ | ||
| void set_points(const std::vector<TrajectoryT>& points); |
There was a problem hiding this comment.
need to provide exactly the points size of the current trajectory, maybe that should be mentioned
| /** | ||
| * @brief Set the trajectory points from a vector of points | ||
| * @param points vector of new points | ||
| * @throw IncompatibleSizeException if points vector is empty |
There was a problem hiding this comment.
or if the provided points size does not match the current size
| void add_point(const TrajectoryT& new_point); | ||
|
|
||
| /** | ||
| * @brief Add new points to trajectory | ||
| * @param new_point the new point | ||
| * @throw IncompatibleSizeException if points vector is empty | ||
| */ | ||
| void add_points(const std::vector<TrajectoryT>& new_points); | ||
|
|
||
| /** | ||
| * @brief Insert new trajectory point between two already existing points | ||
| * @param new_point the new point | ||
| * @param index the desired position of the new point in the queue | ||
| * @throw std::out_of_range if index is out of range | ||
| */ | ||
| void insert_point(const TrajectoryT& new_point, unsigned int index); | ||
|
|
||
| /** | ||
| * @brief Set the trajectory point at given index | ||
| * @param point the new point | ||
| * @param index the index | ||
| * @throw std::out_of_range if index is out of range | ||
| */ | ||
| void set_point(const TrajectoryT& point, unsigned int index); |
There was a problem hiding this comment.
If you read through it's obvious what each does, but I feel we could maybe merge 'add' and 'insert' with an overload maybe.
There was a problem hiding this comment.
That could be neat indeed and it would also resolve the previous point about insert_points
| * @param joint_names the joint names of the trajectory point | ||
| * @return the joint state representation of the trajectory point | ||
| */ | ||
| JointState to_joint_state(const std::vector<std::string>& joint_names) const { |
There was a problem hiding this comment.
how about the opposite/symmetric in JointState?
There was a problem hiding this comment.
I'm not sure what you mean?
There was a problem hiding this comment.
Both in this and the next comment: a method to_trajectory_point for Joint/CartesianStates. But now that I see this again, not sure how beneficial it would be, the constructor is easy enough to use, and you would would have to provide the duration either way.
| * @param reference_frame the underlying reference frame of the trajectory point | ||
| * @return the Cartesian state representation of the trajectory point | ||
| */ | ||
| CartesianState to_cartesian_state(const std::string& reference_frame) const { |
| * @brief Get list of trajectory points | ||
| * @return queue of the Cartesian states of the trajectory |
There was a problem hiding this comment.
Just noticed, but probably exists before; are we using points and states interchangeably? I am afraid it might get a bit confusing. Especially since we are returning actual points from the base class, but states from here.
There was a problem hiding this comment.
Yeah I can see where you are coming from, let me think about it. But users should not bother with what's in the base class, those are not public interfaces of this class
| * @param index the desired position of the new point in the queue | ||
| * @throw std::out_of_range if index is out of range | ||
| */ | ||
| void insert_point(const TrajectoryT& new_point, unsigned int index); |
There was a problem hiding this comment.
You think we need an insert_points as well?
3 participants
Description
It is time to revisit the implementation of trajectory classes from some time ago. Please review this while keeping all our work on different trajectory controllers in mind, asking yourself what do I need from a trajectory representation, what interfaces would be convenient?
Currently, the proposed implementation has the following interfaces:
get_durationby indexget_durationsall of themget_time_from_startby indexget_times_from_startall of themget_trajectory_durationget_sizedelete_pointthe last one, or by indexresetget_pointsget all statesget_pointget the state by indexadd_pointwith state + durationadd_pointswith states + durationsinsert_pointwith state and duration by indexset_pointwith state and duration by indexset_pointswith all states and durationsoperator[]to getpair<state, duration>by indexSomething that I don't like a loot is that we don't have getters for the durations in
doublebut onlystd::chrono::nanosecondsbut at the same time, we usually have to translate to a ROS message anyway so it doesn't really matter. It's also more correct to usestd::chronotypes.Review guidelines
Estimated Time of Review: 15 minutes
Checklist before merging: