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

Named joint states helper #71

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 59 additions & 0 deletions examples/set_get_joints_example.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
"""
This example shows PyRoboPlan capabilities for getting and setting joint states easily with named configurations.
"""

from pinocchio.visualize import MeshcatVisualizer

from pyroboplan.core.utils import (
get_random_collision_free_state,
)
from pyroboplan.models.two_dof import load_models, add_object_collisions
from pyroboplan.visualization.set_get_joints import (
NamedJointConfigurationsOptions,
NamedJointConfigurations,
)

if __name__ == "__main__":
# Show an example saved joints states with the 2DoF robot
model, collision_model, visual_model = load_models()
add_object_collisions(model, collision_model, visual_model)
ee_name = "ee"
data = model.createData()
collision_data = collision_model.createData()

# Define the initial configuration
q = get_random_collision_free_state(model, collision_model, distance_padding=0.1)

# Initialize
named_joints_configuration = NamedJointConfigurations(model, collision_model)
named_joints_configuration["home"] = q
Copy link
Owner

Choose a reason for hiding this comment

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

it would be nice to save the "home" pose in the actual src/pyroboplan/models/two_dof.py, so you could retrieve an initial set of joint configurations and add to them.


# Initialize visualizer
visualizer = MeshcatVisualizer(model, collision_model, visual_model, data=data)
visualizer.initViewer(open=True)
visualizer.loadViewerModel()
visualizer.display(q)

while True:
# get user input
user_input = input(
"Press 'Enter' to show another random state, enter a new name to save the current state as a named joint configuration, enter a used name to visualize that saved state, type 'h' or 'help' to see the existing saved joint configurations, or ctrl-c to quit.\n"
Copy link
Owner

@sea-bass sea-bass Sep 14, 2024

Choose a reason for hiding this comment

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

highly recommend splitting up this into multiple lines -- it was a bit hard to read.

)
print()

if user_input:
# if the input isn't empty
user_input = user_input.lower()
if user_input == "h" or user_input == "help":
print("Stored states:")
print(named_joints_configuration)
Comment on lines +48 to +49
Copy link
Owner

Choose a reason for hiding this comment

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

Might be good to also show their numeric values. Perhaps this can be some kind of print() or show() method of the NamedJointConfigurations class itself?

if user_input in named_joints_configuration:
named_joints_configuration.visualize_state(visualizer, user_input)
else:
named_joints_configuration[user_input] = q
Comment on lines +44 to +53
Copy link
Owner

Choose a reason for hiding this comment

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

You probably also want to add confirmation that a new named state was added / the robot is going to an existing state.

else:
# if the input is empty, make a new state
q = get_random_collision_free_state(
model, collision_model, distance_padding=0.1
)
visualizer.display(q)
19 changes: 19 additions & 0 deletions src/pyroboplan/core/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,25 @@ def check_within_limits(model, q):
)


def check_valid_pose(model, q):
Copy link
Owner

Choose a reason for hiding this comment

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

can you add a unit test for this function as well?

"""
Checks whether a particular joint configuration is a valid configuration for the given model.

Parameters
----------
model : `pinocchio.Model`
The model for which to check the joint configuration's validity.
q : array-like
The joint configuration to check.

Returns
-------
bool
True if the configuration q is a possible configuration of the given model
"""
return len(q) == model.nq and check_within_limits(model, q)
Copy link
Owner

Choose a reason for hiding this comment

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

nit: put parentheses around these conditions



def extract_cartesian_pose(model, target_frame, q, data=None):
"""
Extracts the Cartesian pose of a specified model frame given a joint configuration.
Expand Down
98 changes: 98 additions & 0 deletions src/pyroboplan/visualization/set_get_joints.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
from pyroboplan.core.utils import (
Copy link
Owner

Choose a reason for hiding this comment

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

not sure if visualization is the right module for this, it's probably good under src/pyroboplan/core as a new file there.

Also the name set_get_joints.py maybe should be replaced with named_joint_configurations.py or something like that?

check_collisions_at_state,
check_valid_pose,
check_within_limits,
)


class NamedJointConfigurationsOptions:
"""Options for named joint configurations"""

def __init__(self, allow_collisions=False):
"""
Initializes a set of named joint configuration options.

