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.

Sign up for GitHub

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

Jump to bottom

Initial urdf parser plugin that parses sdformat #1

Merged
merged 152 commits into from
Nov 2, 2020
Merged

Initial urdf parser plugin that parses sdformat #1

merged 152 commits into from
Nov 2, 2020

Conversation

sloretz
Copy link
Contributor

@sloretz sloretz commented Jul 15, 2020 •
edited
Loading

This is ready for review.

@scpeters @IanTheEngineer FYI

requires ros2/urdf#13 (This is merged 🎉)
requires ros/resource_retriever#50 (no longer uses this)

@sloretz
Initial urdf parser plugin that parses sdformat
b8a6ff5 
Lots of TODOs

Signed-off-by: Shane Loretz<[email protected]>
Signed-off-by: Shane Loretz <[email protected]>
@sloretz sloretz self-assigned this Jul 15, 2020
sloretz added 4 commits July 17, 2020 08:52
@sloretz
Visual and Collision with Box geometry
035dbc8 
Signed-off-by: Shane Loretz <[email protected]>
@sloretz
Sphere geometry
c80edcc 
Signed-off-by: Shane Loretz <[email protected]>
@sloretz
Cylinder geometry
30de800 
Signed-off-by: Shane Loretz <[email protected]>
@sloretz
Mesh support
9f9ac7e 
Signed-off-by: Shane Loretz <[email protected]>
@sloretz sloretz mentioned this pull request Jul 18, 2020
Closed
@ros-discourse
Copy link

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-2020-07-16/15468/1

@scpeters scpeters mentioned this pull request Jul 23, 2020
Open
scpeters
scpeters reviewed Jul 24, 2020
View reviewed changes
src/sdformat_urdf.cpp Outdated Show resolved Hide resolved
@sloretz sloretz mentioned this pull request Jul 27, 2020
Closed
EricCousineau-TRI
EricCousineau-TRI reviewed Jul 30, 2020
View reviewed changes
package.xml Outdated Show resolved Hide resolved
package.xml Outdated Show resolved Hide resolved
src/sdformat_urdf.cpp Outdated Show resolved Hide resolved
include/sdformat_urdf/sdformat_urdf.hpp Outdated Show resolved Hide resolved
src/sdformat_urdf.cpp Outdated Show resolved Hide resolved
clalancette
clalancette reviewed Aug 10, 2020
View reviewed changes
Copy link

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

A few mostly minor things in here.

package.xml Outdated Show resolved Hide resolved
src/sdformat_urdf.cpp Outdated Show resolved Hide resolved
src/sdformat_urdf.cpp Outdated Show resolved Hide resolved
src/sdformat_urdf_plugin.cpp Outdated Show resolved Hide resolved
src/sdformat_urdf_plugin.cpp Outdated Show resolved Hide resolved
@sloretz sloretz mentioned this pull request Aug 19, 2020
Merged
@sloretz
Include ignition/math/Pose3.hh
ab6637b 
Signed-off-by: Shane Loretz <[email protected]>
@sloretz
Use final on plugin class
21adf18 
Signed-off-by: Shane Loretz <[email protected]>
@sloretz
Use TinyXML2 to filter non-sdf XML documents
7a5ad72 
Signed-off-by: Shane Loretz <[email protected]>
@sloretz
Apache 2.0 License
512dfc2 
Signed-off-by: Shane Loretz <[email protected]>
@sloretz
Satisfy uncrustify
63fd805 
Signed-off-by: Shane Loretz <[email protected]>
@sloretz
Satisfy cpplint
16c133f 
Signed-off-by: Shane Loretz <[email protected]>
sloretz added 11 commits October 22, 2020 10:56
@sloretz
the the -> the
6552021 
Signed-off-by: Shane Loretz <[email protected]>
@sloretz
Compare WorldCount against 0u
4681be2 
Signed-off-by: Shane Loretz <[email protected]>
@sloretz
Concatenate link and visual names to make material name
197bbf8 
Signed-off-by: Shane Loretz <[email protected]>
@sloretz
Compatibility with CMake earlier than 3.14
a533c05 
Signed-off-by: Shane Loretz <[email protected]>
@sloretz
Use target_link_libraries()
f25ffe2 
Signed-off-by: Shane Loretz <[email protected]>
@sloretz
One install rule per target
86f5f54 
Signed-off-by: Shane Loretz <[email protected]>
@sloretz
voilated -> violated
2c375de 
Signed-off-by: Shane Loretz <[email protected]>
@sloretz
Use axis2 for second axis
48d9136 
Signed-off-by: Shane Loretz <[email protected]>
@sloretz
Universal joint perpendicular axis
7d0ac7a 
Signed-off-by: Shane Loretz <[email protected]>
@sloretz
Use Pose3d::operator*
b06afde 
Signed-off-by: Shane Loretz <[email protected]>
@sloretz
structures -> DOM objects
aff9def 
Signed-off-by: Shane Loretz <[email protected]>
@sloretz
Copy link
Contributor Author

