-
Notifications
You must be signed in to change notification settings - Fork 42
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
Allow specifying relative poses #317
Conversation
c114a0a
to
c52f594
Compare
@kumar-sanjeeev Since I've been giving you a hard time with reviews, I'd like to give you a chance to do the same for this one, if you'd like. There are still lots more cases to handle, but initially I'm curious what you think of this way to express relative poses, both in the programmatic and YAML interfaces. The other open question is what the "reference pose" is defined as.
Perhaps the |
@sea-bass thank you for giving me this chance :). I will get back to you. |
I like the thought process behind relative poses. I have one question that comes to mind, after obtaining the absolute pose of an entity using |
Yes it should, but this already happens when you try to add the entity with the transformed pose -- not when you do the transformation. |
Okay, understood. At the moment, I don't see any significant challenges in implementing the suggested logic for relative pose. |
@kumar-sanjeeev this is ready for review! |
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.
Noice!
LGTM. Just a few basic comments for improving readability:
I was wondering if it would make sense to mention at the top of the World schema
:
"Refer to the Specifying Poses
section below to better understand how poses can be specified in PyRoboSim."
This addition could help users quickly locate the relevant section on poses when they are reading the YAML file.
def construct_pose_from_args(pose_args, world): | ||
""" | ||
Constructs a PyRoboSim pose from a dictionary of YAML arguments. | ||
|
||
This handles poses of different formats, as well as relative pose specification. | ||
|
||
:param pose_args: A dictionary containing the pose arguments. | ||
:type pose_args: dict | ||
:param world: The world to use for looking up entities. | ||
:type world: :class:`pyrobosim.core.world.World` | ||
:return: Pose object | ||
:rype: :class:`pyrobosim.utils.pose.Pose` | ||
""" | ||
pose = Pose.construct(pose_args) | ||
if "relative_to" in pose_args: | ||
pose = world.get_pose_relative_to(pose, pose_args["relative_to"]) | ||
return pose | ||
|
||
|
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.
does it make sense to put this function in pose.py
?
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 wanted to do this, but it felt strange to have to pass in a world in a function in pose.py
. If we were using Python type hints, then this would actually cause a circular import of World
.
Perhaps a better candidate might be pyrobosim/utils/knowledge.py
? All the other functions in this file already accept worlds, so this file you definitely need to have World imported first.
It's the same reason World.get_pose_relative_to()
wasn't just a free function in pose.py
that accepted a World
object as input. This one is even worse because it has many other imports in the body of this function (including Room
, Location
, etc.) and would cause several circular imports even without type hints.
I think at some point the world.py
file (currently 1740 lines of code) will need to be split up into more utilities, and this would be one of them when that time comes.
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 understand. Let’s keep it as is for now. I remember when I was working on the type hints issue
, I encountered a lot of circular import errors.
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.
Perfect !
I also tested the changes locally. The examples seem to be working fine, and the docs and test cases are running fine as well.
This PR implements a mechanism to express poses relative to other entities.
Closes #298