mosra / magnum-integration

Integration libraries for the Magnum C++11 graphics engine
https://magnum.graphics/
Other
99 stars 44 forks source link

Dart Integration #28

Closed costashatz closed 6 years ago

costashatz commented 6 years ago

This PR adds an integration to the DART simulator. In short:

Illustration: ezgif-5-e1da75dd98

@mosra let me know of any changes you would like me to do!

mosra commented 6 years ago

Oh, one more thing: what's the size of the test data, roughly? GitHub doesn't want to tell me :)

costashatz commented 6 years ago

Oh, one more thing: what's the size of the test data, roughly? GitHub doesn't want to tell me :)

I removed the big files (most probably they are in the git memory, but I guess we can remove them). So, the total file size of the test data at the moment is roughly 450KB!

mosra commented 6 years ago

450 kB? for all that? That's cool. Much less than data for some audio importer tests :D

costashatz commented 6 years ago

450 kB? for all that? That's cool. Much less than data for some audio importer tests :D

There's nothing really. The STL files occupy most of the space and then I borrowed the textured collada file from the AssimpImporter test. The URDF files are just text files and we have a few degrees of freedom for the arm..!

costashatz commented 6 years ago

@mosra I think I addressed all the issues you raised! Thanks for the extensive review! Let me know if you have any more comments..!

Two things that remain are:

I will try to address the first one as soon as possible and then we can see the second one!

mosra commented 6 years ago

Thank you for the fast responses! Will get back to it this evening, hopefully :)

coveralls commented 6 years ago

Coverage Status

Coverage increased (+6.8%) to 56.777% when pulling ba7676d69116164dabd5b95b86e82b4344c45455 on costashatz:dart-integration into c8ccb4559a21cb0b0af5038437f9247931d60809 on mosra:master.

mosra commented 6 years ago

Okay, I am very happy with the code now :) Last things that came to my mind:

costashatz commented 6 years ago

Does DART provide a FindDART.cmake module? Because there's a builtin FindDart.cmake and I doubt it's for DART simulation :) Because of the minor casing difference (that on windows would cause cmake to use the builtin module and then fail horribly), I think we should bundle FindDART.cmake in the modules/ directory in order to avoid such errors when DART is not installed. But I see only CMake configs in their repo and no Find module. (I hate this aspect of CMake, argh.) I don't know how well you are with CMake, so unless you want to tackle this yourself I can look into it and try to provide some proxy FindDART,cmake that just redirects to their config module.

I am not as good as you, but I can give it a try (I am not horribly bad at CMake). I have to do the versioning thing also. If you have any hints of what I should do (for redirecting to their config module), please shoot. Overall, I certainly dislike CMake and I prefer python-based build systems (waf, scons, etc.), but CMake won the market so we have to bear with it! :stuck_out_tongue:

CI: if you are willing to build DART for Travis (since the PPA with DART packages is not whitelisted and since whitelisting new PPAs is not possible anymore since almost a year ago, argh, Travis!!), that would be great. If you don't want to, I fully understand, it's a messy job ;)

I will give this a try.

I.e., not using the underscore (these are public members) and naming them without the *Data suffix, as "Data" is already contained in the struct name

That's true. You're right. Now that we are on it, maybe we can rename the whole structure as well (VisualData is not the ideal name for it). I was thinking DartShapeData or something like this (maybe just ShapeData as it is inside the DartIntegration namespace). What do you think?

mosra commented 6 years ago

If you have any hints of what I should do

I have no idea, actually :D I'm doing the inverse (including the Find module from the config module) in MagnumConfig.cmake (I don't even remember anymore why exactly this was needed and what CMake "speciality" was this working around again, but whatever) so a similar thing could be done in the FindDART.cmake proxy and could "just work", hopefully. But there you don't know the location of their config module at all -- would need to search for it first probably (and then produce some nice error message if not found).

I will give this a try.

Thank you :+1:

ShapeData

Yup, ShapeData is nice :+1:

costashatz commented 6 years ago

Does DART provide a FindDART.cmake module

I was examining this and I realized that when installing DART, you get the following files:

/usr/local/lib/pkgconfig/dart.pc 

/usr/local/share/dart/cmake/DARTConfig.cmake
/usr/local/share/dart/cmake/DARTConfigVersion.cmake

The dart.pc is the one used to point to the other files. I have a feeling that we should not do anything and they (the developers of DART) took care of it.

mosra commented 6 years ago

I wanted to avoid this problem:

CMake Error at CMakeLists.txt:55 (find_package):
  By not providing "FindDART.cmake" in CMAKE_MODULE_PATH this project has
  asked CMake to find a package configuration file provided by "DART", but
  CMake did not find one.

  Could not find a package configuration file provided by "DART" with any of
  the following names:

    DARTConfig.cmake
    dart-config.cmake

  Add the installation prefix of "DART" to CMAKE_PREFIX_PATH or set
  "DART_DIR" to a directory containing one of the above files.  If "DART"
  provides a separate development package or SDK, be sure it has been
  installed.

This is what the people get when they don't have DART installed. Incredibly confusing message that CMake for some reason spits out instead of just saying "sorry, something called DART is not installed". This gets even worse on Windows, where due to the case insensivity of the file system an error similar to the following will be produced:

CMake Error at /usr/share/cmake-3.10/Modules/FindPackageHandleStandardArgs.cmake:137 (message):
  Could NOT find Dart (missing: DART_ROOT)
Call Stack (most recent call first):
  /usr/share/cmake-3.10/Modules/FindPackageHandleStandardArgs.cmake:378 (_FPHSA_FAILURE_MESSAGE)
  /usr/share/cmake-3.10/Modules/FindDart.cmake:30 (FIND_PACKAGE_HANDLE_STANDARD_ARGS)
  CMakeLists.txt:55 (find_package)

But -- let's keep this as-is for now. The solution doesn't seem very obvious at the moment and it can be always fixed later when people actually start complaining ;)

costashatz commented 6 years ago

@mosra can you check if what I did makes sense and if it works (on windows it should not!)? Thanks!

mosra commented 6 years ago

This will inevitably get complicated and I don't think we're able to provide a solution that's flexible enough ... :/

Got another idea (way too late, sorry): it's possible to say find_package(DART CONFIG REQUIRED), which will produce a bit shorter error message in case DART is not found and also behaves correctly on Windows (not aliasing with Dart anymore). I think we could live with that. So, instead of providing FindDART.cmake, could you just add CONFIG to all find_package(DART) commands?

costashatz commented 6 years ago

Got another idea (way too late, sorry): it's possible to say find_package(DART CONFIG REQUIRED), which will produce a bit shorter error message in case DART is not found and also behaves correctly on Windows (not aliasing with Dart anymore). I think we could live with that. So, instead of providing FindDART.cmake, could you just add CONFIG to all find_package(DART) commands?

Yes..! Why not?

mosra commented 6 years ago

Merged into master now. Thank you for the contribution, looking forward to the example ;)