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.

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 marker display #229

Merged
merged 29 commits into from
Mar 17, 2018
Merged

Conversation

Martin-Idel-SI
Copy link
Contributor

Closes #100

This PR also adds a considerable amount of tests that are disabled for now.
It introduces convenience methods for scene graph introspection.

@Martin-Idel-SI
Copy link
Contributor Author

I'd say we wait for CI until the Pose-Display is sorted out (we'll need to rebase then anyway).

Martin-Idel-SI and others added 28 commits March 12, 2018 08:55
- Fix QueueSizeProperty in MarkerDisplay
- Fix MarkerArray subscription
Delete render_panel after visualization manager since the display cleanup in the visualization manager requires an existing render_panel
@Martin-Idel-SI
Copy link
Contributor Author

Martin-Idel-SI commented Mar 12, 2018

CI after rebase on master:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

It says it's going to not build but ignore visualization_msgs - but that's bad, because we need those. Obviously, later, it can't find them. I don't see why that happens.

@Martin-Idel-SI
Copy link
Contributor Author

I updated my repos file and everything, let's see what hapens:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@Martin-Idel-SI
Copy link
Contributor Author

I have no idea why the build ignores the common_interfaces repository. How can we fix this @wjwwood ?

@wjwwood
Copy link
Member

wjwwood commented Mar 15, 2018

@Martin-Idel-SI we disable several of the message packages so to save time in the builds, I'll have to remove that one:

https://github.com/ros2/ci/blob/d06052d66c407722e469b04184e05d32b20f24d7/ros2_batch_job/__main__.py#L77-L88

@wjwwood
Copy link
Member

wjwwood commented Mar 15, 2018

Until that is merged, you can use reenable_visualization_msgs in the CI_SCRIPTS_BRANCH field of the Jenkins jobs.

@Martin-Idel-SI
Copy link
Contributor Author

Thanks! So I tried that, let's see where we're at:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@@ -35,6 +35,7 @@
<test_depend>ament_cmake_gmock</test_depend>
<test_depend>ament_cmake_lint_cmake</test_depend>
<test_depend>ament_cmake_uncrustify</test_depend>
<test_depend>rviz_rendering_tests</test_depend>
Copy link
Member

Choose a reason for hiding this comment

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

You'll need a <test_depend> or <depend> (if you use it in the source and not just the tests) on visualization_msgs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Clearly, yes. Thanks for noting that oversight on my part!

@Martin-Idel-SI
Copy link
Contributor Author

Next try:
Building on master in workspace /var/lib/jenkins/jobs/ci_launcher/workspace

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@wjwwood
Copy link
Member

wjwwood commented Mar 16, 2018

Windows warning is a false positive, so CI looks good, let me do a code review real quick.

@wjwwood wjwwood added enhancement New feature or request in review Waiting for review (Kanban column) labels Mar 16, 2018
@wjwwood
Copy link
Member

wjwwood commented Mar 17, 2018

It would be good, in the future, to migrate some of the "acceptance tests" from rviz, like these:

#include "ros/ros.h"

#include "ros/ros.h"

#!/usr/bin/env python

@wjwwood wjwwood merged commit 2398b50 into ros2:ros2 Mar 17, 2018
@wjwwood wjwwood removed the in review Waiting for review (Kanban column) label Mar 17, 2018
@greimela-si greimela-si deleted the feature/migrate_marker_display branch March 19, 2018 08:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants