-
Notifications
You must be signed in to change notification settings - Fork 33
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
Simulate simple 2D robot dynamics #151
Conversation
0858bd2
to
6216c0d
Compare
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 like how the dynamics is implemented and only have a few small comments.
@@ -23,6 +24,10 @@ def __init__( | |||
radius=0.0, | |||
height=0.0, | |||
color=(0.8, 0.0, 0.8), | |||
max_linear_velocity=np.inf, |
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 am in favour of always getting a limit from user instead of an infinity default limit.
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 the default should actually be unlimited, because there will probably be a core set of users who just want to command a velocity based on something else. So I might leave this as is.
:return: True if the robot is moving, False otherwise. | ||
:rtype: bool | ||
""" | ||
return self.executing_nav or np.count_nonzero(self.dynamics.velocity) > 0 |
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.
A get_current_velocity()
method in the RobotDynamics2D
could be useful here, instead of directly accessing the velocity attribute.
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 the RobotDynamics2D
instance can be considered a part of the Robot
class... so I'm OK leaving this to just directly access velocity here, as is done in Robot.get_pose()
which directly accesses robot.dynamics.pose
.
Thanks for the review @ibrahiminfinite! I made a few of the changes suggested, and will most likely leave the rest as is, if that's okay. Will work on the ROS wrapper now... |
Yeah, the rest can be ignored. |
This PR adds simple 2D robot dynamics to core (non-ROS) pyrobosim.
Partially closes #148