search

gazebosim / gazebo-classic

Gazebo classic. For the latest version, see https://github.com/gazebosim/gz-sim
http://classic.gazebosim.org/
Other
1.19k stars 480 forks source link

InelasticCollision/0 ODE test failure #1394

Open osrf-migration opened 9 years ago

osrf-migration commented 9 years ago

Original report (archived issue) by Steve Peters (Bitbucket: Steven Peters, GitHub: scpeters).

The behavior of ODELink::GetWorldForce was changed in pull request #1284, targeted to gazebo_2.2. I discovered a test failure in gazebo3 and up after merging forward:

195: [ RUN      ] PhysicsEngines/PhysicsTest.InelasticCollision/0
...
195: test/integration/physics.cc:962: Failure
195: Value of: box_model->GetLink("link")->GetWorldForce() == math::Vector3(f, 0, 0)
195:   Actual: false
195: Expected: true

Prior to pull request #1284, the Link::GetWorldForce function returned a measure of the current force on a link. After, it returned a cached value from the previous timestep. This test is now failing because it calls Link::SetForce and then immediately expects Link::GetWorldForce to match that value. I believe it would require a simulation step for the force to propagate to the cache.

@hsu what is the intended behavior of Link::GetWorldForce? Is the test expectation valid? Will this cause problems for users such that we shouldn't push this out on a patch release?

Also, this should be a reminder that adding features to old branches can cause problems when merging forward.

osrf-migration commented 9 years ago

Original comment by Steve Peters (Bitbucket: Steven Peters, GitHub: scpeters).

Improved error message in branch issue_1394 (08e046105735670e5b6c06fdc9337e9d4d04071d):

[ RUN      ] PhysicsEngines/PhysicsTest.InelasticCollision/0
[Msg] Waiting for master.
[Msg] Connected to gazebo master @ http://127.0.0.1:11345
[Msg] Publicized address: 172.23.2.90

[Wrn] [ODEPhysics.cc:219] Gravity vector is (0, 0, 0). Objects will float.
[Dbg] [ServerFixture.cc:144] ServerFixture load in 0.8 seconds, timeout after 600 seconds
/home/scpeters/ws/gazebo/src/gazebo/test/integration/physics.cc:965: Failure
Value of: math::Vector3(f, 0, 0)
  Actual: 1000 0 0
Expected: box_model->GetLink("link")->GetWorldForce()
Which is: 0 0 0
[Dbg] [ServerFixture.cc:95] ServerFixture::Unload
[  FAILED  ] PhysicsEngines/PhysicsTest.InelasticCollision/0, where GetParam() = "ode" (4108 ms)
osrf-migration commented 9 years ago

Original comment by Steve Peters (Bitbucket: Steven Peters, GitHub: scpeters).

@hsu

osrf-migration commented 9 years ago

Original comment by John Hsu (Bitbucket: hsu, GitHub: hsu).

I guess the question is: is pull request #1284 a bug fix or a feature change for GetWorldForce? If former, the original test is validating a bug :)

Original documentation on GetWorldForce

      /// \brief Get the force applied to the body in the world frame.
      /// \return Force applied to the body in the world frame.
      public: virtual math::Vector3 GetWorldForce() const = 0;

is vague. If by Force applied to the body, we are referring to all forces applied to the body from previous time step, then I vote we remove or update the original failing unit test. Either way, this raises the issue that we should update documentation to be more clear on what is meant by it.

osrf-migration commented 9 years ago

Original comment by Steve Peters (Bitbucket: Steven Peters, GitHub: scpeters).

While we're at it, we should clarify SetForce and AddForce, since SetForce is called right before the failing expectation. See also #878.

Since there's no easy answer, I'll propose to disable the failing expectation with an error message pointing to this issue.

osrf-migration commented 9 years ago

Original comment by Jose Luis Rivero (Bitbucket: Jose Luis Rivero, GitHub: j-rivero).

+1 for deactivating known failing tests.

osrf-migration commented 9 years ago

Original comment by Steve Peters (Bitbucket: Steven Peters, GitHub: scpeters).

Test deactivated in 8eeae31f4ee1f920722fb32696c403e638d467b0 (branch issue_1394_3.1). Awaiting jenkins results...

osrf-migration commented 9 years ago

Original comment by Steve Peters (Bitbucket: Steven Peters, GitHub: scpeters).

Jenkins is a little broken right now...

https://bitbucket.org/osrf/release-tools/commits/a3907ffe7b09eb50b75799adb72fd4696177a80b

osrf-migration commented 9 years ago

Original comment by Steve Peters (Bitbucket: Steven Peters, GitHub: scpeters).

test disabled in pull request #1405

osrf-migration commented 8 years ago

Original comment by Nate Koenig (Bitbucket: Nathan Koenig).