-
Notifications
You must be signed in to change notification settings - Fork 7
Custom mpc behavior and residual #425
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
Conversation
| /** | ||
| * @brief TODO(...) | ||
| */ | ||
| class CustomMpcBehavior : public moveit_studio::behaviors::MPCBehaviorBase<CustomMpcBehavior> |
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.
so if this is custom, I assume we already have a bunch of core ones? and this is an example of a user creating one?
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.
I believe we have MPCPoseTracking and MPCPointCloudClearance so far.
| * @brief Define the residuals used by this MPC behavior. | ||
| * @details This method allows the use of a MPCProblem instance to add residual-specific ports. | ||
| * @return A BT::PortsList containing the required ports for the behavior. | ||
| */ |
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.
so, as with the README, I am getting a bit confused by residuals, so they are required to be defined for every behavior and contribute to the cost optimization? Are there some always populated by default, or does it depend on what the model is?
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.
I think they are essentially costs in the cost function, so they're always required, and there's no default. It's tightly coupled to the model, since the mujoco entities used in the residual must exist in the model.
| : moveit_studio::behaviors::MPCBehaviorBase<CustomMpcBehavior>(name, config, shared_resources) | ||
| { | ||
| } | ||
|
|
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.
since this doesn't have much in it, is this just to facilitate the creation of the new 'site_tracking' residual?
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.
Not just site_tracking. All the residuals are defined in getResiduals() in the .hpp. It does seem a little bit strange. We should probably add some documentation to the docs page about how to make an MPC behavior? (e.g., copy this template, then fill out the getResiduals() function tuple?)
|
|
||
| int SiteTrackingResidualFunction::getNumDims() const | ||
| { | ||
| return 6; |
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.
is this always true? is this a specific mujoco model that's always used?
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.
This is for a 3D spatial pose target, which is always 6 dof.
| // Test that ClassLoader is able to find and instantiate each behavior using the package's plugin description info. | ||
| EXPECT_NO_THROW((void)factory.instantiateTreeNode("test_behavior_name", "CustomMpcBehavior", BT::NodeConfiguration())); | ||
| } | ||
|
|
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.
should there be a more specific residuals based unit test? Making sure your simulated timestep forward produces a valid position/orientation, checking for bad inputs, etc
|
|
||
| // ---------- Residual (0-2) ---------- | ||
| const mjtNum delta_time = data.time - initial_data->time; | ||
|
|
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.
is it ok if this is negative?
| current_target_pos[2] = target_position[2] + target_position_velocity[2] * delta_time; | ||
|
|
||
| const mjtNum* tip_xpos_current = &data.site_xpos[3 * site_tip_ind_]; | ||
|
|
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.
index is always guaranteed to be 1?
| residual[2] = tip_xpos_current[2] - current_target_pos[2]; | ||
|
|
||
| // ---------- Residual (3-5) ---------- | ||
|
|
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.
so you mean the residual state of the rotation after simulating forward one timestep?
| residual[3] = relative_quat_target[0]; | ||
| residual[4] = relative_quat_target[1]; | ||
| residual[5] = relative_quat_target[2]; | ||
| } |
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.
I'm still confused how this is actually used. So you move the position and orientation forward one timestep then subtract it from data from mujoco? to get the difference between the actual and the desired?
then how is this being used later?
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.
This should be finding the difference between the target_pose and the tip_pose at the same (current) time-step.
bkanator
left a comment
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.
just adding comments to get familiar with the purpose of the code
rlpratt12
left a comment
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.
FYI - I guess we'll need to retarget to re-cored example_ws, so just move all these changes into your Core branch?
There are a ton of ports for CustomMpcBehavior, that I don't see documented anywhere. We should probably make that a section in the docs? Default ports? And explain them and some heuristics on reasonable values and how to tune them?
differentiable, follow_joint_trajectory_topic, gradient_num_trajectory, gradient_spline_points, sampling_control_width, sampling_exploration, sampling_sample_width, sampling_spline_points, set_ctrl_command_to_current_state, total_return, warmup_iterations...
residual specific:
the "max" param for each residual: e.g. max_cartesian_acceleration_goal
|
|
||
| int SiteTrackingResidualFunction::getNumDims() const | ||
| { | ||
| return 6; |
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.
This is for a 3D spatial pose target, which is always 6 dof.
| * @brief Define the residuals used by this MPC behavior. | ||
| * @details This method allows the use of a MPCProblem instance to add residual-specific ports. | ||
| * @return A BT::PortsList containing the required ports for the behavior. | ||
| */ |
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.
I think they are essentially costs in the cost function, so they're always required, and there's no default. It's tightly coupled to the model, since the mujoco entities used in the residual must exist in the model.
| /** | ||
| * @brief TODO(...) | ||
| */ | ||
| class CustomMpcBehavior : public moveit_studio::behaviors::MPCBehaviorBase<CustomMpcBehavior> |
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.
I believe we have MPCPoseTracking and MPCPointCloudClearance so far.
|
|
||
| BT::KeyValueVector CustomMpcBehavior::metadata() | ||
| { | ||
| return { { "description", "Custom MPC behavior" }, { "subcategory", "Motion - Execute" } }; |
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.
Update category. Add the inline constexpr auto kDescription... above.
| /** | ||
| * \brief Residual functions for tracking a moving pose. | ||
| */ | ||
| class SiteTrackingResidualFunction final : public moveit_pro_mpc::ResidualFunctionInterface |
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.
We shoudn't be duplicating SiteTrackingResidualFunction here. See my comment in your Core PR, we need a better way to expose examples like this without copying code.
| : moveit_studio::behaviors::MPCBehaviorBase<CustomMpcBehavior>(name, config, shared_resources) | ||
| { | ||
| } | ||
|
|
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.
Not just site_tracking. All the residuals are defined in getResiduals() in the .hpp. It does seem a little bit strange. We should probably add some documentation to the docs page about how to make an MPC behavior? (e.g., copy this template, then fill out the getResiduals() function tuple?)
| residual[3] = relative_quat_target[0]; | ||
| residual[4] = relative_quat_target[1]; | ||
| residual[5] = relative_quat_target[2]; | ||
| } |
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.
This should be finding the difference between the target_pose and the tip_pose at the same (current) time-step.
No description provided.