Parameters
----------
allow_collisions : bool
Whether to allow in-collision states to be stored.
"""
self.allow_collisions = allow_collisions


class NamedJointConfigurations:
"""Named joint configurations helper

This is a simple helper to get, set, and visualize joint states for a given model.
Each model should have its own NamedJointConfigurations, and validation is performed to
ensure joint states are dimensionally valid and, optionally, within joint limits.

"""

def __init__(
self, model, collision_model, options=NamedJointConfigurationsOptions()
):
"""
Creates an instance of a named joint configurations object.

Parameters
----------
model : `pinocchio.Model`
The model to use for this named joint configurations.
collision_model : `pinocchio.Model`
The model to use for collision checking.
options : `NamedJointConfigurationsOptions`, optional
The options to use for this named joint configurations object.
If not specified, default options are used.
"""
self.model = model
self.collision_model = collision_model
self.data = self.model.createData()
self.collision_data = self.collision_model.createData()
self.options = options
self.configuration_dict = {}

Copy link
Author

Choose a reason for hiding this comment

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

I should also override __delitem__, I think.

Copy link
Owner

Choose a reason for hiding this comment

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

Definitely, removing named configurations is well within reason

def __setitem__(self, key, state):
"""
Set the configuration at key to state
"""
# first, check if the state is valid
if not check_valid_pose(self.model, state):
if not check_within_limits(self.model, state):
Comment on lines +60 to +61
Copy link
Owner

Choose a reason for hiding this comment

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

I think you may want to reconsider this function... since check_valid_pose already calls check_within_limits internally, that seems redundant. Is it worth maybe just removing that function and doing two separate (non-nested) checks for length and then limits?

raise ValueError("Tried to add state outside of model's joint limits")
raise ValueError(
f"Tried to add state of dimension {len(state)} to model with number of joints {self.model.nq}"
)

# then, check collisions if necessary
if not self.options.allow_collisions:
if check_collisions_at_state(self.model, self.collision_model, state):
Comment on lines +68 to +69
Copy link
Owner

Choose a reason for hiding this comment

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

you can just and these 2 instead of more nesting -- but this is exactly the "check and early return" pattern you should use for the previous checks too

raise ValueError("Tried to add state in collision")

# finally, add to the dict; if the key is of the wrong type then the error message from this should be sufficiently explanatory
self.configuration_dict[key] = state

def __getitem__(self, key):
"""
Get the configuration stored at key
"""
# just return the dict at the key. Errors from accessing the dict incorrectly (invalid key type, no entry) should be sufficiently explanatory
return self.configuration_dict[key]

def __contains__(self, key):
"""
See whether this object contains a configuration at key
"""
return key in self.configuration_dict

def __str__(self):
"""
Print the states in this object
"""
return str([k for k in self.configuration_dict])
Comment on lines +88 to +92
Copy link
Owner

Choose a reason for hiding this comment

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

How does __str__() differ from __repr__()? Genuine question, just that I've always done the latter when I want my class to be printable.

Copy link
Author

Choose a reason for hiding this comment

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

I also didn't know this, even __str__ was new to me 😄
But I looked it up and it seems that (according to this and a couple other places) that __repr__ is supposed to be the be-all end-all representation for a programmer or more advanced user, while the __str__ is supposed to be readable. So maybe I should have the __str__ method give just the joint states that are there by name and __repr__ should also include the actual states like you were suggesting?


def visualize_state(self, visualizer, name):
"""
Visualize the joint state stored in key `name`
"""
visualizer.display(self[name])
Comment on lines +75 to +98
Copy link
Owner

Choose a reason for hiding this comment

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

fill in these docstrings

Loading