diff --git a/doc/APIreference/functions.rst b/doc/APIreference/functions.rst index 96ade764..09619b39 100644 --- a/doc/APIreference/functions.rst +++ b/doc/APIreference/functions.rst @@ -47,7 +47,9 @@ mj_compile .. mujoco-include:: mj_compile -Compile spec to model. +Compile :ref:`mjSpec` to :ref:`mjModel`. A spec can be edited and compiled multiple times, returning a new +:ref:`mjModel` instance that takes the edits into account. +If compilation fails, :ref:`mj_compile` returns ``NULL``; the error can be read with :ref:`mjs_getError`. .. _mj_recompile: @@ -63,10 +65,8 @@ reallocate existing :ref:`mjModel` and :ref:`mjData` instances in-place. Second, newly added or removed degrees of freedom. This allows the user to continue simulation with the same model and data struct pointers while editing the model programmatically. -.. admonition:: Incomplete implementation - :class: attention - - This function is currently incomplete, preserving only ``mjData.qpos`` and ``mjData.qvel``. +:ref:`mj_recompile` returns 0 if compilation succeed. In the case of failure, the given :ref:`mjModel` and :ref:`mjData` +instances will be deleted; as in :ref:`mj_compile`, the compilation error can be read with :ref:`mjs_getError`. .. _mj_saveLastXML: diff --git a/doc/APIreference/functions_override.rst b/doc/APIreference/functions_override.rst index 4e7b6b50..d703cf94 100644 --- a/doc/APIreference/functions_override.rst +++ b/doc/APIreference/functions_override.rst @@ -29,6 +29,12 @@ The key function here is :ref:`mj_loadXML`. It invokes the built-in parser and c a valid mjModel, or NULL - in which case the user should check the error information in the user-provided string. The model and all files referenced in it can be loaded from disk or from a VFS when provided. +.. _mj_compile: + +Compile :ref:`mjSpec` to :ref:`mjModel`. A spec can be edited and compiled multiple times, returning a new +:ref:`mjModel` instance that takes the edits into account. +If compilation fails, :ref:`mj_compile` returns ``NULL``; the error can be read with :ref:`mjs_getError`. + .. _mj_recompile: Recompile spec to model, preserving the state. Like :ref:`mj_compile`, this function compiles an :ref:`mjSpec` to an @@ -38,10 +44,8 @@ reallocate existing :ref:`mjModel` and :ref:`mjData` instances in-place. Second, newly added or removed degrees of freedom. This allows the user to continue simulation with the same model and data struct pointers while editing the model programmatically. -.. admonition:: Incomplete implementation - :class: attention - - This function is currently incomplete, preserving only ``mjData.qpos`` and ``mjData.qvel``. +:ref:`mj_recompile` returns 0 if compilation succeed. In the case of failure, the given :ref:`mjModel` and :ref:`mjData` +instances will be deleted; as in :ref:`mj_compile`, the compilation error can be read with :ref:`mjs_getError`. .. _Mainsimulation: diff --git a/doc/includes/references.h b/doc/includes/references.h index a70bc244..4ecd4b74 100644 --- a/doc/includes/references.h +++ b/doc/includes/references.h @@ -3103,7 +3103,7 @@ mjModel* mj_loadXML(const char* filename, const mjVFS* vfs, char* error, int err mjSpec* mj_parseXML(const char* filename, const mjVFS* vfs, char* error, int error_sz); mjSpec* mj_parseXMLString(const char* xml, const mjVFS* vfs, char* error, int error_sz); mjModel* mj_compile(mjSpec* s, const mjVFS* vfs); -void mj_recompile(mjSpec* s, const mjVFS* vfs, mjModel* m, mjData* d); +int mj_recompile(mjSpec* s, const mjVFS* vfs, mjModel* m, mjData* d); int mj_saveLastXML(const char* filename, const mjModel* m, char* error, int error_sz); void mj_freeLastXML(void); void mj_copyBack(mjSpec* s, const mjModel* m); diff --git a/include/mujoco/mujoco.h b/include/mujoco/mujoco.h index 26014657..0d115c9f 100644 --- a/include/mujoco/mujoco.h +++ b/include/mujoco/mujoco.h @@ -106,8 +106,8 @@ MJAPI mjSpec* mj_parseXMLString(const char* xml, const mjVFS* vfs, char* error, // Compile spec to model. MJAPI mjModel* mj_compile(mjSpec* s, const mjVFS* vfs); -// Recompile spec to model, preserving the state. -MJAPI void mj_recompile(mjSpec* s, const mjVFS* vfs, mjModel* m, mjData* d); +// Recompile spec to model, preserving the state, return 0 on success. +MJAPI int mj_recompile(mjSpec* s, const mjVFS* vfs, mjModel* m, mjData* d); // Update XML data structures with info from low-level model, save as MJCF. // If error is not NULL, it must have size error_sz. diff --git a/introspect/functions.py b/introspect/functions.py index d787bf1c..90e744a0 100644 --- a/introspect/functions.py +++ b/introspect/functions.py @@ -251,7 +251,7 @@ FUNCTIONS: Mapping[str, FunctionDecl] = dict([ ('mj_recompile', FunctionDecl( name='mj_recompile', - return_type=ValueType(name='void'), + return_type=ValueType(name='int'), parameters=( FunctionParameterDecl( name='s', @@ -278,7 +278,7 @@ FUNCTIONS: Mapping[str, FunctionDecl] = dict([ ), ), ), - doc='Recompile spec to model, preserving the state.', + doc='Recompile spec to model, preserving the state, return 0 on success.', # pylint: disable=line-too-long )), ('mj_saveLastXML', FunctionDecl( diff --git a/python/mujoco/specs_test.py b/python/mujoco/specs_test.py index a860f443..c440d91d 100644 --- a/python/mujoco/specs_test.py +++ b/python/mujoco/specs_test.py @@ -352,5 +352,29 @@ class SpecsTest(absltest.TestCase): self.assertEqual(model.nplugin, 1) self.assertEqual(model.body_plugin[1], 0) + def test_recompile_error(self): + main_xml = """ + + + + + + + + """ + + spec = mujoco.MjSpec() + spec.from_string(main_xml) + model = spec.compile() + data = mujoco.MjData(model) + + spec.add_material().name = 'yellow' + spec.add_material().name = 'yellow' + + with self.assertRaisesRegex( + ValueError, "Error: repeated name 'yellow' in material" + ): + spec.recompile(model, data) + if __name__ == '__main__': absltest.main() diff --git a/python/mujoco/structs.cc b/python/mujoco/structs.cc index 66566fe7..20b6678a 100644 --- a/python/mujoco/structs.cc +++ b/python/mujoco/structs.cc @@ -456,7 +456,9 @@ py::tuple RecompileSpec(raw::MjSpec* spec, const MjModelWrapper& old_m, raw::MjModel* m = static_cast(mju_malloc(sizeof(mjModel))); m->buffer = nullptr; raw::MjData* d = mj_copyData(nullptr, old_m.get(), old_d.get()); - mj_recompile(spec, nullptr, m, d); + if (mj_recompile(spec, nullptr, m, d)) { + throw py::value_error(mjs_getError(spec)); + } py::object m_pyobj = py::cast((MjModelWrapper(m))); py::object d_pyobj = diff --git a/src/user/user_api.cc b/src/user/user_api.cc index f598db61..228299f1 100644 --- a/src/user/user_api.cc +++ b/src/user/user_api.cc @@ -90,8 +90,8 @@ mjModel* mj_compile(mjSpec* s, const mjVFS* vfs) { -// recompile spec into existing model and data while preserving the state -void mj_recompile(mjSpec* s, const mjVFS* vfs, mjModel* m, mjData* d) { +// recompile spec to model, preserving the state, return 0 on success +[[nodiscard]] int mj_recompile(mjSpec* s, const mjVFS* vfs, mjModel* m, mjData* d) { mjCModel* modelC = static_cast(s->element); std::string state_name = "state"; mjtNum time = 0; @@ -99,13 +99,19 @@ void mj_recompile(mjSpec* s, const mjVFS* vfs, mjModel* m, mjData* d) { time = d->time; modelC->SaveState(state_name, d->qpos, d->qvel, d->act, d->ctrl, d->mocap_pos, d->mocap_quat); } - modelC->Compile(vfs, &m); + if (!modelC->Compile(vfs, &m)) { + if (d) { + mj_deleteData(d); + } + return -1; + }; if (d) { modelC->MakeData(m, &d); modelC->RestoreState(state_name, m->qpos0, m->body_pos, m->body_quat, d->qpos, d->qvel, d->act, d->ctrl, d->mocap_pos, d->mocap_quat); d->time = time; } + return 0; } diff --git a/src/user/user_api.h b/src/user/user_api.h index c30abd53..82ce193a 100644 --- a/src/user/user_api.h +++ b/src/user/user_api.h @@ -42,8 +42,8 @@ MJAPI mjSpec* mj_makeSpec(void); // Compile spec to model. MJAPI mjModel* mj_compile(mjSpec* s, const mjVFS* vfs); -// Recompile spec to model preserving the current state. -MJAPI void mj_recompile(mjSpec* s, const mjVFS* vfs, mjModel* m, mjData* d); +// Recompile spec to model, preserving the state, return 0 on success. +MJAPI int mj_recompile(mjSpec* s, const mjVFS* vfs, mjModel* m, mjData* d); // Copy spec. MJAPI mjSpec* mj_copySpec(const mjSpec* s); diff --git a/test/user/user_api_test.cc b/test/user/user_api_test.cc index 346f179d..cc88aec9 100644 --- a/test/user/user_api_test.cc +++ b/test/user/user_api_test.cc @@ -140,6 +140,27 @@ TEST_F(PluginTest, ActivatePlugin) { mj_deleteModel(model); } +TEST_F(MujocoTest, RecompileFails) { + mjSpec* spec = mj_makeSpec(); + mjsBody* body = mjs_addBody(mjs_findBody(spec, "world"), 0); + mjsGeom* geom = mjs_addGeom(body, 0); + geom->type = mjGEOM_SPHERE; + geom->size[0] = 1; + + mjModel* model = mj_compile(spec, 0); + mjData* data = mj_makeData(model); + + mjsMaterial* mat1 = mjs_addMaterial(spec, 0); + mjsMaterial* mat2 = mjs_addMaterial(spec, 0); + mjs_setString(mat1->name, "yellow"); + mjs_setString(mat2->name, "yellow"); + + EXPECT_EQ(mj_recompile(spec, 0, model, data), -1); + EXPECT_STREQ(mjs_getError(spec), "Error: repeated name 'yellow' in material"); + + mj_deleteSpec(spec); +} + // ------------------- test recompilation multiple files ----------------------- TEST_F(PluginTest, RecompileCompare) { mjtNum tol = 0;