-
Notifications
You must be signed in to change notification settings - Fork 212
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 scalar displays #367
Conversation
CI was done by @Martin-Idel-SI |
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, just with a few style nitpick's. I'll run CI and build/test locally asap.
rviz_default_plugins/CMakeLists.txt
Outdated
@@ -190,7 +199,7 @@ set(rviz_default_plugins_source_files | |||
src/rviz_default_plugins/view_controllers/orbit/orbit_view_controller.cpp | |||
src/rviz_default_plugins/view_controllers/ortho/fixed_orientation_ortho_view_controller.cpp | |||
src/rviz_default_plugins/view_controllers/xy_orbit/xy_orbit_view_controller.cpp | |||
) | |||
test/rviz_default_plugins/publishers/fluid_pressure_publisher.hpp) |
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.
test/rviz_default_plugins/publishers/fluid_pressure_publisher.hpp) | |
test/rviz_default_plugins/publishers/fluid_pressure_publisher.hpp | |
) |
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.
Why is this file added to the sources list? Shouldn't it instead be part of the test executable?
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.
My bad, it should be part of the test executable of course. Fixed!
rviz_default_plugins/CMakeLists.txt
Outdated
ament_add_gmock(point_cloud_scalar_display_test | ||
test/rviz_default_plugins/displays/pointcloud/point_cloud_scalar_display_test.cpp | ||
test/rviz_default_plugins/displays/display_test_fixture.cpp | ||
${SKIP_DISPLAY_TESTS}) |
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.
Please use 2 space indentations.
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.
Here and below.
* \class FluidPressureDisplay | ||
* \brief Displays an FluidPressure message of type sensor_msgs::FluidPressure | ||
* | ||
*/ |
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 should be like this:
/// Display a FluidPressure message of type sensor_msgs::FluidPressure.
/**
* \class FluidPressureDisplay
*/
Note: that the *
's align vertically, Display
and not Displays
, and a
not an
.
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.
These doc style changes should be applied to the rest of the doc blocks in the rest of this file.
/** | ||
* \class FluidPressureDisplay | ||
* \brief Displays an FluidPressure message of type sensor_msgs::FluidPressure | ||
* |
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.
* |
void updateQueueSize(); | ||
/** | ||
* \class FluidPressureDisplay | ||
* \brief Displays an FluidPressure message of type sensor_msgs::FluidPressure |
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.
Move the brief above the /**
block comment and use ///
.
@@ -121,6 +143,16 @@ | |||
<message_type>sensor_msgs/Range</message_type> | |||
</class> | |||
|
|||
<class | |||
name="rviz_default_plugins/RelativeHumidity" |
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.
2 space indentation here too.
Done! Fixed the remaining style errors and added a missing keyword to the visual testing framework README |
…luid pressure) header and source file from rviz to rviz2
changed pointers to smart pointers replaced outdated headers updated CMakeList and plgins_description
They test if color changes correctly. Refactored point cloud common page object by implementing the queuesize method. Therefore laser scan, point cloud and point cloud 2 page objects become redundant
Added the header file point cloud scalar that includes all functionality that was possible to template. This excludes the process message function and the set initial values und update properties function. Furthermore the process message function was split up into several smaller functions where the createpointcloud2message function allows for unit tests because no instance of the actual point cloud is needed.
Test for correct memory allocation and correct copy of values. Also uncrustified files with linter errors
updated ported displays. Changed ament for colcon
Indentation, doc blocks
without it the tests do not execute
without it the tests do not run
3c395bf
to
54a2453
Compare
@wjwwood The unstable builds of the previous round of CI were all uncrustify errors that are already fixed by #366. So I rebased and started new CI build just to be sure (we no longer have our own CI infrastructure at TNG available). The Linux failure above was repeatedly caused by a docker connection error and is not related to the code problems. |
* Revert "Migrate scalar displays (#367)" This reverts commit 9f3f3a6. * Revert "Handle FindEigen3 module's differing definitions (#370)" This reverts commit 2077b3a. * Revert "Skip the system directories when looking for OGRE (#371)" This reverts commit 61de77f. * Revert "Revert "Visibility followup for marker" (#369)" This reverts commit 712f903.
Closes #90 #89 #78 #77
This PR migrates temperature, illuminance, relative humidity and fluid pressure displays.
Since they work extremely similar they were refactored by introducing a point cloud scalar parent class
Refactored display page objects by moving set queue size to point cloud common page object and deleting unnecessary page objects.
CI:
Including Visual Tests: