search

gazebosim / gazebo-classic

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

J_orig not correct inside ODE quickstep PGS_LCP solver #1515

Open osrf-migration opened 9 years ago

osrf-migration commented 9 years ago

Original report (archived issue) by Ying Lu (Bitbucket: rosebudflyaway).

The J_orig is allocated here, to keep a clean copy of J, but we didn't copy the values in J to J_orig after we create J

The J_orig is passed to call the PGS_LCP here, with wrong values.

Inside PGS_LCP function, J_orig is used here, which is inside the preconditioning computation, of which precon_iteration = 0, so it doesn't affect the performance.

Once preconditioning is enabled, there would be an issue. I will make a pull request on this

osrf-migration commented 9 years ago

Original comment by Ying Lu (Bitbucket: rosebudflyaway).

osrf-migration commented 9 years ago

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

Nice catch! Looks like a bug to me.

osrf-migration commented 9 years ago

Original comment by Ying Lu (Bitbucket: rosebudflyaway).

add one more line here, right after the create J operation.

I will make pull request with another one, since this is a one-line change.

osrf-migration commented 9 years ago

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

I am confused by this bug, isn't J_orig assigned here?

osrf-migration commented 9 years ago

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

Good point @hsu , I had forgotten that quickstep.cpp had already been split up. It does look like J_orig is set at quickstep_pgs_lcp.cpp:912

osrf-migration commented 9 years ago

Original comment by Ying Lu (Bitbucket: rosebudflyaway).

The place that John pointed here is inside if (qs->precon_iterations > 0) here. But the default branch has qs->precon_iterations = 0

osrf-migration commented 9 years ago

Original comment by Ying Lu (Bitbucket: rosebudflyaway).

Maybe we could assign J_orig here, and we don't have to worry about assign values to J_orig inside the solvers.

osrf-migration commented 7 years ago

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