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

Physics lockstep for all sensors #2761

Merged
merged 33 commits into from
Jul 16, 2020

Conversation

mabelzhang
Copy link
Collaborator

@mabelzhang mabelzhang commented Jun 20, 2020

Part of issue #2736. Builds on top of #2746 (can't target that branch because that's on a fork. Should merge that one first).

Extends the physics lockstep from CameraSensor in #2746 to all sensors.

All non-rendering sensors are covered by new code in Sensor.

Rendering sensors that do not inherit from CameraSensor are GpuRaySensor and MultiCameraSensor. Each of these classes has new variables renderNeeded and nextRenderingTime, which are updated by new methods (SetActive(), PrerenderEnded(), NeedsUpdate(), NextRequiredTimestamp(), ResetLastUpdateTime()) and existing method Render().

Because of these new members, while it's possible to check whether a sensor is a rendering one via this->dataPtr->category == IMAGE and put everything in Sensor, I decided to only have these members in the rendering sensor classes, even if it means the methods look mostly similar (but not exactly the same because the actual sensor variable is different) across all 3 rendering sensors.

Tests for strict rate have been added to:
INTEGRATION_camera_sensor
INTEGRATION_gpu_laser
INTEGRATION_multicamera_sensor
INTEGRATION_logical_camera_sensor
INTEGRATION_contact_sensor
INTEGRATION_laser
INTEGRATION_imu
INTEGRATION_transceiver

mabelzhang added 16 commits May 24, 2020 03:27
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
…ailing

Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
…imestamp

Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
@mabelzhang mabelzhang requested a review from iche033 June 20, 2020 08:03
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
@mabelzhang mabelzhang marked this pull request as ready for review June 29, 2020 16:08
@mabelzhang mabelzhang marked this pull request as draft June 29, 2020 16:52
@mabelzhang mabelzhang closed this Jun 29, 2020
@mabelzhang mabelzhang reopened this Jun 29, 2020
@mabelzhang
Copy link
Collaborator Author

Sorry about the fidgeting above. The 5 latest commits weren't showing up in the Draft state for some reason. Closing and reopening the PR made them show up.

Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
@mabelzhang mabelzhang marked this pull request as ready for review July 10, 2020 07:49
@mabelzhang mabelzhang closed this Jul 10, 2020
@mabelzhang mabelzhang reopened this Jul 10, 2020
@mabelzhang mabelzhang changed the base branch from respect_fps_gz11new to respect_fps July 10, 2020 07:51
@mabelzhang mabelzhang changed the base branch from respect_fps to respect_fps_gz11new July 10, 2020 07:51
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
@mabelzhang
Copy link
Collaborator Author

This is ready for review.
The only thing pending is I can't quite get the new transceiver test to work yet. I have to manually call Update() for some reason, otherwise it hangs. It doesn't Update() automatically when physics is unpaused... Is there something special about this sensor or am I missing a flag? I call Init(), SetActive(true) on the sensor, and then I call SetPause(false). No messages come in. I will look more into this.

Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
@mabelzhang
Copy link
Collaborator Author

Now that we aren't using the SDF tag, some .world files can be removed. They are mostly copies of existing ones except the <update_rate> tag, which we can set via SetUpdateRate(). I'll change that.

Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
@mabelzhang
Copy link
Collaborator Author

Transceiver test is done.
I've added all the sensor tests that are currently being tested with a world file.

I tried to eliminate some new .world SDF files to reduce the line diffs, but I couldn't get them to pass the tests. With laser, it seems calling SetUpdateRate() has no effect, maybe the sensors are already completely loaded after Load()? Logical camera I suspect will have the same issue. With contact sensor, it got a null pointer for the sensor whenever I switched to the existing world file, which is only different from the one I added by one line, the <update_rate> line. I can't see a reason it'd get a null pointer for the sensor for one file but not the other. Most peculiar.

So it seems the most solid to test with the new .world files specifying a large <update_rate> directly. The other new world files (gpu laser, imu, multicamera) are very stripped down from the original ones, so I think these are good to keep anyway.

@iche033
Copy link
Contributor

iche033 commented Jul 11, 2020

