Raise warning when attaching a spec with pending keyframes to a different spec.

PiperOrigin-RevId: 685677622
Change-Id: Iba522c523ebd2afe64d6ab28c6b6b4b3cd399cf8

src/user/user_model.cc

@@ -463,7 +463,7 @@ mjCModel& mjCModel::operator-=(const mjCBody& subtree) {
   }
 
   // all keyframes are now pending and they will be resized
-  StoreKeyframes();
+  StoreKeyframes(this);
   DeleteAll(keys_);
 
   // remove body from tree
@@ -3142,9 +3142,15 @@ template void mjCModel::RestoreState<mjtNum>(
 
 
 // resolve keyframe references
-void mjCModel::StoreKeyframes() {
+void mjCModel::StoreKeyframes(mjCModel* dest) {
   bool resetlists = false;
 
+  if (this != dest && !key_pending_.empty()) {
+    mju_warning(
+        "Child model has pending keyframes. They will not be namespaced correctly. "
+        "To prevent this, compile the child model before attaching it again.");
+  }
+
   // create tree lists if they are empty, occurs if an uncompiled model is attached
   if (bodies_.size() == 1 && geoms_.empty() && sites_.empty() && joints_.empty() &&
       cameras_.empty() && lights_.empty() && frames_.empty()) {
@@ -3172,7 +3178,7 @@ void mjCModel::StoreKeyframes() {
     info.ctrl = !key->spec_ctrl_.empty();
     info.mpos = !key->spec_mpos_.empty();
     info.mquat = !key->spec_mquat_.empty();
-    key_pending_.push_back(info);
+    dest->key_pending_.push_back(info);
     ResizeKeyframe(key, qpos0.data(), body_pos0.data(), body_quat0.data());
     SaveState(info.name, key->spec_qpos_.data(), key->spec_qvel_.data(),
               key->spec_act_.data(), key->spec_ctrl_.data(),

src/user/user_model.h

@@ -297,7 +297,7 @@ class mjCModel : public mjCModel_, private mjSpec {
   void MakeData(const mjModel* m, mjData** dest);
 
   // resolve keyframe references
-  void StoreKeyframes();
+  void StoreKeyframes(mjCModel* dest);
 
   // map from default class name to default class pointer
   std::unordered_map<std::string, mjCDef*> def_map;

src/user/user_objects.cc

@@ -848,7 +848,7 @@ mjCBody& mjCBody::operator+=(const mjCFrame& other) {
   mjCBody* subtree = other.body;
   other.model->prefix = other.prefix;
   other.model->suffix = other.suffix;
-  other.model->StoreKeyframes();
+  other.model->StoreKeyframes(model);
 
   // attach defaults
   if (other.model != model) {
@@ -1808,7 +1808,7 @@ mjCFrame& mjCFrame::operator=(const mjCFrame& other) {
 mjCFrame& mjCFrame::operator+=(const mjCBody& other) {
   other.model->prefix = other.prefix;
   other.model->suffix = other.suffix;
-  other.model->StoreKeyframes();
+  other.model->StoreKeyframes(model);
   other.model->prefix = "";
   other.model->suffix = "";
 

test/user/user_api_test.cc

@@ -20,12 +20,12 @@
 #include <filesystem>
 #include <memory>
 #include <string>
-#include <utility>
 #include <vector>
 
 #include <gmock/gmock.h>
 #include <gtest/gtest.h>
 #include <absl/strings/match.h>
+#include "src/cc/array_safety.h"
 #include <mujoco/mujoco.h>
 #include <mujoco/mjspec.h>
 #include "src/xml/xml_api.h"
@@ -1534,7 +1534,7 @@ TEST_F(MujocoTest, InitTexture) {
   mj_deleteSpec(spec);
 }
 
-TEST_F(MujocoTest, AttachNestedKeyframe) {
+void AttachNestedKeyframe(bool compile) {
   static constexpr char parent_xml[] = R"(
   <mujoco>
     <worldbody>
@@ -1599,6 +1599,32 @@ TEST_F(MujocoTest, AttachNestedKeyframe) {
     </keyframe>
   </mujoco>)";
 
+  static constexpr char expected_xml_uncompiled[] = R"(
+  <mujoco>
+    <worldbody>
+      <body name="body">
+        <joint name="joint" type="slide"/>
+        <geom size=".1"/>
+      </body>
+      <frame name="frame">
+        <body name="child-body">
+          <joint name="child-joint" type="slide"/>
+          <geom size=".2"/>
+          <frame name="child-frame">
+            <body name="child-gchild-body">
+              <joint name="child-gchild-joint" type="slide"/>
+              <geom size=".3"/>
+            </body>
+          </frame>
+        </body>
+      </frame>
+    </worldbody>
+    <keyframe>
+      <key name="child-key2" time="2" qpos="0 2 0"/>
+      <key name="gchild-key1" time="1" qpos="0 0 1"/>
+    </keyframe>
+  </mujoco>)";
+
   std::array<char, 1000> er;
   mjSpec* parent = mj_parseXMLString(parent_xml, 0, er.data(), er.size());
   EXPECT_THAT(parent, NotNull()) << er.data();
@@ -1612,17 +1638,28 @@ TEST_F(MujocoTest, AttachNestedKeyframe) {
                  mjs_findBody(gchild, "body"), "gchild-", "");
 
   // compile required before further attachment
-  mjModel* m_child = mj_compile(child, 0);
+  mjModel* m_child = compile ? mj_compile(child, 0) : nullptr;
+
+  // check warning is issued, empty for a compiled model
+  static char warning[1024];
+  warning[0] = '\0';
+  mju_user_warning = [](const char* msg) {
+      util::strcpy_arr(warning, msg);
+  };
 
   // attach child to parent
   mjs_attachBody(mjs_findFrame(parent, "frame"),
                  mjs_findBody(child, "body"), "child-", "");
 
+  EXPECT_THAT(warning, HasSubstr(compile ? "" : "model has pending keyframes"));
+
+  // compare models
   mjtNum tol = 0;
   std::string field = "";
   mjModel* m_attached = mj_compile(parent, 0);
   EXPECT_THAT(m_attached, NotNull());
-  mjModel* m_expected = LoadModelFromString(expected_xml, er.data(), er.size());
+  mjModel* m_expected = LoadModelFromString(
+      compile ? expected_xml : expected_xml_uncompiled, er.data(), er.size());
   EXPECT_THAT(m_expected, NotNull()) << er.data();
   EXPECT_LE(CompareModel(m_attached, m_expected, field), tol)
             << "Expected and attached models are different!\n"
@@ -1636,6 +1673,11 @@ TEST_F(MujocoTest, AttachNestedKeyframe) {
   mj_deleteModel(m_child);
 }
 
+TEST_F(MujocoTest, TestAttachNestedKeyframe) {
+  AttachNestedKeyframe(/*compile=*/true);
+  AttachNestedKeyframe(/*compile=*/false);
+}
+
 TEST_F(MujocoTest, RepeatedAttachKeyframe) {
   static constexpr char xml_1[] = R"(
     <mujoco model="MuJoCo Model">