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

Updated the URDFPrefabMaker to avoid referencing SDF link names #523

Merged

Conversation

lemonade-dm
Copy link
Contributor

Link names within an SDF are only unique with a tag, not unique across the entire SDF document.

The CreatePrefabTemplateFromUrdfOrSdf function has been modified to create a JointMapper object which stores a mapping of joints to all their parent and child links. It also creates a mapping of joints to the model they are directly attached to.

A similar LinkMapper structure has been added for SDF links, which stores a mapping of the link to their attached model.

Instead of CreatePrefabTemplateFromUrdfOrSdf creating a mapping of SDF link name to O3DE Entity, that mapping has now been updated toi map the SDF link pointer to O3DE Entity. This ensures that SDF links with same name are not lost when mapping to O3DE entities. This is needed as SDF links and joints are only required to be unique relative to their attached model. Two links on different models, can have the same link name (http://sdformat.org/tutorials?tut=composition_proposal#1-6-proposed-parsing-stages)

Updated the Robot Importer Visitor callback to require "ModelStack" parameters, which passes in the stack of sdf::Model objects that were visited on the way to the sdf element being visited be that a link, joint or nested model. Also updated the GetAllJoints and GetAllLinks function to store a fully qualified name for the joint/link including the ancestor model names when returning the name to joint and name to link map respectively.

@lemonade-dm lemonade-dm force-pushed the sdf-multi-model-support-fixes branch 2 times, most recently from 3eb291b to 84e765b Compare September 21, 2023 16:02
@michalpelka
Copy link
Contributor

michalpelka commented Sep 25, 2023

I've rebased it locally to test but I've one test failing:

143: [  FAILED  ] UrdfParserTest.TestPathResolve_ValidPathRelativeToAncestorPath_ResolvesCorrectly

Here is rebased branch : https://github.com/RobotecAI/o3de-extras/tree/mp/sdf-multi-model-support-fixes_rb
How to test PR? I've tested it against the URDF, but I think better would be to run it against multimodel SDFs. Is there any test files?
Edit:
The test TestPathResolve_ValidPathRelativeToAncestorPath_ResolvesCorrectly fails for development as well 🙈

@lemonade-dm
Copy link
Contributor Author

lemonade-dm commented Sep 25, 2023

I've rebased it locally to test but I've one test failing:

143: [  FAILED  ] UrdfParserTest.TestPathResolve_ValidPathRelativeToAncestorPath_ResolvesCorrectly

Here is rebased branch : https://github.com/RobotecAI/o3de-extras/tree/mp/sdf-multi-model-support-fixes_rb How to test PR? I've tested it against the URDF, but I think better would be to run it against multimodel SDFs. Is there any test files? Edit: The test TestPathResolve_ValidPathRelativeToAncestorPath_ResolvesCorrectly fails for development as well 🙈

That test failure isn't due to this change, but do to the combination that the UrdfParserTests.cpp is using an updated version of the AZ::IO::Path CompareRelative internal function to fix comparing paths with paths in them added in this commit https://github.com/o3de/o3de/blame/3d70b5c813119f14723409dd01b339a6be8afded/Code/Framework/AzCore/AzCore/IO/Path/PathParser.inl#L773-L776.
That commit is only available in the o3de development not stabilization/2310 currently, so the test only pass in when using the o3de repo development branch
The test passes in if using o3de development with the o3de-extras repo.

@lemonade-dm
Copy link
Contributor Author

I've rebased it locally to test but I've one test failing:

143: [  FAILED  ] UrdfParserTest.TestPathResolve_ValidPathRelativeToAncestorPath_ResolvesCorrectly

Here is rebased branch : https://github.com/RobotecAI/o3de-extras/tree/mp/sdf-multi-model-support-fixes_rb How to test PR? I've tested it against the URDF, but I think better would be to run it against multimodel SDFs. Is there any test files? Edit: The test TestPathResolve_ValidPathRelativeToAncestorPath_ResolvesCorrectly fails for development as well 🙈

That test failure isn't due to this change, but do combination that the UrdfParserTests.cpp is using an updated version of the AZ::IO::Path CompareRelative internal function to fix comparing paths with paths in them added in this commit https://github.com/o3de/o3de/blame/3d70b5c813119f14723409dd01b339a6be8afded/Code/Framework/AzCore/AzCore/IO/Path/PathParser.inl#L773-L776. That commit is only available in the o3de development not stabilization/2310 currently, so the test only pass in when using the o3de repo development branch The test passes in o3de development

Now there are three choices here that can resolve or mitigate the issue.

  1. As the failure is only in the UrdfParserTest.TestPathResolve_ValidPathRelativeToAncestorPath_ResolvesCorrectly Unit Test and not in any actual functionality due to that test comparing a path of
"/home/foo/ros_ws/install/foo_robot/meshes/bar.dae" == "/home/foo/ros_ws/install/foo_robot/./meshes/bar.dae"

The test failure can be ignored.

  1. The AZ IO Path changes can be cherry picked into stabilization/2310 and that would resolve the test failure
  2. The test code can be modified to use a call to LexicallyNormal() before the comparison, which would have the comparison succeed as in an older revision of the code

@lemonade-dm
Copy link
Contributor Author

Here is rebased branch : https://github.com/RobotecAI/o3de-extras/tree/mp/sdf-multi-model-support-fixes_rb How to test PR? I've tested it against the URDF, but I think better would be to run it against multimodel SDFs. Is there any test files?

For test files, the aws-robomaker-hospital-world repo hospital.world SDF file and the aws-robomaker-bookstore-world repo bookstore.world contains a tag with multiple models within them and those files are used for testing this change.

…ey are not globally unique

The `CreatePrefabTemplateFromUrdfOrSdf` function has been modified to create a JointMapper object which stores a mapping of joints to all their parent and child links.
It also creates a mapping of joints to the model they are directly attached to.

A similar LinkMapper structure has been added for SDF links, which stores a mapping of the link to their attached model.

Instead of `CreatePrefabTemplateFromUrdfOrSdf` creating a mapping of SDF link name to O3DE Entity, that mapping has now been updated toi map the SDF link pointer to O3DE Entity.
This ensures that SDF links with same name are not lost when mapping to O3DE entities.
This is needed as SDF links and joints are only required to be unique relative to their attached model.
Two links on different models, can have the same link name (http://sdformat.org/tutorials?tut=composition_proposal#1-6-proposed-parsing-stages)

Updated the Robot Importer Visitor callback to require "ModelStack" parameters, which passes in the stack of `sdf::Model` objects that were visited on the way to the sdf element being visited be that a link, joint or nested model.
Also updated the GetAllJoints and GetAllLinks function to store a fully qualified name for the joint/link including the ancestor model names when returning the name to joint and name to link map respectively.

Signed-off-by: lumberyard-employee-dm <56135373+lumberyard-employee-dm@users.noreply.github.com>
… grammar

Signed-off-by: lumberyard-employee-dm <56135373+lumberyard-employee-dm@users.noreply.github.com>
@lemonade-dm
Copy link
Contributor Author

I've rebased it locally to test but I've one test failing:

143: [  FAILED  ] UrdfParserTest.TestPathResolve_ValidPathRelativeToAncestorPath_ResolvesCorrectly

Here is rebased branch : https://github.com/RobotecAI/o3de-extras/tree/mp/sdf-multi-model-support-fixes_rb How to test PR? I've tested it against the URDF, but I think better would be to run it against multimodel SDFs. Is there any test files? Edit: The test TestPathResolve_ValidPathRelativeToAncestorPath_ResolvesCorrectly fails for development as well 🙈

That test failure isn't due to this change, but do combination that the UrdfParserTests.cpp is using an updated version of the AZ::IO::Path CompareRelative internal function to fix comparing paths with paths in them added in this commit https://github.com/o3de/o3de/blame/3d70b5c813119f14723409dd01b339a6be8afded/Code/Framework/AzCore/AzCore/IO/Path/PathParser.inl#L773-L776. That commit is only available in the o3de development not stabilization/2310 currently, so the test only pass in when using the o3de repo development branch The test passes in o3de development

Now there are three choices here that can resolve or mitigate the issue.

1. As the failure is only in the `UrdfParserTest.TestPathResolve_ValidPathRelativeToAncestorPath_ResolvesCorrectly` Unit Test and not in any actual functionality due to that test comparing a path of
"/home/foo/ros_ws/install/foo_robot/meshes/bar.dae" == "/home/foo/ros_ws/install/foo_robot/./meshes/bar.dae"

The test failure can be ignored.

2. The AZ IO Path [changes](https://github.com/o3de/o3de/commit/183d594453d712e4d9991f96e502bd0f3e847faa) can be cherry picked into stabilization/2310 and that would resolve the test failure

3. The test code can be modified to use a call to `LexicallyNormal()` before the comparison, which would have the comparison succeed as in an [older revision of the code](https://github.com/o3de/o3de-extras/pull/505/commits/3daf84ba43ac19ed0a37e68bdea47fe8f84cce3b#diff-51028f9e7742ab6c69b09457939ea407a0b1ff06edb5d451eaf5e8c0e057db36L982-L985)

The AZ::IO::Path changes have been cherry picked to the o3de repo stabilization/2310 branch in PR o3de/o3de#16798, so the Unit Test will now succeed when using the stabilization branch

Copy link
Contributor

@michalpelka michalpelka left a comment

Choose a reason for hiding this comment

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

My previous concerns were unrelated to this change. Approving.

@lemonade-dm lemonade-dm merged commit 6acba55 into o3de:development Sep 27, 2023
2 checks passed
michalpelka pushed a commit to RobotecAI/o3de-extras that referenced this pull request Oct 5, 2023
…#523)

Updated the URDFPrefabMaker to avoid referencing SDF link names as they are not globally unique

The `CreatePrefabTemplateFromUrdfOrSdf` function has been modified to create a JointMapper object which stores a mapping of joints to all their parent and child links.
It also creates a mapping of joints to the model they are directly attached to.

A similar LinkMapper structure has been added for SDF links, which stores a mapping of the link to their attached model.

Instead of `CreatePrefabTemplateFromUrdfOrSdf` creating a mapping of SDF link name to O3DE Entity, that mapping has now been updated toi map the SDF link pointer to O3DE Entity.
This ensures that SDF links with same name are not lost when mapping to O3DE entities.
This is needed as SDF links and joints are only required to be unique relative to their attached model.
Two links on different models, can have the same link name (http://sdformat.org/tutorials?tut=composition_proposal#1-6-proposed-parsing-stages)

Updated the Robot Importer Visitor callback to require "ModelStack" parameters, which passes in the stack of `sdf::Model` objects that were visited on the way to the sdf element being visited be that a link, joint or nested model.
Also updated the GetAllJoints and GetAllLinks function to store a fully qualified name for the joint/link including the ancestor model names when returning the name to joint and name to link map respectively.

Signed-off-by: lumberyard-employee-dm <56135373+lumberyard-employee-dm@users.noreply.github.com>

Addressed PR feedback for o3de/o3de-extra#523 around invalid text and grammar

Signed-off-by: lumberyard-employee-dm <56135373+lumberyard-employee-dm@users.noreply.github.com>
@lemonade-dm lemonade-dm deleted the sdf-multi-model-support-fixes branch October 16, 2023 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants