erwincoumans / tiny-differentiable-simulator

Tiny Differentiable Simulator is a header-only C++ and CUDA physics library for reinforcement learning and robotics with zero dependencies.
Apache License 2.0
1.2k stars 129 forks source link

CUDA codegen support for Windows #47

Closed eric-heiden closed 3 years ago

eric-heiden commented 3 years ago
erwincoumans commented 3 years ago

Thanks for the PR! mmm, travis build is not starting, reported here: https://travis-ci.community/t/build-not-starting/7748/20

erwincoumans commented 3 years ago

Perhaps closing re-open will trigger a build, trying now.

erwincoumans commented 3 years ago

The PR reverts/overwrites a lot of recent changes, for example in the CMakeLists.txt

 #include_directories("third_party/eigen3")

only targets that really use eigen will use target_include_directories:

target_include_directories(pendulum_example_eigen_gui PRIVATE ../src  ${EIGEN3_INCLUDE_DIRS})

and we added

IF(CMAKE_SYSTEM_NAME MATCHES "Linux")   
link_libraries(stdc++fs)    
ENDIF()

and various other places in the code:

Algebra::to_double(Algebra::abs(link.D)) > 0.0);

should be

 Algebra::abs(link.D) > Algebra::zero());             

and can we use cmake version 3.12 instead of 3.13?

Would it be possible to start from latest revision and not revert those changes?

erwincoumans commented 3 years ago

Thanks @eric-heiden , can you check my comment above?

eric-heiden commented 3 years ago

Hi Erwin, sorry for the delay! I reverted many of the changes now.

My Eigen3 installation seems to be messed up, so I wonder if we could just use the local Eigen3 distribution that we provide in third_party by default? I commented out find_package(Eigen3) for now.

This change:

Algebra::to_double(Algebra::abs(link.D)) > 0.0);

is necessary for CppADCodeGen. Otherwise we get this GreaterThanZero exception during the code generation when running it in debug mode.

I just added a definition for TDS_HOME that points to the project folder, so we can use FileUtils::find_file also when TDS is used as a submodule (none of the defined prefixes could resolve the path when I ran some demo with Visual Studio on Windows). In file_utils.hpp, I had to comment out

        if (exe_name_pos) {
          root = path_to_exe;
        }

Because none of the prefixes applied to the root folder would find the file in the TDS data folder on Windows with Visual Studio. This isn't necessary anymore with the TDS_HOME definition anyway.

erwincoumans commented 3 years ago

Thanks!

Yes, we can have an option to enable/disable system-Eigen and leave it disabled by default.

This line is very bad, we can add some #ifdef around it with a comment about buggy CppADCodeGen

Algebra::to_double(Algebra::abs(link.D)) > 0.0);

Can you describe more in detail about the exception? Is that a bug in CppADCodeGen?

In general, we shouldn't have "Algebra::to_double", but it should be named 'debugging_to_double' to make sure it is not used in our regular library. It breaks determinism for fixed point implementation for example.

eric-heiden commented 3 years ago

This exception is triggered for the CppADCodeGen scalar type, which for some reason is not a parameter variable:

if (!x.isParameter()) {
    throw cg::CGException("GreaterThanZero cannot be called for non-parameters");
}

https://github.com/joaoleal/CppADCodeGen/blob/master/include/cppad/cg/ordered.hpp#L23

I'm using the compile-time test is_cppad_scalar<Scalar>::value now to only convert to double for this CppAD scalar, so the other types behave the same as before.

eric-heiden commented 3 years ago

I just added the tests we had before for gradient computation and code generation.

erwincoumans commented 3 years ago

Thanks! Looks like the tests are failing:

/home/travis/build/google-research/tiny-differentiable-simulator/python/../src/dynamics/forward_dynamics.hpp:86:19: error: ‘is_cppad_scalar’ was not declared in this scope
   86 |     if constexpr (is_cppad_scalar<Scalar>::value) {
      |                   ^~~~~~~~~~~~~~~
erwincoumans commented 3 years ago

Thanks, CI is taking too long, I'll merge and fix open issues manually.