Add <include> tag support in MJCF parser #2557
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
👋 Hi,
This is a reminder message to assign an extra build label to this Pull Request if needed.
By default, this PR will be build with minimal build options (URDF support and Python bindings)
The possible extra labels are:
- build_collision (build Pinocchio with coal support)
- build_casadi (build Pinocchio with CasADi support)
- build_autodiff (build Pinocchio with CppAD support)
- build_codegen (build Pinocchio with CppADCodeGen support)
- build_extra (build Pinocchio with extra algorithms)
- build_mpfr (build Pinocchio with Boost.Multiprecision support)
- build_sdf (build Pinocchio with SDF parser)
- build_accelerate (build Pinocchio with APPLE Accelerate framework support)
- build_all (build Pinocchio with ALL the options stated above)
Thanks.
The Pinocchio development team.
| @@ -6,6 +6,91 @@ | |||
| #include "pinocchio/multibody/model.hpp" | |||
| #include "pinocchio/algorithm/contact-info.hpp" | |||
|
|
|||
| namespace | |||
| { | |||
| void updateMeshPaths(boost::property_tree::ptree & tree, const boost::filesystem::path & basePath) | |||
There was a problem hiding this comment.
Should I make them member functions?
There was a problem hiding this comment.
Hi, yes, since this function only handles Meshes, could you move it to the struct for mjcfMeshes, please ?
JafarAbdi
force-pushed
the
feature/mjcf_include
branch
from
42b84ac to
ac9bc4a
Compare
|
Hello, Thanks a lot for the PR, it is very welcome. I made a few comments, when you have time to look at them. Thanks again |
Thanks! I think you forgot to submit your feedback 😄 |
There was a problem hiding this comment.
Hi, could you move all the new functions into the namespace below (pinocchio/mjcf/details) ?
| @@ -6,6 +6,91 @@ | |||
| #include "pinocchio/multibody/model.hpp" | |||
| #include "pinocchio/algorithm/contact-info.hpp" | |||
|
|
|||
| namespace | |||
| { | |||
| void updateMeshPaths(boost::property_tree::ptree & tree, const boost::filesystem::path & basePath) | |||
There was a problem hiding this comment.
Hi, yes, since this function only handles Meshes, could you move it to the struct for mjcfMeshes, please ?
| auto it = tree.begin(); | ||
| while (it != tree.end()) | ||
| { | ||
| if (it->first == "include") |
There was a problem hiding this comment.
In the mujoco doc (https://mujoco.readthedocs.io/en/stable/XMLreference.html#include) they recommand to use the attach keyword.
Would it be a lot of work to also add support for this tag, since they essentially do the same thing according to the documentation ?
unittest/mjcf.cpp
Outdated
There was a problem hiding this comment.
From my understanding, can be with any type of files, including a model file. Could you add some a test where the included file contains and some joints ?
Also, do you know how the compiler is dealt with, in case it appears in both files ? And how is the default tag handled as well ?
Could you add some test to make sure our parser does the same as in mujoco concerning these two points ?
Thank you !
jcarpent
force-pushed
the
feature/mjcf_include
branch
from
5c58c51 to
22233ad
Compare
This PR adds support for the tag in the MJCF parser, enabling nested includes and updating mesh paths relative to the included file's location. Includes some unit tests!
To test with a more complex model try https://github.com/RainbowRobotics/rby1-sdk/tree/main/models/rby1a/mujoco