Use lifecycle nodes for playback - implements mute/unmute

[danthony06]

Related to #55
Related to #675
Replaces #644

Introduces "mute/unmute" playback feature to suppress publishing of messages while time continues to flow. When rosbag2 is unmuted, time will have passed, so there are messages in the meantime that were skipped - the next published message will be up to date with the current time.

[emersonknapp]

Hey @danthony06 - is this mostly a rebase on master? It's not gonna be possible to mentally pull apart the changes, so i'm inclined to just merge it to the target branch and go from there. You agree?

[Karsten1987]

@danthony06 are you going to continue the work for this feature? If so, I'd actually propose pretty much the opposite of @emersonknapp's comment. That is, simply merge the current ROS2 into your branch and work from there. Merging it into the ros:start_paused_lifecycle branch might be complicated for you - not sure if you have the sufficient permissions to push/rebase that branch. If you'd like to continue this feature, I think it's best we close the other branches/PR in favor of yours.

[danthony06]

Yes, sorry for the slow response. I'll get on this tomorrow.

…

On Mon, Mar 8, 2021, 8:03 PM Karsten Knese ***@***.***> wrote: @danthony06 <https://github.com/danthony06> are you going to continue the work for this feature? If so, I'd actually propose pretty much the opposite of @emersonknapp <https://github.com/emersonknapp>'s comment. That is, simply merge the current ROS2 into your branch and work from there. Merging it into the ros:start_paused_lifecycle branch might be complicated for you - not sure if you have the sufficient permissions to push/rebase that branch. If you'd like to continue this feature, I think it's best we close the other branches/PR in favor of yours. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#666 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABBZWHCB3PD6YYH3HLF6RYLTCV6VJANCNFSM4YCZ6LEA> .

[danthony06]

I don't have permissions to pull into @mabelzhang 's branch. If possible, I'd like to go with @Karsten1987's approach, close the other PR, and just go with this branch. Does that sound okay?

[emersonknapp]

Yeah, I agree - it makes more sense for you to start a new PR from your branch if you'll be the primary one working on it. Sorry for the confusion. ONce you've opened yours, we can close this PR.

[danthony06]

Sorry, trying to merge in from the main branch on the main repository, and there's a lot of conflicts.

[emersonknapp]

@danthony06 I've updated the PR title and description with my best understanding of the feature being proposed here - can you double check that I've got it right?

[danthony06]

Yes, that's right. I'm targeting master for my merge, that's correct, right?

[emersonknapp]

Yes, that's right. I'm targeting master for my merge, that's correct, right?

Yes, you want to target your PR against master - you could use this PR and change the base

[emersonknapp]

It looks like you're still actively working on this. If so, could you convert it to a draft, and change it back to ready for review when you are ready for review?

[danthony06]

Will do. I finished merging changes back in front master, and I've got someone helping me test it.

…

On Mon, Mar 22, 2021, 10:36 PM Emerson Knapp ***@***.***> wrote: It looks like you're still actively working on this. If so, could you convert it to a draft, and change it back to ready for review when you are ready for review? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#666 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABBZWHD5IZKL7TG2A3WF6TDTFAEDNANCNFSM4YCZ6LEA> .

[danthony06]

@emersonknapp Could you directly convert this to a draft? Looks like you should be able to, according to this documentation:

https://github.blog/changelog/2020-04-08-convert-pull-request-to-draft/

[emersonknapp]

Yes. I've also gone ahead and changed the base while I'm at it. As the PR owner, you also have the ability to do these. The draft button just isn't obvious - it's small text under the "reviewers" list on the right panel.

[emersonknapp]

I just took a closer look at this - and I'd like to ask you to take a step back for a moment on implementation.

This overlaps with the functionality that was designed at #675 and is being collaboratively implemented/tracked in #696

I think you're hitting on a lot of important things - but the true extent of this PR was not communicated clearly. You are not implementing "mute/unmute" that I can see - the TimeTranslator functionality is clearly for pause/resume and rate of playback time control - and matches with the discussion going on at #689 (comment)

This PR contains too many features and high level architectural decisions to accept as-is, especially given the multiple parallel threads of work happening around this feature. In order to make progress and be able to have you contribute in this, we are going to need to coordinate. The best way I can see to do this is via the ROS 2 Tooling Working Group - I've decided to use the breakout session to coordinate - I hope you can make it

https://discourse.ros.org/t/tooling-wg-breakout-session-rosbag2-playback-time-control/19711?u=emersonknapp

[danthony06]

I should be able to make it then.

…

On Tue, Mar 30, 2021, 12:49 PM Emerson Knapp ***@***.***> wrote: I just took a closer look at this - and I'd like to ask you to take a step back for a moment on implementation. This overlaps with the functionality that was designed at #675 <#675> and is being collaboratively implemented/tracked in #696 <#696> I think you're hitting on a lot of important things - but the true extent of this PR was not communicated clearly. You are not implementing "mute/unmute" that I can see - the TimeTranslator functionality is clearly for pause/resume and rate of playback time control - and matches with the discussion going on at #689 (comment) <#689 (comment)> This PR contains too many features and high level architectural decisions to accept as-is, especially given the multiple parallel threads of work happening around this feature. In order to make progress and be able to have you contribute in this, we are going to need to coordinate. The best way I can see to do this is via the ROS 2 Tooling Working Group - I've decided to use the breakout session to coordinate - I hope you can make it https://discourse.ros.org/t/tooling-wg-breakout-session-rosbag2-playback-time-control/19711?u=emersonknapp — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#666 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABBZWHDHKBF5G4QJVCUPIATTGIFJ3ANCNFSM4YCZ6LEA> .

[danthony06]

So, is there still interest in this PR?

[emersonknapp]

So, is there still interest in this PR?

I saw you signed in to the WG meeting this morning, were you able to follow the discussion? Is there any functionality of this PR that is not currently being addressed by the tracking on #696 ?

If there is new functionality not covered there, let's figure out how it fits into the design, then open focused PRs for those individual features.

If this doesn't introduce any concpets that aren't there, do you want to pick up an unclaimed item from the list? Once #689 is landed then much of the remaining work should be easy to parallelize.

[danthony06]

Yes, I followed the meeting. I'm willing to help out on #696. Is there a particular piece you want me to look at?

[danthony06]

I'm closing this PR because of the changes introduced in #696.