robotlearn / raisimpy

Raisimpy: Python wrappers for RaiSim
MIT License
40 stars 11 forks source link

major bug fixes, compatibility to 0.4.1 #4

Closed jhwangbo closed 5 years ago

jhwangbo commented 5 years ago

Hi Brian, there are a few changes

  1. First of all, there was a serious memory issue in the code. In C++, there is no square bracket operator that can take two arguments. If you use two arguments, it will ignore one of the two and just use it as a single argument operator. The convert_np_to_mat function had this issue. I added a round bracket operator that can take two arguments are pushed to raisimLib v0.4.1. I changed raisimpy to use this operator instead.

  2. compile issue resolved. You wrapped many of the non-api methods as well. In pybind, you don't have to wrap all methods but only the api ones. This will save you time. and also in this way, you probably don't need the private includes of ODE.

  3. Mesh example added. from v0.4.0, you can use non-convex mesh in raisim. You can find the example in examples/meshes

  4. anymal training example fixed. Previously the dimension of the P gain of the PD controller was wrong in raisimGym example. raisimgym_py seems faster if I just set the number of thread to 1. This means that multithreading is actually slower than a single-threaded training. This is why I highly recommend wrapping the whole vectorized environment with pybind, as it is done in raisimGym. You can easily get X10 performance boost

cheers Jemin

bdelhaisse commented 5 years ago

Hi Jemin, Thanks for the fixes :)

  1. You are right; I have been coding a lot in Python recently and the square bracket operator escaped my attention. I saw that you also modified the MatDyn class to account for the round bracket operator, so I will also modify the other methods in converter.cpp (such as convert_np_to_matdyn, convert_np_to_transformation, etc) to account for these changes.
  2. I think I only wrapped the non-api methods (such as Ogre/ODE related methods and structures) that you used in your examples and/or as arguments to your api methods/constructors (such as dSpaceID which was an argument to the Mesh constructor, or dGeomID which is given to the CollisionDefinition constructor). These dSpaceID and dGeomID are the reasons why I needed the private includes of ODE. As for other methods that were provided in raisimLib/Ogre, I wrapped most of the methods that were public, as at that time, I considered them to be part of the API (like init_collision_bodies that you just removed from raisimpy in the associated PR). The bug associated to the compilation issue is because the signature of init_collision_bodies has changed and dxTrimeshData has not been wrapped and the associated include file (which contains the definition and not only the declaration) has not been provided.
  3. Thanks, I will definitely test it this weekend :)
  4. I didn't have the time yet to check how to speed up and optimize the code in raisimpy_gym. @jhwangbo Just by curiosity, do you still have that X10 performance boost when you also use 1 thread with raisimGym?

Best, Brian

jhwangbo commented 5 years ago
  1. square brackets are fine with vectors. Just as it is in Eigen.
  2. The users should not create objects by themselves but only through the World class. In this case, you don't even have to define a constructor for object classes. Check out how I modified the Mesh class. There is an issue in raisim that object constructors are "public" instead of "protected". I guess this is misleading the users. I have to fix this soon
  3. Some benchmark timings on my machine (8700K, RTX 2080)

raisimgym 1 thread: 1.7 seconds per iteration 40 threads: 0.65 seconds per iteration

raisimpy_gym 1 thread: 8 seconds 40 threads: 12 seconds

raisimpy_gym no parallelization tool, only with for loop 1 thread: 6 seconds

I don't know how to profile parallel code in python but this is the profiling result for the third case

step() function in env.py (49.5 seconds)

The actual time spent on simulation is about a quarter of the total.