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

Update Backends Documentation #438

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Update Backends Documentation #438

wants to merge 4 commits into from

Conversation

yck011522
Copy link
Contributor

Updated the docs for describing Backend Features interfaces. Note that the examples where how two planners can be combined is speculative to a future PR where the planner functions are called directly.

@yck011522 yck011522 added the no changelog Disables changelog checker label Jun 20, 2024
Copy link
Member

@gonzalocasas gonzalocasas left a comment

Choose a reason for hiding this comment

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

After looking quite lengthy at the current state, your diagrams, the various examples and so on, I think the ClientInterface as lost a bit its raison d'etre, and the relationship robot-client-planner is upside down for no good reason.

I propose the following:

  • Client will no longer contain a planner, so all the forwarding of functions in ClientInterface goes away. Its responsibilites are mostly limited to just keeping a connection open to the external system.
  • Planners will contain a client as today (but the relation will not be recursive as it is right now).
  • Robot will not need a client anymore, instead it will need a planner instance in the constructor.

We discussed potentially removing the forwarding entirely from Robot, in which case, we could remove also planner from it completely.
However, this will have a huge impact on the API, and it affects all existing Grasshopper definitions that use COMPAS FAB quite a bit, so I say we keep the idea that Robot is the carrier of a connection to the planner by containing an instance of a planner.

This changes the relationship of the whole pipeline to be Robot -> Planner(composite of backend features) -> Client.

In code, this could look like this from an API perspective:

# Example 1: ROS + MoveIt, explicit instances
client = RosClient()
robot = client.load_robot()
robot.planner = MoveItPlanner(client)

robot.inverse_kinematics(frame)


# Example 2: PyBullet, explicit instances
client = PyBulletClient()
robot = client.load_robot(urdf_filename)
robot.planner = PyBulletPlanner(client)

robot.inverse_kinematics(frame)

# Example 3: ROS + future Tesseract planner, explicit instances, @jf---
client = RosClient()
robot = client.load_robot()
robot.planner = TesseractPlanner(client)

robot.inverse_kinematics(frame)

# Example 4: ROS + custom planner
class SlerpInterpolatedPlanCartesianMotion(PlanCartesianMotion):
    def plan_cartesian_motion(self, *args, **kwargs):
        # here add nice implementation of interpolated cartesian planner
        pass

# This planner inherits all the features from MoveIt, and only replaces the cartesian motion planning feature with a custom one.
class CustomMoveItPlanner(SlerpInterpolatedPlanCartesianMotion, MoveItPlanner):
    pass

client = RosClient()
robot = client.load_robot()
robot.planner = CustomMoveItPlanner(client)

robot.plan_cartesian_motion(waypoints)

# Example 5: default planner creation via factory method
client = RosClient()
robot = client.load_robot()
robot.planner = client.new_planner()

robot.inverse_kinematics(frame)

# Example 6: non-default planner created via factory method (this is an alternative syntax for example 1, it's not mutually exclusive)
client = RosClient()
robot = client.load_robot()
robot.planner = client.new_planner(CustomMoveItPlanner)

robot.plan_cartesian_motion(waypoints)

docs/developer/backends.rst Outdated Show resolved Hide resolved
docs/developer/backends.rst Outdated Show resolved Hide resolved
docs/developer/backends.rst Outdated Show resolved Hide resolved
ik_result = calculate_example_ik(robot, frame)
# or equivalently:
ik_result = calculate_example_ik.inverse_kinematics(robot, frame)
ik_result = robot.inverse_kinematics(robot, frame)


These backend feature interfaces exist in part to enforce a common
Copy link
Member

Choose a reason for hiding this comment

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

There's a typo in line 70 (not in your changes, but if you're updating, it, it makes sense to change. It says "These interfaces as exist to allow", but I guess it should read "These interfaces also exist to allow"

Comment on lines +56 to +61
# Instantiate the client and change its planner to the ExamplePlanner
client = MyExamplePlanner(robot)
client.planner = ExamplePlanner(client)
# Call the inverse kinematics method as usual
frame = Frame([0, 0, 0], [1, 0, 0], [0, 1, 0])
ik_result = calculate_example_ik(robot, frame)
# or equivalently:
ik_result = calculate_example_ik.inverse_kinematics(robot, frame)
ik_result = robot.inverse_kinematics(robot, frame)
Copy link
Member

Choose a reason for hiding this comment

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

This example doesn't work for multiple reasons:

  • client is assigned a MyExamplePlanner instance, which is not a client (did you mean MyExampleClient?)
  • MyExamplePlanner is anyway not the name of the example class above
  • robot is never assigned the client instance, so line 61 wouldn't work either
  • The call to robot would not have robot as first param, that's only if you call the IK feature directly skipping what the robot class does for you. Maybe you meant ik_result = robot.inverse_kinematics(frame)

But I think the example is not the best anyway. I would suggest changing it to limit it to a more concrete example of what actually works if the person reading up to here implements (and understand) only this part.

So, as a first applied example -in my opinion- the correct thing is to show only what was coded above and not more, ie. how to instantiate the planner and use the backend feature:

# you will need instances of client and robot
planner = ExamplePlanner(client)
ik_result = planner.inverse_kinematics(robot, frame)

Right after this semi-trivial example, we could a more elaborate one, but as you implied on the conversation on slack, it's kind of tricky to find one because the current client <-> planner <-> robot relationship is kind of convoluted.

One example using the current implementation could be this (notice that it depends on the tiny PR that I just sent):

# We will use ROS as an example of a client
with RosClient(planner_type=ExamplePlanner) as client:
   robot = client.load_robot()
   ik_result = robot.inverse_kinematics(frame)

Comment on lines +79 to +88
with ClientA(robot) as client_a, ClientB(robot) as client_b:
inverse_kinematics = client_a.planner.inverse_kinematics
plan_motion = client_b.planner.plan_motion

frame = Frame([0, 0, 0], [1, 0, 0], [0, 1, 0])
ik_result = inverse_kinematics(frame)

start_configuration = robot.zero_configuration()
goal_configuration = robot.random_configuration()
motion_plan = plan_motion(start_configuration, goal_configuration)
Copy link
Member

Choose a reason for hiding this comment

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

This second example in general has never made much sense. I think we could kill the entire ClientA/ClientB example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So do we remove the rest starting from "These interface also exist to allow mixing..."?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changelog Disables changelog checker
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants