Named joint states helper #71
Conversation
| self.collision_data = self.collision_model.createData() | ||
| self.options = options | ||
| self.configuration_dict = {} | ||
|
|
There was a problem hiding this comment.
I should also override __delitem__, I think.
There was a problem hiding this comment.
Definitely, removing named configurations is well within reason
|
|
||
| # Initialize | ||
| named_joints_configuration = NamedJointConfigurations(model, collision_model) | ||
| named_joints_configuration["home"] = q |
There was a problem hiding this comment.
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.
| 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" |
There was a problem hiding this comment.
highly recommend splitting up this into multiple lines -- it was a bit hard to read.
| 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) | ||
| if user_input in named_joints_configuration: | ||
| named_joints_configuration.visualize_state(visualizer, user_input) | ||
| else: | ||
| named_joints_configuration[user_input] = q |
There was a problem hiding this comment.
You probably also want to add confirmation that a new named state was added / the robot is going to an existing state.
| print("Stored states:") | ||
| print(named_joints_configuration) |
There was a problem hiding this comment.
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?
| 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) |
There was a problem hiding this comment.
nit: put parentheses around these conditions
| @@ -0,0 +1,98 @@ | |||
| from pyroboplan.core.utils import ( | |||
There was a problem hiding this comment.
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?
| if not check_valid_pose(self.model, state): | ||
| if not check_within_limits(self.model, state): |
There was a problem hiding this comment.
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?
| if not self.options.allow_collisions: | ||
| if check_collisions_at_state(self.model, self.collision_model, state): |
There was a problem hiding this comment.
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
| 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]) | ||
|
|
||
| def visualize_state(self, visualizer, name): | ||
| """ | ||
| Visualize the joint state stored in key `name` | ||
| """ | ||
| visualizer.display(self[name]) |
| def __str__(self): | ||
| """ | ||
| Print the states in this object | ||
| """ | ||
| return str([k for k in self.configuration_dict]) |
There was a problem hiding this comment.
How does __str__() differ from __repr__()? Genuine question, just that I've always done the latter when I want my class to be printable.
There was a problem hiding this comment.
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?
Resolves #65
Before merging this, I want to tackle #56 (since, as mentioned, named states should be able to pertain to a subset of joints)
Just making this PR now to get it out there and see if there is feedback about the preliminary state of this.
I will also be adding some unit tests before marking this PR ready for review.