From 1e7c10988785edf1f45dce08366190f625dc5c00 Mon Sep 17 00:00:00 2001 From: Alessio Quaglino Date: Mon, 14 Oct 2024 05:53:12 -0700 Subject: [PATCH] Raise warning when attaching a spec with pending keyframes to a different spec. PiperOrigin-RevId: 685677622 Change-Id: Iba522c523ebd2afe64d6ab28c6b6b4b3cd399cf8 --- src/user/user_model.cc | 12 ++++++--- src/user/user_model.h | 2 +- src/user/user_objects.cc | 4 +-- test/user/user_api_test.cc | 50 +++++++++++++++++++++++++++++++++++--- 4 files changed, 58 insertions(+), 10 deletions(-) diff --git a/src/user/user_model.cc b/src/user/user_model.cc index f0e7e803..91ae83d5 100644 --- a/src/user/user_model.cc +++ b/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( // 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(), diff --git a/src/user/user_model.h b/src/user/user_model.h index d6c6d81a..a6e9462d 100644 --- a/src/user/user_model.h +++ b/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 def_map; diff --git a/src/user/user_objects.cc b/src/user/user_objects.cc index 8ff71507..4293fb02 100644 --- a/src/user/user_objects.cc +++ b/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 = ""; diff --git a/test/user/user_api_test.cc b/test/user/user_api_test.cc index 2679a926..bf5c6079 100644 --- a/test/user/user_api_test.cc +++ b/test/user/user_api_test.cc @@ -20,12 +20,12 @@ #include #include #include -#include #include #include #include #include +#include "src/cc/array_safety.h" #include #include #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"( @@ -1599,6 +1599,32 @@ TEST_F(MujocoTest, AttachNestedKeyframe) { )"; + static constexpr char expected_xml_uncompiled[] = R"( + + + + + + + + + + + + + + + + + + + + + + + + )"; + std::array 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"(