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

Set slip compliance #56

Merged
merged 18 commits into from
Sep 17, 2020
Merged

Set slip compliance #56

merged 18 commits into from
Sep 17, 2020

Conversation

JShep1
Copy link

@JShep1 JShep1 commented May 13, 2020

Required for the wheel slip plugin

…rtsim, so slip compliance can be adjusted dynamically (needed by WheelSlipPlugin)
@JShep1 JShep1 requested a review from mxgrey as a code owner May 13, 2020 00:35
@github-actions github-actions bot added the 📜 blueprint Ignition Blueprint label May 13, 2020
@JShep1 JShep1 added the enhancement New feature or request label May 13, 2020
@JShep1 JShep1 marked this pull request as draft May 13, 2020 00:48
@codecov
Copy link

codecov bot commented May 13, 2020

Codecov Report

Merging #56 into ign-physics1 will decrease coverage by 0.27%.
The diff coverage is 40.90%.

Impacted file tree graph

@@               Coverage Diff                @@
##           ign-physics1      #56      +/-   ##
================================================
- Coverage         83.21%   82.93%   -0.28%     
================================================
  Files                75       75              
  Lines              2436     2484      +48     
================================================
+ Hits               2027     2060      +33     
- Misses              409      424      +15     
Impacted Files Coverage Δ
dartsim/src/SDFFeatures.cc 71.84% <ø> (+1.29%) ⬆️
dartsim/src/ShapeFeatures.cc 36.42% <31.57%> (-1.63%) ⬇️
include/ignition/physics/detail/Shape.hh 100.00% <100.00%> (ø)
include/ignition/physics/detail/GetEntities.hh 100.00% <0.00%> (ø)
dartsim/src/EntityManagementFeatures.cc 67.03% <0.00%> (+3.84%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5b9013e...827f80d. Read the comment docs.

@JShep1 JShep1 marked this pull request as ready for review May 26, 2020 23:03
@JShep1 JShep1 requested a review from azeey May 26, 2020 23:03
scpeters and others added 3 commits May 26, 2020 16:47
…rtsim, so slip compliance can be adjusted dynamically (needed by WheelSlipPlugin)
…tics/ign-physics into set_slip_compliance

Signed-off-by: John Shepherd <john@openrobotics.org>
Copy link
Contributor

@azeey azeey left a comment

Choose a reason for hiding this comment

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

Looks good! I made some minor comments. Since calls to setSlipCompliance and setSecondarySlipCompliance are not implemented in the official version of DART yet, we should probably add guards like we did in here to the implementation of the slip compliance features in the dartsim plugin.

@scpeters
Copy link
Member

@osrf-jenkins run tests please

@scpeters
Copy link
Member

scpeters commented Jul 6, 2020

@osrf-jenkins run tests please

1 similar comment
@scpeters
Copy link
Member

scpeters commented Jul 7, 2020

@osrf-jenkins run tests please

John Shepherd added 2 commits July 15, 2020 17:06
Signed-off-by: John Shepherd <john@openrobotics.org>
@JShep1 JShep1 requested a review from azeey July 16, 2020 00:43
John added 3 commits July 15, 2020 17:44
Signed-off-by: John Shepherd <john@openrobotics.org>
…tics/ign-physics into set_slip_compliance

Signed-off-by: John Shepherd <john@openrobotics.org>
Signed-off-by: John Shepherd <john@openrobotics.org>
@azeey
Copy link
Contributor

azeey commented Aug 18, 2020

@scpeters pointed me to another relevant test in Gazebo11 that would be nice to add here https://github.com/osrf/gazebo/blob/a9f093f2fc7fa7fc49efa73719f6536ad28f8af7/test/integration/physics_friction.cc#L529. The motion of the objects seem different from Gazebo-classic, so I'm investigating why. I think there is a bug in DART when the center of mass is offset from the origin of a link.

Copy link
Contributor

@azeey azeey left a comment

Choose a reason for hiding this comment

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

LGTM!

@JShep1
Copy link
Author

JShep1 commented Aug 29, 2020

The wheel slip changes were merged into upstream dart, we will need new debs for CI. All tests both on physics and gazebo are passing for me locally. I'm holding off on merging these two PRs until we can get CI green.

@nkoenig
Copy link
Contributor

nkoenig commented Sep 16, 2020

DART has been released. I think this PR is next in line.

@nkoenig
Copy link
Contributor

nkoenig commented Sep 16, 2020

@osrf-jenkins run tests please

@azeey
Copy link
Contributor

azeey commented Sep 16, 2020

I was waiting on osrf/homebrew-simulation#1130

@azeey
Copy link
Contributor

azeey commented Sep 16, 2020

@osrf-jenkins run tests please

@azeey
Copy link
Contributor

azeey commented Sep 16, 2020

@JShep1 Can you rerun github action CI? Seems like I don't have permission to do it.

@nkoenig
Copy link
Contributor

nkoenig commented Sep 16, 2020

@JShep1 Can you rerun github action CI? Seems like I don't have permission to do it.

I don't understand why there is no button to re-run jobs. I think we all should have the same permissions.

@JShep1
Copy link
Author

JShep1 commented Sep 16, 2020

@JShep1 Can you rerun github action CI? Seems like I don't have permission to do it.

Seems like they were triggered by your comment @azeey , they show as being started an hour ago when you made the comment, perhaps it took a little bit to trigger them?

@azeey
Copy link
Contributor

azeey commented Sep 16, 2020

Seems like they were triggered by your comment @azeey , they show as being started an hour ago when you made the comment, perhaps it took a little bit to trigger them?

I meant just the Github Actions CI. My comment restarted the Jenkins CI, but not the Github Actions CI.

I also tried using the REST API to rerun it and it responded with "Unable to re-run this workflow run because it was created over a month ago". I think our best bet is to push an empty commit, but that will probably restart jenkins as well :(

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
@nkoenig
Copy link
Contributor

nkoenig commented Sep 16, 2020

I also tried using the REST API to rerun it and it responded with "Unable to re-run this workflow run because it was created over a month ago". I think our best bet is to push an empty commit, but that will probably restart jenkins as well :(

And now the button on the github actions is there. So, an old PR can loose CI buttons. Interesting.

@chapulina
Copy link
Contributor

👮‍♀️

There are a couple of old commits from @scpeters that aren't signed. I think it's fine to manually accept DCO for the PR, but please whoever squash-merges this PR, be sure to add his signature so we don't have issues merging this PR forward to other branches.

➡️ Signed-off-by: Steve Peters <scpeters@openrobotics.org>

And always remember not to remove other signatures.

@nkoenig
Copy link
Contributor

nkoenig commented Sep 17, 2020

The Ubuntu CI keeps failing. The logs show:

2020-09-16T23:34:51.5187965Z ##[error]The operation was canceled.

during compilation. I'll try again.

@nkoenig
Copy link
Contributor

nkoenig commented Sep 17, 2020

The Ubuntu CI keeps failing. The logs show:

2020-09-16T23:34:51.5187965Z ##[error]The operation was canceled.

during compilation. I'll try again.

This has happened three times. Unless anyone has ideas, I'll merge this sometime tonight.

@azeey
Copy link
Contributor

azeey commented Sep 17, 2020

It could be timing out. ign-physics1 takes a really long time to compile on Ubuntu with tests enabled.

@nkoenig
Copy link
Contributor

nkoenig commented Sep 17, 2020

It could be timing out. ign-physics1 takes a really long time to compile on Ubuntu with tests enabled.

It errors out at around 15-16 minutes. I'll try to muck with actions.

@chapulina
Copy link
Contributor

wow the actions CI usually runs for close to 3 hours for ign-physics1 😨

https://github.com/ignitionrobotics/ign-physics/actions?query=workflow%3A%22Ubuntu+CI%22+branch%3Aign-physics1

It's been over 2h 55min and it's still running for this branch

@nkoenig
Copy link
Contributor

nkoenig commented Sep 17, 2020

wow, it finished. I guess I was too impatient.

@nkoenig
Copy link
Contributor

nkoenig commented Sep 17, 2020

Okay. I think everything is fine. I'm merging.

@nkoenig nkoenig merged commit ba79fa7 into ign-physics1 Sep 17, 2020
@nkoenig nkoenig deleted the set_slip_compliance branch September 17, 2020 04:18
@chapulina chapulina mentioned this pull request Sep 21, 2020
@chapulina
Copy link
Contributor

Please remember to squash-merge feature PRs. The commits from this PR are now interleaved with the rest of the commits in the release branches are are difficult to identify.

We don't completely lose the separate commits when squash-merging. The squashed commit has a link to the PR and the PR page displays all the individual commits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📜 blueprint Ignition Blueprint enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants