Migrate scalar displays by GW1708 · Pull Request #367 · ros2/rviz

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

  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

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

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

Conversation

GW1708 commented Dec 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GW1708 commented Dec 20, 2018

Uh oh!

wjwwood left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GW1708 commented Jan 7, 2019

Uh oh!

andreasholzner commented Jan 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andreasholzner commented Jan 7, 2019

Uh oh!

wjwwood commented Jan 7, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

GW1708 commented Dec 20, 2018 •

edited

Loading

andreasholzner commented Jan 7, 2019 •

edited

Loading