Check that there are no pending keyframes while writing a spec.

This can happen when a body is detached without compiling or recompiling.

Also added error catching when trying to delete a body instead of detaching it.

Fixes #2327

PiperOrigin-RevId: 712526730
Change-Id: I4b48df83120fca12d475c85b3a54893841449653

doc/APIreference/functions.rst

@@ -3918,7 +3918,7 @@ Add frame to body.
 
 .. mujoco-include:: mjs_delete
 
-Delete object corresponding to the given element.
+Delete object corresponding to the given element, return 0 on success.
 
 .. _AddNonTreeElements:
 

doc/includes/references.h

@@ -3578,7 +3578,7 @@ mjsGeom* mjs_addGeom(mjsBody* body, const mjsDefault* def);
 mjsCamera* mjs_addCamera(mjsBody* body, const mjsDefault* def);
 mjsLight* mjs_addLight(mjsBody* body, const mjsDefault* def);
 mjsFrame* mjs_addFrame(mjsBody* body, mjsFrame* parentframe);
-void mjs_delete(mjsElement* element);
+int mjs_delete(mjsElement* element);
 mjsActuator* mjs_addActuator(mjSpec* s, const mjsDefault* def);
 mjsSensor* mjs_addSensor(mjSpec* s);
 mjsFlex* mjs_addFlex(mjSpec* s);

include/mujoco/mujoco.h

@@ -121,7 +121,7 @@ MJAPI void mj_freeLastXML(void);
 // If length of the output buffer is too small, returns the required size.
 MJAPI int mj_saveXMLString(const mjSpec* s, char* xml, int xml_sz, char* error, int error_sz);
 
-// Save spec to XML file, return 1 on success, 0 otherwise.
+// Save spec to XML file, return 0 on success, -1 otherwise.
 MJAPI int mj_saveXML(const mjSpec* s, const char* filename, char* error, int error_sz);
 
 
@@ -1453,8 +1453,8 @@ MJAPI mjsLight* mjs_addLight(mjsBody* body, const mjsDefault* def);
 // Add frame to body.
 MJAPI mjsFrame* mjs_addFrame(mjsBody* body, mjsFrame* parentframe);
 
-// Delete object corresponding to the given element.
+// Delete object corresponding to the given element, return 0 on success.
-MJAPI void mjs_delete(mjsElement* element);
+MJAPI int mjs_delete(mjsElement* element);
 
 
 //---------------------------------- Non-tree elements ---------------------------------------------

introspect/functions.py

@@ -379,7 +379,7 @@ FUNCTIONS: Mapping[str, FunctionDecl] = dict([
                  type=ValueType(name='int'),
              ),
          ),
-         doc='Save spec to XML file, return 1 on success, 0 otherwise.',
+         doc='Save spec to XML file, return 0 on success, -1 otherwise.',
      )),
     ('mj_step',
      FunctionDecl(
@@ -9277,7 +9277,7 @@ FUNCTIONS: Mapping[str, FunctionDecl] = dict([
     ('mjs_delete',
      FunctionDecl(
          name='mjs_delete',
-         return_type=ValueType(name='void'),
+         return_type=ValueType(name='int'),
          parameters=(
              FunctionParameterDecl(
                  name='element',
@@ -9286,7 +9286,7 @@ FUNCTIONS: Mapping[str, FunctionDecl] = dict([
                  ),
              ),
          ),
-         doc='Delete object corresponding to the given element.',
+         doc='Delete object corresponding to the given element, return 0 on success.',  # pylint: disable=line-too-long
      )),
     ('mjs_addActuator',
      FunctionDecl(

src/user/user_api.cc

@@ -254,10 +254,17 @@ int mjs_activatePlugin(mjSpec* s, const char* name) {
 
 
 
-// delete object, it will call the appropriate destructor since ~mjCBase is virtual
+// delete object, return 0 if success
-void mjs_delete(mjsElement* element) {
+int mjs_delete(mjsElement* element) {
   mjCBase* object = static_cast<mjCBase*>(element);
-  object->model->DeleteElement(element);
+  try {
+    // it will call the appropriate destructor since ~mjCBase is virtual
+    object->model->DeleteElement(element);
+    return 0;
+  } catch (mjCError& e) {
+    object->model->SetError(e);
+    return -1;
+  }
 }
 
 

src/user/user_api.h

@@ -108,8 +108,8 @@ MJAPI mjsLight* mjs_addLight(mjsBody* body, const mjsDefault* def);
 // Add frame to body.
 MJAPI mjsFrame* mjs_addFrame(mjsBody* body, mjsFrame* parentframe);
 
-// Delete object corresponding to the given element.
+// Delete object corresponding to the given element, return 0 on success.
-MJAPI void mjs_delete(mjsElement* element);
+MJAPI int mjs_delete(mjsElement* element);
 
 
 //---------------------------------- Add non-tree elements -----------------------------------------

src/user/user_model.cc

@@ -530,7 +530,6 @@ mjCModel& mjCModel::operator-=(const mjCBody& subtree) {
     ResetTreeLists();
   }
 
-  PointToLocal();
   return *this;
 }
 

src/xml/xml_native_writer.cc

@@ -858,7 +858,7 @@ mjXWriter::mjXWriter(void) {
 // cast model
 void mjXWriter::SetModel(const mjSpec* _spec, const mjModel* m) {
   if (_spec) {
-    model = (mjCModel*)_spec->element;
+    model = static_cast<mjCModel*>(_spec->element);
   }
   if (m) {
     model->CopyBack(m);
@@ -2207,6 +2207,10 @@ void mjXWriter::Keyframe(XMLElement* root) {
   // create section
   XMLElement* section = InsertEnd(root, "keyframe");
 
+  if (!model->key_pending_.empty()) {
+    throw mjXError(0, "Model has pending keyframes. It must be (re)compiled before writing XML.");
+  }
+
   // write all keyframes
   for (int i=0; i<model->nkey; i++) {
     XMLElement* elem = InsertEnd(section, "key");

test/user/user_api_test.cc

@@ -1222,9 +1222,21 @@ void TestDetachBody(bool compile) {
   mjsBody* body = mjs_findBody(child, "body");
   EXPECT_THAT(body, NotNull());
 
+  // get an error if trying to delete the body
+  EXPECT_EQ(mjs_delete(body->element), -1);
+  EXPECT_THAT(mjs_getError(child), HasSubstr("use detach instead"));
+
   // detach subtree
   EXPECT_THAT(mjs_detachBody(child, body), 0);
 
+  // try saving to XML before compiling again
+  std::array<char, 1024> e;
+  std::array<char, 1024> s;
+  EXPECT_EQ(mj_saveXMLString(child, s.data(), 1024, e.data(), 1024), -1);
+  EXPECT_THAT(e.data(), compile
+                  ? HasSubstr("Model has pending keyframes")
+                  : HasSubstr("Only compiled model can be written"));
+
   // compile new model
   mjModel* m_detached = mj_compile(child, 0);
   EXPECT_THAT(m_detached, NotNull());