-
Notifications
You must be signed in to change notification settings - Fork 14
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
Connector: IssacSim -> Ignition #16
Conversation
Signed-off-by: ahcorde <[email protected]>
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.
Some questions regarding the simulatorPoses
flag.
Should it only affect the poses or should we turn off ignition->omniverse completely? From what I understand on the code, it looks like a mix currently where if ignition is the controller, the entire omni->ign code path is disabled, but when omniverse is the controller, only updating of composes from ign->omni is disabled, adding, deleting of models, updating scalings, material etc is still enabled.
Operations other than poses in theory can cause loops as well, if we add a model in ign, it will add a subtree in usd, which causes a change event, which causes a ign request to add a model and so on. Same for operations like changing a model's scale, it's material properties etc.
// { | ||
// ignmsg << "Changed Info Path: " << Path.GetText() << std::endl; | ||
// } | ||
for (const pxr::SdfPath &path : ObjectsChanged.GetResyncedPaths()) |
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 ObjectsChanged
should be _objectsChanged
following ignition convention.
req.mutable_position()->set_z(transforms.position[2]); | ||
|
||
ignition::math::Quaterniond q( | ||
transforms.rotZYX[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.
Shouldn't it be rotXYZ
instead? I realized this is just naming and GetOp
indeed uses pxr::UsdGeomXformOp::TypeRotateXYZ
.
source/ignition_live/main.cpp
Outdated
app.add_option("--pose", simulatorPoses, "Which simulator will handle the poses") | ||
->required(); |
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.
Not sure on the CLI11 feature set, but if it supports it, we should use enums.
source/ignition_live/Scene.cpp
Outdated
ignition::math::Angle(quat.Pitch()).Degree(), | ||
ignition::math::Angle(quat.Yaw()).Degree()), | ||
pxr::UsdGeomXformCommonAPI::RotationOrderXYZ); | ||
if (this->ignitionControlPoses) |
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.
Would it be better to turn off subscription to the scene, joints and poses instead?
source/ignition_live/Scene.cpp
Outdated
@@ -885,6 +904,28 @@ bool Scene::Init() | |||
ignmsg << "Subscribed to topic: [" << topic << "]" << std::endl; | |||
} | |||
|
|||
if (!this->dataPtr->ignitionControlPoses) | |||
{ | |||
// TODO: disabled omniverse -> ignition sync to focus on ignition -> omniverse |
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.
Can this TODO be considered done with this pr?
Signed-off-by: ahcorde <[email protected]>
I'm getting service call timeout on the connector
There are also errors on ignition
I think the problem is that usd allows transformations on geometry but not sdf, so when trying to translate that to sdf we get a non existing target. I notice ignition physics is still running when isaacsim updates a pose, in theory this could lead to conflicting states. I can't test this now because I had to re-install omniverse to fix some other issues and downloads are down so I can't re-install isaacsim, and because of the above errors. |
Signed-off-by: Teo Koon Peng <[email protected]> Co-authored-by: Alejandro Hernández Cordero <[email protected]>
Signed-off-by: ahcorde <[email protected]>
@koonpeng I was testing your changes, and I'm facing this issue. Translations look wrong You need this change in ign-gazebo gazebosim/gz-sim#1394 |
Signed-off-by: ahcorde <[email protected]>
But we are facing another issue. Issac Sim dos not provide the joint angle, we can only see changes in the links Require: gazebosim/gz-sim#1394 I tried to implement something to calculate the joint angle but it's wrong af1daca |
Signed-off-by: ahcorde <[email protected]>
af1daca
to
24dc155
Compare
Wasn't able to try this yet. I'm assuming this is the simulation being ran? https://docs.omniverse.nvidia.com/app_isaacsim/app_isaacsim/tutorial_ros_moveit.html
|
Signed-off-by: ahcorde <[email protected]>
to test this you can run this script using ROS 1: You should add the following ROS plugins:
|
managed to run the sim in isaac but wasn't able to get the connector to work 2022-03-22.15-20-02.mp4ignition logs
ign-omni logs
|
Signed-off-by: ahcorde <[email protected]>
Signed-off-by: ahcorde <[email protected]>
source/ignition_live/Scene.cpp
Outdated
std::size_t found = visualName.find("_visual"); | ||
if (found != std::string::npos) | ||
{ | ||
if (visualName.substr(found).find("_visual") != |
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.
This condition will always be true since found != std::string::npos
is true, a substr of the found index will always contain the search pattern.
source/ignition_live/Scene.cpp
Outdated
std::size_t found = linkName.find("_link"); | ||
if (found != std::string::npos) | ||
{ | ||
if (linkName.substr(found).find("_link") != |
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.
This condition will always be true since found != std::string::npos
is true, a substr of the found index will always contain the search pattern.
@@ -578,6 +628,96 @@ bool Scene::Implementation::UpdateModel(const ignition::msgs::Model &_model) | |||
auto stage = this->stage->Lock(); | |||
|
|||
std::string modelName = _model.name(); | |||
|
|||
auto range = pxr::UsdPrimRange::Stage(*stage); | |||
for (auto const &prim : range) |
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.
Traversing the stage for each update might be bad for performance, maybe add some comments explaining a situation that will cause a model to be in multiple prims and possible improvements to avoid that.
qX = ignition::math::Quaterniond(angleX.Normalized().Radian(), 0, 0); | ||
qY = ignition::math::Quaterniond(0, angleY.Normalized().Radian(), 0); | ||
qZ = ignition::math::Quaterniond(0, 0, angleZ.Normalized().Radian()); | ||
q = ((q * qX) * qY) * qZ; |
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.
Since this is rotXYZ, we should be able to use the rpy constructor.
continue; | ||
auto stage = this->dataPtr->stage->Lock(); | ||
igndbg << "path " << objectsChanged.GetText() << std::endl; | ||
auto modelUSD = stage->GetPrimAtPath(objectsChanged.GetParentPath()); |
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.
Wasn't able to test this as I can't get the panda sim working. My only guess for joints orientation is that maybe the pose is being applied twice, first when calculating the joints position and then again here when calculating the pose?
Signed-off-by: ahcorde <[email protected]>
Signed-off-by: ahcorde <[email protected]>
Signed-off-by: ahcorde <[email protected]>
Signed-off-by: ahcorde <[email protected]>
@@ -35,7 +35,7 @@ class FUSDLayerNoticeListener::Implementation | |||
}; | |||
|
|||
FUSDLayerNoticeListener::FUSDLayerNoticeListener( | |||
std::shared_ptr<ThreadSafe<pxr::UsdStageRefPtr>> _stage, | |||
std::shared_ptr<ThreadSafe<pxr::UsdStageRefPtr>> &_stage, |
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 guess this fixed the double lock issue? Not sure why that happens and how this fixes it, I guess the mutex got copied for some reason?
Signed-off-by: ahcorde [email protected]
🎉 New feature
Summary
Communication between IssacSim -> Ignition.
Added a flag to check if the pose are set by Ignition or IssacSim
If you test this PR you might find some issue relative to this issue gazebosim/gz-sim#1358
Checklist
codecheck
passed (See contributing)Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining
Signed-off-by
messages.