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

Add callbacks for PlayerClock::jump(time_point) API #775

Merged
merged 5 commits into from
Jun 9, 2021
Merged

Add callbacks for PlayerClock::jump(time_point) API #775

merged 5 commits into from
Jun 9, 2021

Conversation

MichaelOrlov
Copy link
Contributor

@MichaelOrlov MichaelOrlov commented Jun 2, 2021 •
edited
Loading

Add callbacks for PlayerClock::jump(time_point) API

Part of #696

@MichaelOrlov
Add callbacks to the jump API
9b6ea84 
Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
@MichaelOrlov
Add unit tests for TimeControllerClock::jump callbacks
c2eb814 
Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
@MichaelOrlov MichaelOrlov changed the title Add callbacks for player clock jump API Add callbacks for player's clock jump API Jun 2, 2021
@MichaelOrlov MichaelOrlov marked this pull request as ready for review June 2, 2021 09:45
@MichaelOrlov MichaelOrlov requested a review from a team as a code owner June 2, 2021 09:45
@MichaelOrlov MichaelOrlov requested review from emersonknapp, david-prody and Karsten1987 and removed request for a team June 2, 2021 09:45
@MichaelOrlov
Address some code style issues and renames
0a3bb00 
Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
@MichaelOrlov MichaelOrlov changed the title Add callbacks for player's clock jump API Add callbacks for PlayerClock::jump(time_point) API Jun 2, 2021
This was referenced Jun 2, 2021
Closed
Closed
emersonknapp
emersonknapp approved these changes Jun 8, 2021
View reviewed changes
Copy link
Collaborator

@emersonknapp emersonknapp left a comment

Choose a reason for hiding this comment

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

Just a nitpick on the documentation formatting - I'd like to change that before we merge so that it is consistent, but this LGTM

rosbag2_cpp/include/rosbag2_cpp/clocks/player_clock.hpp Outdated Show resolved Hide resolved
@MichaelOrlov
Change Doxygen comments to C style.
ec46d02 
Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
@MichaelOrlov
Allow one of the callbacks to be nullptr.
ebcf5bf 
Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
@MichaelOrlov MichaelOrlov merged commit abd4229 into master Jun 9, 2021
@delete-merged-branch delete-merged-branch bot deleted the michaelorlov/player_clock-jump-callbacks branch June 9, 2021 20:43
@clalancette
Copy link
Contributor

Please make sure CI (on https://ci.ros2.org) gets run on these PRs (if it has been, it isn't obvious to me where). In this case, this has caused regressions on Windows: https://ci.ros2.org/view/nightly/job/nightly_win_rel/1961/

@emersonknapp
Copy link
Collaborator

Ah - there wasn't a run. @MichaelOrlov can you quickly fix the windows build? if not, please revert this PR and reopen so we can fix the CI before merging for good

@clalancette
Copy link
Contributor

Should we go ahead and revert this for now?

emersonknapp added a commit that referenced this pull request Jun 11, 2021
@emersonknapp
Revert "Add callbacks for PlayerClock::jump(time_point) API (#775)"
b219fb4 
This reverts commit abd4229.

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp
Copy link
Collaborator

Yes, #778

@emersonknapp emersonknapp mentioned this pull request Jun 11, 2021
Merged
emersonknapp added a commit that referenced this pull request Jun 11, 2021
@emersonknapp
Revert "Add callbacks for PlayerClock::jump(time_point) API (#775)" (#…
758d96b 
…778)

This reverts commit abd4229.

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp
Copy link
Collaborator

@MichaelOrlov I've merged the revert - could you take a look at the windows failures and reopen this contribution?

@MichaelOrlov
Copy link
Contributor Author

@emersonknapp @clalancette Sorry guys, I have not seen notification. And my apologies for the broken CI.

I will take a look on breakage.

@MichaelOrlov
Copy link
Contributor Author

@emersonknapp @clalancette I figured out. Issue was in warnings from MSBuild about type cast in unit tests.

However I don't know how to reopen this MR since branch has been deleted and I don't see any options to reopen MR.
Should I create a new one with the same changes?

@MichaelOrlov MichaelOrlov mentioned this pull request Jun 11, 2021
Merged
@clalancette
Copy link
Contributor

However I don't know how to reopen this MR since branch has been deleted and I don't see any options to reopen MR.
Should I create a new one with the same changes?

Yes, that seems to be the right thing to do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clalancette clalancette clalancette left review comments

@emersonknapp emersonknapp emersonknapp approved these changes

@david-prody david-prody Awaiting requested review from david-prody

@Karsten1987 Karsten1987 Awaiting requested review from Karsten1987

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

Successfully merging this pull request may close these issues.

None yet
3 participants
@MichaelOrlov @clalancette @emersonknapp