-
Notifications
You must be signed in to change notification settings - Fork 205
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
Migrate PathDisplay #236
Migrate PathDisplay #236
Conversation
- ordering of scene nodes is not deterministic => use container matchers
- matchers for ASSERT_THAT can easily be reused - ASSERT_THAT style assertions are better to read
- EqVector3 -> Vector3Eq and EqQuaternion -> QuaternionEq
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.
lgtm, with some minor comments
rviz_default_plugins/CMakeLists.txt
Outdated
@@ -118,6 +121,7 @@ target_link_libraries(rviz_default_plugins | |||
) | |||
ament_target_dependencies(rviz_default_plugins | |||
# geometry_msgs |
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 one should probably also be uncommented?
rviz_default_plugins/package.xml
Outdated
@@ -29,6 +29,8 @@ | |||
<depend>rviz_common</depend> | |||
<depend>rviz_rendering</depend> | |||
<depend>urdf</depend> | |||
<depend>geometry_msgs</depend> | |||
<depend>nav_msgs</depend> |
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.
nitpick: please alphabetically order dependencies :)
* POSSIBILITY OF SUCH DAMAGE. | ||
*/ | ||
|
||
|
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.
nitpick: minimize vertical whitespace
* POSSIBILITY OF SUCH DAMAGE. | ||
*/ | ||
|
||
|
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.
nitpick: mvw
Resolves #81.