-
Notifications
You must be signed in to change notification settings - Fork 276
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
Optical Tactile Sensor Plugin #229
Optical Tactile Sensor Plugin #229
Conversation
Signed-off-by: Martiño Crespo <marticres@gmail.com>
Signed-off-by: Martiño Crespo <marticres@gmail.com>
Signed-off-by: Martiño Crespo <marticres@gmail.com>
Signed-off-by: Martiño Crespo <marticres@gmail.com>
Signed-off-by: Martiño Crespo <marticres@gmail.com>
Signed-off-by: Martiño Crespo <marticres@gmail.com>
Signed-off-by: Martiño Crespo <marticres@gmail.com>
Signed-off-by: Martiño Crespo <marticres@gmail.com>
Signed-off-by: Martiño Crespo <marticres@gmail.com>
Signed-off-by: Martiño Crespo <marticres@gmail.com>
Signed-off-by: Martiño Crespo <marticres@gmail.com>
Signed-off-by: Martiño Crespo <marticres@gmail.com>
Signed-off-by: Martiño Crespo <marticres@gmail.com>
Signed-off-by: Martiño Crespo <marticres@gmail.com>
CC @mabelzhang |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR! I did a first pass. Mostly performance comments and style. Some of the style comments you should see if you run a linter. colcon test --packages-select ignition-gazebo4
probably has some. The checks at the bottom of the PR should also tell you style problems. If you click on the fail ones, they should take you to Jenkins and give you the diffs of new style errors.
Signed-off-by: Martiño Crespo <marticres@gmail.com>
Signed-off-by: Martiño Crespo <marticres@gmail.com>
Signed-off-by: Martiño Crespo <marticres@gmail.com>
@mabelzhang / @azeey : both of you have requests for changes. Would it be possible to do another review? Thanks! |
Looks like this comment is unresolved: #229 (comment) |
@osrf-jenkins run tests |
Mac CI has a new warning https://build.osrfoundation.org/job/ignition_gazebo-ci-pr_any-homebrew-amd64/3994/
That should be an easy fix. Test failures: @chapulina are these flakey by any chance? The reference build 3993 has a different set of failing tests, so I can't really tell. Looks like the badge linked from osrf/buildfarmer has yet a different set of failing tests ha... |
We don't have many builds on
But the And It may be worth rerunning just to see if it wasn't a spurious failure. |
Ah, cool, took me a few minutes to figure out the correct order to click on History to get to those pages you linked. Nice graphs. Jenkins could use better UI. Ubuntu CI has one failure, |
@osrf-jenkins run tests |
Grabbing the results before Jenkins shuts down today. Ubuntu CI: I am not sure if the ContactSystem test came from this PR... I saw another failure before 4316, and I think the earlier one is not from this PR. I didn't write down the number.
Mac CI: So looks like the ones from 3994 above were flaky. |
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Left just a couple of minor comments. Feel free to address them in the follow up PR.
_normalForce * this->forceLength, normalForceOrientationFromSensor); | ||
|
||
normalForcePoseFromWorld = | ||
(normalForcePoseFromSensor + this->depthCameraOffset) + _sensorWorldPose; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using Pose3f::operator*
instead of Pose3f::operator+
since the latter is discouraged. If you do, note that you'll have to swap the order of operations
|
||
////////////////////////////////////////////////// | ||
OpticalTactilePluginVisualization::OpticalTactilePluginVisualization( | ||
std::string &_modelName, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these be const refs?
ignition_gazebo-ci-pr_any-homebrew-amd64 is still reporting some failures:
The first one I don't see anywhere else. Related to the second one, level manager had some related failures, on ignition_gazebo-ci-pr_any-ubuntu_auto-amd64's 2 out of 3 failures are expected ( However, seeing that all the code changes in this PR are strictly within a new directory |
@osrf-jenkins run tests |
Alright, those two on homebrew-amd64 were flaky. Not failing on the rerun. Rerun fails on INTEGRATION_breadcrumbs which is flaky. ubuntu_auto-amd64 still failing on ServerRepeat/VelocityControlTest.PublishCmd/0, which is unexpected.
However, the previous run had 4 unexpected comparison outcomes, the latest one only has 2. So it could just be flaky. |
@osrf-jenkins run tests |
It looks like ServerRepeat/VelocityControlTest.PublishCmd/0 on ubuntu_auto-amd64 is really failing just for this PR. All other tests are passing. Need to look into it. |
Huh, that test is failing elsewhere too I think it's flaky. |
I looked again, the 3 failures of the same test above come from 3 different PRs, so it's definitely flaky. Merging this. |
@mabelzhang was this just a regular merge? It looks like we forgot to squash the PR into one commit before merging. Here's the diff between |
Yes just a regular merge, sorry I wasn't sure what was the preferred method of merge and just went with the default. Noted for the future. |
Optical Tactile Sensor Plugin for GSoC 2020.
It retrieves the contacts given by a contact sensor and computes the normal surfaces of an object given the data returned by a Depth Camera. It also visualizes these forces in a similar way as Gazebo Classic, so this PR partially addresses #112.
Future work will be focused on: