From b22919543ddf096377bce27e4a638b779478ed54 Mon Sep 17 00:00:00 2001 From: Yuval Tassa Date: Sat, 9 Dec 2023 16:05:43 -0800 Subject: [PATCH] Fix bugs related to saving of values. Before this change: - User-modified values of `mjModel.stat.{meanmass, meaninertia}` would not be saved to XML. - Non-user-modified values of `mjModel.stat.{meansize, extent, center}` would always get saved to XML. After this change values are saved to XML only if provided by the user, either in the XML or by changing values in `mjModel` before saving. PiperOrigin-RevId: 589473351 Change-Id: I4dcf4892ef61c35edf424226e731232d861bad8d --- src/user/user_model.cc | 31 ++++++++++++++++++++++++++---- src/user/user_model.h | 10 +++++++++- test/xml/xml_native_writer_test.cc | 28 ++++++++++++++++++++++++++- 3 files changed, 63 insertions(+), 6 deletions(-) diff --git a/src/user/user_model.cc b/src/user/user_model.cc index 4645644e..c1c0300e 100644 --- a/src/user/user_model.cc +++ b/src/user/user_model.cc @@ -114,6 +114,16 @@ mjCModel::mjCModel() { center[0] = mjNAN; center[1] = center[2] = 0; + //------------------------ auto-computed statistics +#ifndef MEMORY_SANITIZER + // initializing as best practice, but want MSAN to catch unintialized use + meaninertia_auto = 0; + meanmass_auto = 0; + meansize_auto = 0; + extent_auto = 0; + center_auto[0] = center_auto[1] = center_auto[2] = 0; +#endif + //------------------------ engine data modelname = "MuJoCo Model"; mj_defaultOption(&option); @@ -3157,6 +3167,13 @@ void mjCModel::TryCompile(mjModel*& m, mjData*& d, const mjVFS* vfs) { // actuator lengthrange computation LengthRange(m, d); + // save automatically-computed statistics, to disambiguate when saving + extent_auto = m->stat.extent; + meaninertia_auto = m->stat.meaninertia; + meanmass_auto = m->stat.meanmass; + meansize_auto = m->stat.meansize; + copyvec(center_auto, m->stat.center, 3); + // override model statistics if defined by user if (mjuu_defined(extent)) m->stat.extent = (mjtNum)extent; if (mjuu_defined(meaninertia)) m->stat.meaninertia = (mjtNum)meaninertia; @@ -3233,10 +3250,16 @@ bool mjCModel::CopyBack(const mjModel* m) { option = m->opt; visual = m->vis; - // runtime-modifiable members of mjStatistic - meansize = m->stat.meansize; - extent = m->stat.extent; - mju_copy3(center, m->stat.center); + // runtime-modifiable members of mjStatistic, if different from computed values + if (m->stat.meaninertia != meaninertia_auto) meaninertia = m->stat.meaninertia; + if (m->stat.meanmass != meanmass_auto) meanmass = m->stat.meanmass; + if (m->stat.meansize != meansize_auto) meansize = m->stat.meansize; + if (m->stat.extent != extent_auto) extent = m->stat.extent; + if (m->stat.center[0] != center_auto[0] || + m->stat.center[1] != center_auto[1] || + m->stat.center[2] != center_auto[2]) { + mju_copy3(center, m->stat.center); + } // qpos0, qpos_spring for (int i=0; i cameras; // list of cameras std::vector lights; // list of lights + //------------------------ internal variables + + // statistics, as computed by mj_setConst + double meaninertia_auto; // mean diagonal inertia, as computed by mj_setConst + double meanmass_auto; // mean body mass, as computed by mj_setConst + double meansize_auto; // mean body size, as computed by mj_setConst + double extent_auto; // spatial extent, as computed by mj_setConst + double center_auto[3]; // center of model, as computed by mj_setConst + // map from object names to ids mjListKeyMap ids; - //------------------------ internal variables bool hasImplicitPluginElem; // already encountered an implicit plugin sensor/actuator bool compiled; // already compiled flag (cannot be compiled again) mjCError errInfo; // last error info diff --git a/test/xml/xml_native_writer_test.cc b/test/xml/xml_native_writer_test.cc index 52b21923..f5989279 100644 --- a/test/xml/xml_native_writer_test.cc +++ b/test/xml/xml_native_writer_test.cc @@ -1135,7 +1135,7 @@ using DecompilerTest = MujocoTest; TEST_F(DecompilerTest, SavesStatitics) { static constexpr char xml[] = R"( - + )"; mjModel* model = LoadModelFromString(xml); @@ -1145,10 +1145,36 @@ TEST_F(DecompilerTest, SavesStatitics) { model->stat.center[0] = 9; model->stat.center[1] = 10; model->stat.center[2] = 11; + model->stat.meanmass = 12; + model->stat.meaninertia = 13; std::string saved_xml = SaveAndReadXml(model); EXPECT_THAT(saved_xml, HasSubstr("meansize=\"7\"")); EXPECT_THAT(saved_xml, HasSubstr("extent=\"8\"")); EXPECT_THAT(saved_xml, HasSubstr("center=\"9 10 11\"")); + EXPECT_THAT(saved_xml, HasSubstr("meanmass=\"12\"")); + EXPECT_THAT(saved_xml, HasSubstr("meaninertia=\"13\"")); + mj_deleteModel(model); +} + +TEST_F(DecompilerTest, DoesntSaveInferredStatitics) { + static constexpr char xml[] = R"( + + + + + + + + )"; + mjModel* model = LoadModelFromString(xml); + ASSERT_THAT(model, NotNull()); + std::string saved_xml = SaveAndReadXml(model); + EXPECT_THAT(saved_xml, Not(HasSubstr("meansize"))); + EXPECT_THAT(saved_xml, Not(HasSubstr("meanmass"))); + EXPECT_THAT(saved_xml, Not(HasSubstr("meaninertia"))); + EXPECT_THAT(saved_xml, Not(HasSubstr("center"))); + EXPECT_THAT(saved_xml, Not(HasSubstr("extent"))); + EXPECT_THAT(saved_xml, Not(HasSubstr("statistic"))); mj_deleteModel(model); }