-
Notifications
You must be signed in to change notification settings - Fork 9
Bump MuJoCo library version to 3.4.0 #46
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bump MuJoCo library version to 3.4.0 #46
Conversation
|
@eholum @eholum-nasa The failing build is due to the removal of
What should we do in this case? Weirdly, it is failing now P.S. I found the breaking API in version 3.3.7 |
|
I would like to take a decision before any further syncs |
|
I later checked the codebase and realized that there was a commit introduced in 3.3.7 version of mujoco : google-deepmind/mujoco@401bf43#diff-3dc22ceeebd71304c41d349c6d273bda172ea88ff49c772dbdcf51b9b19bbd33R2943 has introduced a undocumented API break, but setting it to -1 everything works as usual. I'll open a PR to update their documentation |
|
I've proposed to update their changelog : google-deepmind/mujoco#2996 |
|
@eholum-nasa everything good now ;) |
eholum
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! There are some nice updates in that changelog so looking forward to trying some of them out...
I still think the entire CMakeLists.txt will be cleaned up a bit when the vendor package comes out, but I'm not sure how different versions will affect the need for the macro?
| // Use mjVERSION_HEADER and if it is greater than 337 then do one thing or another | ||
| // Needed due to | ||
| // https://github.com/google-deepmind/mujoco/commit/401bf431b8b0fe6e0a619412a607b5135dc4ded4#diff-3dc22ceeebd71304c41d349c6d273bda172ea88ff49c772dbdcf51b9b19bbd33R2943 | ||
| #if mjVERSION_HEADER < 337 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgive my ignorance here, should we just enforce that the header version match exactly what we expect in the CMakeLists.txt file? Would that make this conditional go away?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, but this allows people to go back to the older version and they don't have to touch the code to know what to fix. So, I would say it is better to have it handled also in the code
eholum
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WFM!
|
Thanks. I'll go ahead and merge it then ;) |

This PR bumps the mujoco version to 3.4.0.
The idea is once the CI is good here, I will also bump the mujoco_vendor and make releases
Closes: #22