Skip to content

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

Merged
MichaelOrlov merged 5 commits intomasterfrom
michaelorlov/player_clock-jump-callbacks
Jun 9, 2021
Merged

Add callbacks for PlayerClock::jump(time_point) API#775
MichaelOrlov merged 5 commits intomasterfrom
michaelorlov/player_clock-jump-callbacks

Conversation

@MichaelOrlov
Copy link
Copy Markdown
Contributor

@MichaelOrlov MichaelOrlov commented Jun 2, 2021 •
edited
Loading

Add callbacks for PlayerClock::jump(time_point) API

Part of #696

@morlov-apexai
Add callbacks to the jump API
9b6ea84 
Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
@morlov-apexai
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 Karsten1987, david-prody and emersonknapp and removed request for a team June 2, 2021 09:45
@morlov-apexai
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
Copy Markdown
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

@morlov-apexai
Change Doxygen comments to C style.
ec46d02 
Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
MichaelOrlov
MichaelOrlov commented Jun 9, 2021
View reviewed changes
@morlov-apexai
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
Copy Markdown
Contributor

clalancette commented Jun 11, 2021

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
Copy Markdown
Collaborator

emersonknapp commented Jun 11, 2021

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
Copy Markdown
Contributor

clalancette commented Jun 11, 2021

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
Copy Markdown
Collaborator

emersonknapp commented Jun 11, 2021

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
Copy Markdown
Collaborator

emersonknapp commented Jun 11, 2021

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

@MichaelOrlov
Copy link
Copy Markdown
Contributor Author

MichaelOrlov commented Jun 11, 2021

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

I will take a look on breakage.

MichaelOrlov
MichaelOrlov commented Jun 11, 2021
View reviewed changes
@MichaelOrlov
Copy link
Copy Markdown
Contributor Author

MichaelOrlov commented Jun 11, 2021

@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
Copy Markdown
Contributor

clalancette commented Jun 14, 2021

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.

4 participants

@MichaelOrlov @clalancette @emersonknapp @morlov-apexai