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

Increase visual testing stability #344

Merged
merged 5 commits into from
Aug 9, 2018
Merged

Increase visual testing stability #344

merged 5 commits into from
Aug 9, 2018

Conversation

greimela-si
Copy link
Contributor

This PR stabilizes the visual tests for the ImageDisplay and the GridCellsDisplay.
Also discussed here: ros2/ci#211 (comment)

Martin-Idel-SI and others added 5 commits July 30, 2018 10:34
@Martin-Idel-SI @greimela-si
Fix GridCells display updating of colour to all grid cells
b907cae
@Martin-Idel-SI @greimela-si
Fix image_display visual test
d975693
@Martin-Idel-SI @greimela-si
Fix GridCellsDisplay
8123d3b 
- setPickColor only sets pick color for selection handlers
- introduce setColor to actually set the points' colour
@greimela-si
Fix uncrustify in PointCloud
930df37
@greimela-si
Use different sleeping mechanism in publisher to prevent blocking
a33cef4 
Use std::this_thread::sleep_for() instead of rclcpp::WallRate::sleep_for()
@esteve esteve added the in review Waiting for review (Kanban column) label Jul 30, 2018
@greimela-si
Copy link
Contributor Author

greimela-si commented Jul 30, 2018 •
edited
Loading

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status
  • Linux with visual tests enabled Build Status

@greimela-si
Copy link
Contributor Author

The results of the CI suggest that the ImageDisplay visual test has been fixed.

However, the Odometry display visual test seems to run into a timeout after passing, which ultimately marks it as failed.

@wjwwood
Copy link
Member

wjwwood commented Aug 2, 2018

Should I review/merge this as an incremental improvement? Or wait for you to make more changes?

Does this address at all the aarch64 failures on our nightly CI job? ros2/ci#211 (comment)

@Martin-Idel-SI
Copy link
Contributor

No, this addresses two issues with visual tests only:

  • The GridCells display doesn't work correctly: changing the colour only changes the colour for NEW cells. This was seen by the visual tests and is fixed for the display.
  • The ImageDisplay test was very flaky on slower machines - this was a problem with the test itself.

wjwwood
wjwwood approved these changes Aug 9, 2018
View reviewed changes
Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

lgtm

@wjwwood wjwwood merged commit 00f7efb into ros2:ros2 Aug 9, 2018
@wjwwood wjwwood removed the in review Waiting for review (Kanban column) label Aug 9, 2018
@Martin-Idel-SI Martin-Idel-SI deleted the bugfix/visual_testing_stability branch August 9, 2018 07:25
sloretz
sloretz reviewed Aug 17, 2018
View reviewed changes
rviz_default_plugins/src/rviz_default_plugins/displays/grid_cells/grid_cells_display.cpp
@@ -101,6 +101,12 @@ void GridCellsDisplay::updateAlpha()
context_->queueRender();
}

void GridCellsDisplay::updateColor()
{
cloud_->setColor(rviz_common::properties::qtToOgre(color_property_->getColor()));
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this change I'm getting a compile error. Is there another repo I need to pull?

/home/developer/workspaces/ros2-dev0/src/ros2/rviz/rviz_default_plugins/src/rviz_default_plugins/displays/grid_cells/grid_cells_display.cpp: In member function ‘void rviz_default_plugins::displays::GridCellsDisplay::updateColor()’:
/home/developer/workspaces/ros2-dev0/src/ros2/rviz/rviz_default_plugins/src/rviz_default_plugins/displays/grid_cells/grid_cells_display.cpp:106:11: error: ‘using element_type = class rviz_rendering::PointCloud {aka class rviz_rendering::PointCloud}’ has no member named ‘setColor’; did you mean ‘setPickColor’?
   cloud_->setColor(rviz_common::properties::qtToOgre(color_property_->getColor()));
           ^~~~~~~~
           setPickColor

Copy link
Member

Choose a reason for hiding this comment

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

What do you mean? It's passing on CI AFAIK.

In general, I wouldn't expect anything to work without all the repositories in the ros2.repos file being up-to-date.

Copy link
Contributor

Choose a reason for hiding this comment

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

The function was introduced in rviz_rendering::PointCloud in rviz_rendering with this PR. So if you recompile this repository, it should be fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

The issue persisted after deleting build/rviz* and install/rviz* and building with --packages-above rcl. However, it is gone after deleting the entire build and install folders and rebuilding everything.

I don't know what happened. It's almost like there is a missing build_depend, but I do see it in the package.xml

rviz/rviz_default_plugins/package.xml

Line 35 in 993d856

<depend>rviz_rendering</depend>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wjwwood wjwwood wjwwood approved these changes

@sloretz sloretz sloretz left review comments

@Martin-Idel-SI Martin-Idel-SI Martin-Idel-SI left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@greimela-si @wjwwood @Martin-Idel-SI @sloretz @esteve