Plan_cartesian_motion uses Waypoints class #422
Conversation
…nt support for FrameWaypoints . PointAxisWaypoints will be in next PR
src/compas_fab/backends/kinematics/analytical_plan_cartesian_motion.py
Outdated
Show resolved
Hide resolved
src/compas_fab/robots/robot.py
Outdated
| If a tool is attached to the robot, the :meth:`~compas_fab.robots.Waypoints.tool_coordinate_frame` parameter | ||
| should be set. |
There was a problem hiding this comment.
I would rephrase this, I have a hard time parsing what it means.
| use_attached_tool_frame : :obj:`bool`, optional | ||
| If ``True`` and there is a tool attached to the planning group, it will use its TCF | ||
| instead of the T0CF to calculate cartesian paths. Defaults to ``True``. |
There was a problem hiding this comment.
I'm not convinced with the removal of this. I think we need to review the idea of having tool_coordinate_frame in the targets and waypoints classes. It feels to me like these could contain the name or key of the tool to use, but the coordinate frame should not be attached inside them, otherwise, if we serialize that, and re-measure the tool, the waypoints need to be somehow retrofitted/updated with the new tcf. That doesn't make much sense.
| def test_kinematics_cartesian_with_tool_coordinate_frame(frame_waypoints): | ||
| if compas.IPY: | ||
| return | ||
| frame_waypoints.tool_coordinate_frame = Frame([0.01, 0.02, -0.03], [1, 0, 0], [0, 1, 0]) |
There was a problem hiding this comment.
Again this feel incorrect. When seeing it here in a full example, it's clearer that is wrong imo. The tool needs to be defined with a key and a TCF, and the waypoints should only say e.g. frame_waypoints.tool_id = "suction_gripper"
src/compas_fab/backends/kinematics/analytical_plan_cartesian_motion.py
Outdated
Show resolved
Hide resolved
src/compas_fab/backends/kinematics/analytical_plan_cartesian_motion.py
Outdated
Show resolved
Hide resolved
src/compas_fab/backends/kinematics/analytical_plan_cartesian_motion.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Gonzalo Casas <[email protected]>
Co-authored-by: Gonzalo Casas <[email protected]>
| # If the user provides a transformation, convert it to a Frame | ||
| if isinstance(tool_coordinate_frame, Transformation): | ||
| tool_coordinate_frame = Frame.from_transformation(tool_coordinate_frame) | ||
| self.tool_coordinate_frame = tool_coordinate_frame |
There was a problem hiding this comment.
This is problematic because it's redundant information: the tool has its frame defined, but this will repeat the same data, losing track of its source, so if the tool changes frame, the target has no way to know the data is incorrect.
There was a problem hiding this comment.
We're merging this, with the pending remark about #435 that really needs to be checked and decided on.
This is a short PR to migrate the plan_cartrsian_motion to use Waypoints class.
At the moment, only the FrameWaypoints are supported as no implementation is added for PointAxisWaypoints yet. Will do that in next PR.
What type of change is this?
Checklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.CHANGELOG.mdfile in theUnreleasedsection under the most fitting heading (e.g.Added,Changed,Removed).invoke test).invoke lint).compas_fab.robots.CollisionMesh.