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

rqt_bag: publish clock time from bag #204

Merged
merged 1 commit into from
Jan 21, 2014
Merged

rqt_bag: publish clock time from bag #204

merged 1 commit into from
Jan 21, 2014

Conversation

MatthiasNieuwenhuisen
Copy link
Contributor

These changes address Issue #102: rqt_bag: missing clock publisher.

@@ -85,16 +109,18 @@ def message_viewed(self, bag, msg_data):
if self.timeline.play_speed <= 0.0:
return

topic, msg, _ = msg_data
topic, msg, t = msg_data
Copy link
Member

Choose a reason for hiding this comment

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

please avoid single letter variable names, calling it "time" or "clock" would be better than "t"

@DorianScholz
Copy link
Member

@dirk-thomas @130s @ablasdel
Any reason why this has not been merged?
It looks good to me, but I'm neither using nor maintaining rqt_bag...

@@ -494,6 +495,8 @@ def _create_player(self):
if not self._player:
try:
self._player = Player(self)
if(self._publish_clock):
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove the parenthesis around the condition.

@MatthiasNieuwenhuisen
Copy link
Contributor Author

@dirk-thomas Thanks for the comments, I hope I have addressed them adequately. I'm not sure if I interpreted your last comment on the exception handling correct, if not let me know.

@@ -22,6 +22,7 @@
<run_depend>rospy</run_depend>
<run_depend version_gte="0.2.12">rqt_gui</run_depend>
<run_depend>rqt_gui_py</run_depend>
<run_depend>rosgraph_msgs</run_depend>
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry to being overly picky - can you please move this run dependency to be in alphabetical order of the other run dependencies. That makes it much easier to find stuff especially when the list grow over time.

@dirk-thomas
Copy link
Contributor

Thanks for doing the changes so promptly. After you have addressed the last comment (run_depend order) can you please also squash your commits into a single one? After that I will happily merge the pull request.

dirk-thomas added a commit that referenced this pull request Jan 21, 2014
rqt_bag: publish clock time from bag
@dirk-thomas dirk-thomas merged commit 7f6e66e into ros-visualization:groovy-devel Jan 21, 2014
@MatthiasNieuwenhuisen MatthiasNieuwenhuisen deleted the groovy-devel branch January 22, 2014 01:12
@mcamurri
Copy link

It does not work also for me. I can see the suggested option on the help command, but then if I run it it does not publish. I have Indigo and I installed the packages through apt-get on Ubuntu 14.04

@MatthiasNieuwenhuisen
Copy link
Contributor Author

Works for me on Indigo, if I

  1. select --clock on the command line,
  2. enable some or all topics (right click on topic -> publish),
  3. push the play button.
    Please let me know if it is not working for you and what are the steps to reproduce your problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants