Migrate scalar displays #367

GW1708 · 2018-12-20T13:35:26Z

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:

Linux
Linux-aarch64
macOS
Windows

Including Visual Tests:

Linux

GW1708 · 2018-12-20T15:53:39Z

CI was done by @Martin-Idel-SI
Visual tests on Linux seem to fail but are fine locally and on our Jenkins

wjwwood

lgtm, just with a few style nitpick's. I'll run CI and build/test locally asap.

wjwwood · 2018-12-21T18:46:02Z

rviz_default_plugins/CMakeLists.txt

@@ -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)


Suggested change

test/rviz_default_plugins/publishers/fluid_pressure_publisher.hpp)

test/rviz_default_plugins/publishers/fluid_pressure_publisher.hpp

)

Why is this file added to the sources list? Shouldn't it instead be part of the test executable?

My bad, it should be part of the test executable of course. Fixed!

wjwwood · 2018-12-21T18:47:49Z

rviz_default_plugins/CMakeLists.txt

+  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})


Please use 2 space indentations.

Here and below.

wjwwood · 2018-12-21T18:49:44Z

...ault_plugins/include/rviz_default_plugins/displays/fluid_pressure/fluid_pressure_display.hpp

+* \class FluidPressureDisplay
+* \brief Displays an FluidPressure message of type sensor_msgs::FluidPressure
+*
+*/


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.

These doc style changes should be applied to the rest of the doc blocks in the rest of this file.

wjwwood · 2018-12-21T18:49:52Z

...ault_plugins/include/rviz_default_plugins/displays/fluid_pressure/fluid_pressure_display.hpp

+/**
+* \class FluidPressureDisplay
+* \brief Displays an FluidPressure message of type sensor_msgs::FluidPressure
+*


Suggested change

*

wjwwood · 2018-12-21T18:50:19Z

...ault_plugins/include/rviz_default_plugins/displays/fluid_pressure/fluid_pressure_display.hpp

-  void updateQueueSize();
+/**
+* \class FluidPressureDisplay
+* \brief Displays an FluidPressure message of type sensor_msgs::FluidPressure


Move the brief above the /** block comment and use ///.

wjwwood · 2018-12-21T18:59:04Z

rviz_default_plugins/plugins_description.xml

@@ -121,6 +143,16 @@
    <message_type>sensor_msgs/Range</message_type>
  </class>

+  <class
+          name="rviz_default_plugins/RelativeHumidity"


2 space indentation here too.

GW1708 · 2019-01-07T11:12:18Z

Done! Fixed the remaining style errors and added a missing keyword to the visual testing framework README

andreasholzner · 2019-01-07T13:34:18Z

Just to be sure a new round of CI:

Linux
Linux-aarch64
macOS
Windows

Visual tests:

…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

andreasholzner · 2019-01-07T15:17:08Z

@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.

Linux
Linux-aarch64
macOS
Windows

wjwwood · 2019-01-07T20:46:34Z

Trying Linux again:

This reverts commit 9f3f3a6.

* 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.

tfoote added the in review Waiting for review (Kanban column) label Dec 20, 2018

wjwwood approved these changes Dec 21, 2018

View reviewed changes

Maximilian Kuehn and others added 9 commits January 7, 2019 16:04

moved scalar displays (temperature, illuminance, relative humidity, f…

47cae95

…luid pressure) header and source file from rviz to rviz2

Changed header and source files to compile successfully

34f71f5

changed pointers to smart pointers replaced outdated headers updated CMakeList and plgins_description

Implemented Visual Tests

01c2a36

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

Implemented point cloud scalar display test

938faf4

Test for correct memory allocation and correct copy of values. Also uncrustified files with linter errors

Update Docs

c289782

updated ported displays. Changed ament for colcon

Pull Request Changes

66901dc

Indentation, doc blocks

Added DBUILD_TESTING keyword

4132529

without it the tests do not execute

added DBUILD_TESTING keyword

54a2453

without it the tests do not run

andreasholzner force-pushed the migrate_scalar_displays branch from 3c395bf to 54a2453 Compare January 7, 2019 15:04

wjwwood approved these changes Jan 7, 2019

View reviewed changes

wjwwood merged commit 9f3f3a6 into ros2:ros2 Jan 7, 2019

wjwwood removed the in review Waiting for review (Kanban column) label Jan 7, 2019

clalancette mentioned this pull request Jan 8, 2019

ROS 2 Crystal Clemmys Patch Release 1 ros2/ros2#638

Closed

This was referenced Jan 8, 2019

Migrate RelativeHumidityDisplay #89

Closed

Migrate IlluminanceDisplay #78

Closed

Migrate FluidPressureDisplay #77

Closed

wjwwood added a commit that referenced this pull request Jan 14, 2019

Revert "Migrate scalar displays (#367)"

3282f17

This reverts commit 9f3f3a6.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate scalar displays #367

Migrate scalar displays #367

GW1708 commented Dec 20, 2018 •

edited

Loading

GW1708 commented Dec 20, 2018

wjwwood left a comment

wjwwood Dec 21, 2018

wjwwood Dec 21, 2018

GW1708 Jan 7, 2019

wjwwood Dec 21, 2018

wjwwood Dec 21, 2018

wjwwood Dec 21, 2018

wjwwood Dec 21, 2018

wjwwood Dec 21, 2018

wjwwood Dec 21, 2018

wjwwood Dec 21, 2018

GW1708 commented Jan 7, 2019

andreasholzner commented Jan 7, 2019 •

edited

Loading

andreasholzner commented Jan 7, 2019

wjwwood commented Jan 7, 2019

	test/rviz_default_plugins/publishers/fluid_pressure_publisher.hpp)
	test/rviz_default_plugins/publishers/fluid_pressure_publisher.hpp
	)

Migrate scalar displays #367

Migrate scalar displays #367

Conversation

GW1708 commented Dec 20, 2018 • edited Loading

GW1708 commented Dec 20, 2018

wjwwood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GW1708 commented Jan 7, 2019

andreasholzner commented Jan 7, 2019 • edited Loading

andreasholzner commented Jan 7, 2019

wjwwood commented Jan 7, 2019

GW1708 commented Dec 20, 2018 •

edited

Loading

andreasholzner commented Jan 7, 2019 •

edited

Loading