sloretz commented Oct 23, 2020

@azeey I think I addressed all changes requested. Mind having a second look? 🙇

azeey
azeey reviewed Oct 27, 2020
View reviewed changes
Copy link
Contributor

@azeey azeey left a comment

Choose a reason for hiding this comment

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

I have now gone through all the tests and everything looks great! I think it would be good to add a test where the inertial, visual and collision are affected by the link's pose. Correct me if I'm wrong, but the tests I have seen had all these elements in the root link so the pose of the link had no impact on the poses of these elements, or the SDFormat link had an identity pose.

sdformat_urdf/test/include/test_tools.hpp Show resolved Hide resolved
sdformat_urdf/test/joint_tests.cpp Outdated Show resolved Hide resolved
sdformat_urdf/test/geometry_tests.cpp Show resolved Hide resolved
sdformat_urdf/test/geometry_tests.cpp Show resolved Hide resolved
sdformat_urdf/test/joint_tests.cpp Show resolved Hide resolved
sdformat_urdf/test/joint_tests.cpp Show resolved Hide resolved
sdformat_urdf/test/link_tests.cpp Show resolved Hide resolved
@sloretz
Document EXPECT_NAMES()
e924fe6 
Signed-off-by: Shane Loretz <[email protected]>
@gavanderhoorn

This comment has been minimized.

Sign in to view
@sloretz
Compare normalized joint axes
84d23f5 
Signed-off-by: Shane Loretz <[email protected]>
@sloretz
Check casted pointer before using them
bb65e1f 
Signed-off-by: Shane Loretz <[email protected]>
@sloretz
Check geometry pointer isn't null before using it
9ae4804 
Signed-off-by: Shane Loretz <[email protected]>
@sloretz
Assert joint isn't null before using it
d9ca9fb 
Signed-off-by: Shane Loretz <[email protected]>
@sloretz
Assert link isn't null before using it
ec3f81b 
Signed-off-by: Shane Loretz <[email protected]>
@sloretz
Assert link isn't null before using it
645a2e9 
Signed-off-by: Shane Loretz <[email protected]>
@sloretz
Color = 0.4 * ambient + 0.8 * diffuse
c84405d 
Signed-off-by: Shane Loretz <[email protected]>
@sloretz
Fix application of link pose on inertial/visual/collision
6582d52 
Signed-off-by: Shane Loretz <[email protected]>
@sloretz
Add pose_joint_all test
e4664a8 
Signed-off-by: Shane Loretz <[email protected]>
@sloretz
Copy link
Contributor Author

sloretz commented Oct 29, 2020

I think it would be good to add a test where the inertial, visual and collision are affected by the link's pose. Correct me if I'm wrong, but the tests I have seen had all these elements in the root link so the pose of the link had no impact on the poses of these elements, or the SDFormat link had an identity pose.

As in it's missing a test of a link with joint to a link having non-zero pose and non-zero inertial/visual/collision pose? Added pose_joint_all model and test in e4664a8, and fix a bug it discovered in how those poses get calculated in 6582d52

@sloretz
Copy link
Contributor Author

sloretz commented Oct 29, 2020

@azeey I think I've addressed all PR comments. Mind having another look?

azeey
azeey approved these changes Oct 30, 2020
View reviewed changes
Copy link
Contributor

@azeey azeey left a comment

Choose a reason for hiding this comment

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

This looks great! Thanks for iterating with me.

@sloretz sloretz merged commit efc3913 into ros2 Nov 2, 2020
@sloretz sloretz deleted the ros2-initial branch November 2, 2020 17:12
@azeey azeey mentioned this pull request Apr 19, 2021
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@EricCousineau-TRI EricCousineau-TRI EricCousineau-TRI left review comments

@scpeters scpeters scpeters left review comments

@clalancette clalancette clalancette left review comments

@azeey azeey azeey approved these changes

Assignees

@sloretz sloretz

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@sloretz @ros-discourse @azeey @scpeters @gavanderhoorn @clalancette @EricCousineau-TRI