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

Combine InteractiveMarkerViewer classes. #521

Merged
merged 21 commits into from
Oct 9, 2019
Merged

Combine InteractiveMarkerViewer classes. #521

merged 21 commits into from
Oct 9, 2019

Conversation

aditya-vk
Copy link
Contributor

@aditya-vk aditya-vk commented Apr 22, 2019 •
edited by brianhou
Loading

Simplifies the code in InteractiveMarkerViewer. Refer to #520 for details (followed suggestion 2).

Resolves #519. Resolves #520.

Before creating a pull request

  • Document new methods and classes
  • Format code with make format

Before merging a pull request

  • Set version target by selecting a milestone on the right side
  • Summarize this change in CHANGELOG.md
  • Add unit test(s) for this change

@aditya-vk aditya-vk requested review from brianhou and Kelym April 22, 2019 22:02
@codecov
Copy link

codecov bot commented Apr 22, 2019 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@d374e55). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master     #521   +/-   ##
=========================================
  Coverage          ?   75.59%           
=========================================
  Files             ?      243           
  Lines             ?     5856           
  Branches          ?        0           
=========================================
  Hits              ?     4427           
  Misses            ?     1429           
  Partials          ?        0

@brianhou brianhou changed the title Combine InteractiveMarkerViewer scripts. Combine InteractiveMarkerViewer classes. Apr 22, 2019
Kelym
Kelym requested changes Apr 29, 2019
View reviewed changes
Copy link
Member

@Kelym Kelym left a comment

Choose a reason for hiding this comment

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

Need to clarify whether mWorld is necessary for a viewer to have SkeletonMarkers, embed the answer in doc and set guard in code accordingly. Plus a few nit.

@aditya-vk
Copy link
Contributor Author

@Kelym @brianhou Addressed the comments. Added a docstring that mWorld can be a nullptr if we are just adding markers, and otherwise the skeletons in the world shall also get updated. Could you review?

@aditya-vk aditya-vk requested a review from Kelym September 12, 2019 03:30
Kelym
Kelym requested changes Sep 23, 2019
View reviewed changes
Copy link
Member

@Kelym Kelym left a comment

Choose a reason for hiding this comment

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

LGTM, except one bug in update(). See suggestion.

@aditya-vk @Kelym
Update src/rviz/InteractiveMarkerViewer.cpp
3ac1cad 
Co-Authored-By: Liyiming Ke <dustocyno@gmail.com>
@aditya-vk aditya-vk mentioned this pull request Oct 2, 2019
Open
@aditya-vk aditya-vk requested a review from gilwoolee as a code owner October 8, 2019 01:13
@aditya-vk
Copy link
Contributor Author

@Kelym I updated the formatting and the ChangeLog. Should be ready for merging into master?

brianhou
brianhou reviewed Oct 8, 2019
View reviewed changes
aditya-vk and others added 2 commits October 8, 2019 14:26
@aditya-vk @brianhou
Update CHANGELOG.md
16bff7c 
Co-Authored-By: Brian Hou <brianhou@users.noreply.github.com>
@brianhou
Merge branch 'master' into rviz-cleanup
53dd754
Kelym
Kelym approved these changes Oct 9, 2019
View reviewed changes
Copy link
Member

@Kelym Kelym left a comment

Choose a reason for hiding this comment

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

LGTM.

@brianhou brianhou merged commit 96ef929 into master Oct 9, 2019
@brianhou brianhou deleted the rviz-cleanup branch October 9, 2019 00:36
This was referenced Apr 2, 2021
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brianhou brianhou brianhou left review comments

@Kelym Kelym Kelym approved these changes

@gilwoolee gilwoolee Awaiting requested review from gilwoolee

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

Successfully merging this pull request may close these issues.

(World)InteractiveMarkerViewer Clean up rviz namespace
3 participants
@aditya-vk @brianhou @Kelym