I wonder if the update rate issue you are experiencing is due to the same problem mentioned in pull request #2784. I can see that bug occurring if you are calling SetUpdateRate with a larger value than what's defined in SDF since the sensor manager's update loop is being throttled by the original rate.

Copy link
Contributor

@iche033 iche033 left a comment

Choose a reason for hiding this comment

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

changes look good to me. I tested the remaining rendering sensors (depth, gpu ray, and multicamera) and confirm that lockstep is working. There is one test failure to address.

I made one comment about ABI which I think we don't have to do anything for now. Other places look fine and I don't see an ABI issue. There are probably more ABI issues when merging from respect_fps_gz11new branch to gazebo11. Once this is merged, we could just create a draft PR and look at the abi-checker report to find all ABI issues.


/// \brief Return the next timestamp going to be used by the sensor
/// \return the timestamp
public: double NextRequiredTimestamp() const override;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think overriding these functions should be fine according to explanation here since we're overriding virtual functions from primary base class

double rate = static_cast<double>(totalMsgs) / dt;
gzdbg << "timer [" << dt << "] seconds rate [" << rate << "] fps\n";
const double tolerance = 0.02;
EXPECT_GT(rate, updateRate * (1 - tolerance));
Copy link
Contributor

Choose a reason for hiding this comment

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

jenkins build caught a test failure here. I can also reproduce it locally.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay. In 02269c0, I am disabling that test for Simbody, which is test number 2 that is failing. Many other tests in this file disabled it, and I thought about that but left it in since it didn't give me a problem. Maybe that was because I don't have Simbody installed. Let's see if Jenkins is happy now.

Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
@mabelzhang
Copy link
Collaborator Author

mabelzhang commented Jul 14, 2020

same problem mentioned in pull request #2784

Does that need to be in gazebo11 or do we just leave it, since the tests here now work around it?

@mabelzhang
Copy link
Collaborator Author

mabelzhang commented Jul 14, 2020

In e7da851, I changed Sensor::useStrictRate to be static, re Steve's comment in Sensor.hh https://github.com/osrf/gazebo/pull/2746/files#r453859682
We said that since lockstep is now a global setting, this variable can be static for ABI compatibility.

@iche033
Copy link
Contributor

iche033 commented Jul 14, 2020

Does that need to be in gazebo11 or do we just leave it, since the tests here now work around it?

I think we can leave the changes here as they are now since the tests are passing.

@iche033
Copy link
Contributor

iche033 commented Jul 14, 2020

the latest changes look good to me. I'll wait for jenkins build to finish

@iche033
Copy link
Contributor

iche033 commented Jul 15, 2020

there are a few tests failing in the ubuntu-gpu-nvidia build due to a weird boost problem, which I can't reproduce locally. It could just be an issue on the jenkins machine.

Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
@mabelzhang
Copy link
Collaborator Author

I looked at the ones on ubuntu_auto-amd64-gpu-nvidia that had actual errors (as opposed to "test did not run" kind of thing), and looks like it's these ones:
PhysicsEngines/JointForceTorqueTest.GetForceTorqueWithAppliedForce/3
PhysicsEngines/JointForceTorqueTest.GetForceTorqueWithAppliedForceReset/3
PhysicsEngines/Pioneer2dx.StraightLine/1
JointInspector_TEST.AppliedSignal
ModelData_TEST.BoundingCollision
ManTest.gzserver
ManTest.gazebo

The ManTest ones I tried to fix in 9c61a7c.

All the others I can reproduce locally. They are also failing on the buildfarmer dashboard though, so I think they are not new:
https://build.osrfoundation.org/job/gazebo-ci-gazebo11-bionic-amd64-gpu-nvidia/lastBuild/testReport/

Copy link
Contributor

@iche033 iche033 left a comment

Choose a reason for hiding this comment

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

good catch on the ManTest. The latest builds look good now.

@mabelzhang mabelzhang merged commit 0b290dd into respect_fps_gz11new Jul 16, 2020
@mabelzhang mabelzhang deleted the respect_fps_gz11_allSensors branch July 16, 2020 00:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants