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 pose with covariance display #471

Merged

Conversation

Martin-Idel
Copy link
Contributor

Closes #87

Icon is missing, this was already proposed as a PR here: #430
As always: Added a visual test - I adapted test and page objects from the odometry display ones and therefore kept the copyrights from those files, too, and left a comment that those are basically copy&paste.

There is a lot of common code between this display, the pose display and the pose array display. It could be refactored, but that would make this PR really large and difficult to review, so I would suggest to do this separately. Here, I only brought the display as close to the pose display code as possible.

- extract selection handler (shorten name due
  to include guards)
- make code closer to pose display code
- common code (also with odometry display) could
  be extracted
@claireyywang claireyywang added the enhancement New feature or request label Nov 21, 2019
@wjwwood
Copy link
Member

wjwwood commented Nov 21, 2019

CI:

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

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
Copy link
Member

wjwwood commented Nov 22, 2019

Looks like there's a build failure on the Windows CI, maybe symbol visibility related?

@Martin-Idel
Copy link
Contributor Author

@wjwwood Yes, definitely. Hopefully the additions already fix everything, I don't have a Windows to test right now.

Maybe we should add -fvisibility=hidden to the gcc and clang builds to make it more similar to the Windows one...

@wjwwood
Copy link
Member

wjwwood commented Nov 22, 2019

Thanks, CI:

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

@wjwwood wjwwood merged commit 045227a into ros2:ros2 Nov 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request requires-changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate PoseWithCovarianceDisplay
3 participants