google-deepmind / mujoco_mpc

Real-time behaviour synthesis with MuJoCo, using Predictive Control
https://github.com/deepmind/mujoco_mpc
Apache License 2.0
952 stars 142 forks source link

Fix protobuf includes #215

Closed hartikainen closed 7 months ago

hartikainen commented 10 months ago

Tested and working on M2 Mac with brew install protobuf.

Should fix #214

hartikainen commented 10 months ago

Tests are currently failing due to missing protobuf.

nimrod-gileadi commented 10 months ago

Looks like the Ubuntu build is failing because of strict checks on proto 3 syntax. I think the thing it's complaining about was fixed in later versions of the proto compiler, where optional is supported for proto 3 files.

Maybe it's because we're using an old image of Ubuntu (to get around the latest issue with incompatible clang and stdc++).

a1k0n commented 10 months ago

Thanks for this! I was able to build with these changes, combined with @hongmindavidkim's comment: https://github.com/google-deepmind/mujoco_mpc/issues/214#issuecomment-1797847399

Here's the full diff I used:

diff --git a/mjpc/CMakeLists.txt b/mjpc/CMakeLists.txt
index e8e4dd3..f8d1206 100644
--- a/mjpc/CMakeLists.txt
+++ b/mjpc/CMakeLists.txt
@@ -24,8 +24,8 @@ target_link_libraries(
 )
 target_include_directories(threadpool PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/..)

-set(_PROTOBUF_LIBPROTOBUF libprotobuf)
-set(_PROTOBUF_PROTOC $<TARGET_FILE:protoc>)
+find_package(Protobuf REQUIRED)
+include_directories(${Protobuf_INCLUDE_DIRS})

 get_filename_component(agent_state_proto "./agent_state.proto" ABSOLUTE)
 get_filename_component(agent_state_proto_path "${agent_state_proto}" PATH)
@@ -36,11 +36,12 @@ add_custom_command(
   OUTPUT
   "${agent_state_proto_srcs}"
   "${agent_state_proto_hdrs}"
-  COMMAND ${_PROTOBUF_PROTOC}
+  COMMAND ${Protobuf_PROTOC_EXECUTABLE}
   ARGS
   --cpp_out "${CMAKE_CURRENT_BINARY_DIR}"
   -I "${agent_state_proto_path}"
   "${agent_state_proto}"
+  --experimental_allow_proto3_optional
   DEPENDS
   "${agent_state_proto}"
 )
@@ -52,7 +53,7 @@ add_library(agent_state_proto_lib STATIC

 target_link_libraries(
   agent_state_proto_lib
-  ${_PROTOBUF_LIBPROTOBUF}
+  ${Protobuf_LIBRARIES}
 )

 add_library(
@@ -169,7 +170,7 @@ target_link_libraries(
   lodepng
   mujoco::mujoco
   mujoco::platform_ui_adapter
-  ${_PROTOBUF_LIBPROTOBUF}
+  ${Protobuf_LIBRARIES}
   threadpool
   Threads::Threads
 )
@@ -192,7 +193,7 @@ target_link_libraries(
   agent_state_proto_lib
   libmjpc
   mujoco::mujoco
-  ${_PROTOBUF_LIBPROTOBUF}
+  ${Protobuf_LIBRARIES}
   threadpool
   Threads::Threads
 )
diff --git a/mjpc/agent.cc b/mjpc/agent.cc
index 7b6cc5b..898ea5f 100644
--- a/mjpc/agent.cc
+++ b/mjpc/agent.cc
@@ -250,12 +250,12 @@ agent_state::State Agent::GetAgentState(mjModel* model, mjData* data) {
     if (absl::StartsWith(numeric_name, "residual_select_")) {
       std::string_view name =
           absl::StripPrefix(numeric_name, "residual_select_");
-      (*task_parameters)[name].set_selection(mjpc::ResidualSelection(
+      (*task_parameters)[std::string(name)].set_selection(mjpc::ResidualSelection(
           agent_model, name, ActiveTask()->parameters[shift]));
       shift++;
     } else if (absl::StartsWith(numeric_name, "residual_")) {
       std::string_view name = absl::StripPrefix(numeric_name, "residual_");
-      (*task_parameters)[name].set_numeric(ActiveTask()->parameters[shift]);
+      (*task_parameters)[std::string(name)].set_numeric(ActiveTask()->parameters[shift]);
       shift++;
     }
   }
hartikainen commented 10 months ago

I'm still having problems when trying to build the gRPC together with the main mjpc binary (as seen in the failing tests). I'll try to look into fixing this early next week.

vikashplus commented 9 months ago

I was able to use this fix to get things working on Mac-M3 today. Do we have any update for Ubuntu?

thowell commented 7 months ago

@hartikainen I think the issue is fixed, please reopen if closing is a mistake.