From 5dc26cf4a0ce0d54618391ea1587b34ea11f47af Mon Sep 17 00:00:00 2001 From: Yuval Tassa Date: Sun, 18 Sep 2022 11:09:23 -0700 Subject: [PATCH] Fix bugs in actuator parsing. - Adhesion actuator defaults were not parsed, now fixed. - Both damper and adhesion actuators were requiring `ctrlrange` at parse time, which prevented this attribute from being inherited from defaults, now fixed. - Added tests for all 3 cases. PiperOrigin-RevId: 475145255 Change-Id: I8dd6196e20734136d990f522893ebd4d156cd7a8 --- src/xml/xml_native_reader.cc | 9 +-- test/xml/xml_native_reader_test.cc | 103 +++++++++++++++++++++++++---- 2 files changed, 95 insertions(+), 17 deletions(-) diff --git a/src/xml/xml_native_reader.cc b/src/xml/xml_native_reader.cc index ce48762c..4ec15cbe 100644 --- a/src/xml/xml_native_reader.cc +++ b/src/xml/xml_native_reader.cc @@ -1598,7 +1598,7 @@ void mjXReader::OneActuator(XMLElement* elem, mjCActuator* pact) { pact->gainprm[2] = -pact->gainprm[2]; // require nonnegative range - ReadAttr(elem, "ctrlrange", 2, pact->ctrlrange, text, true); + ReadAttr(elem, "ctrlrange", 2, pact->ctrlrange, text); if (pact->ctrlrange[0]<0 || pact->ctrlrange[1]<0) { throw mjXError(elem, "damper control range cannot be negative"); } @@ -1676,7 +1676,7 @@ void mjXReader::OneActuator(XMLElement* elem, mjCActuator* pact) { throw mjXError(elem, "adhesion gain cannot be negative"); // require nonnegative range - ReadAttr(elem, "ctrlrange", 2, pact->ctrlrange, text, true); + ReadAttr(elem, "ctrlrange", 2, pact->ctrlrange, text); if (pact->ctrlrange[0]<0 || pact->ctrlrange[1]<0) { throw mjXError(elem, "adhesion control range cannot be negative"); } @@ -1947,7 +1947,7 @@ void mjXReader::Default(XMLElement* section, int parentid) { // read tendon else if (name=="tendon") OneTendon(elem, &def->tendon); - // read actuator: general, motor, position, velocity, cylinder + // read actuator else if (name=="general" || name=="motor" || name=="position" || @@ -1955,7 +1955,8 @@ void mjXReader::Default(XMLElement* section, int parentid) { name=="damper" || name=="intvelocity" || name=="cylinder" || - name=="muscle") { + name=="muscle" || + name=="adhesion") { OneActuator(elem, &def->actuator); } diff --git a/test/xml/xml_native_reader_test.cc b/test/xml/xml_native_reader_test.cc index 331e514e..b28fceee 100644 --- a/test/xml/xml_native_reader_test.cc +++ b/test/xml/xml_native_reader_test.cc @@ -365,7 +365,7 @@ TEST_F(UserDataTest, RequiresControlRange) { - + @@ -376,7 +376,7 @@ TEST_F(UserDataTest, RequiresControlRange) { std::array error; mjModel* model = LoadModelFromString(xml, error.data(), error.size()); ASSERT_THAT(model, IsNull()); - EXPECT_THAT(error.data(), HasSubstr("required attribute missing: 'ctrlrange'")); + EXPECT_THAT(error.data(), HasSubstr("invalid control range")); } TEST_F(UserDataTest, PositiveControlRange) { @@ -553,11 +553,11 @@ TEST_F(ActuatorTest, ReadsByte) { mj_deleteModel(model); } -// ------------- test intvelocity parsing --------------------------------------- +// ---------------- test actuator parsing -------------------------------------- -using IntegratedVelocityTest = MujocoTest; +using ActuatorParseTest = MujocoTest; -TEST_F(IntegratedVelocityTest, CheckEquivalence) { +TEST_F(ActuatorParseTest, IntvelocityCheckEquivalence) { static constexpr char xml[] = R"( @@ -568,14 +568,14 @@ TEST_F(IntegratedVelocityTest, CheckEquivalence) { - + )"; std::array error; mjModel* model = LoadModelFromString(xml, error.data(), error.size()); - ASSERT_THAT(model, testing::NotNull()); + ASSERT_THAT(model, NotNull()); // same actlimited EXPECT_EQ(model->actuator_actlimited[0], 1); EXPECT_EQ(model->actuator_actlimited[1], 1); @@ -606,7 +606,7 @@ TEST_F(IntegratedVelocityTest, CheckEquivalence) { mj_deleteModel(model); } -TEST_F(IntegratedVelocityTest, CheckDefaultsIfNotSpecified) { +TEST_F(ActuatorParseTest, IntvelocityCheckDefaultsIfNotSpecified) { static constexpr char xml[] = R"( @@ -622,7 +622,7 @@ TEST_F(IntegratedVelocityTest, CheckDefaultsIfNotSpecified) { )"; std::array error; mjModel* model = LoadModelFromString(xml, error.data(), error.size()); - ASSERT_THAT(model, testing::NotNull()); + ASSERT_THAT(model, NotNull()); // check that by default kp = 1 EXPECT_DOUBLE_EQ(model->actuator_gainprm[0], 1.0); // check that biasprm is (0, -1, 0) @@ -632,7 +632,7 @@ TEST_F(IntegratedVelocityTest, CheckDefaultsIfNotSpecified) { mj_deleteModel(model); } -TEST_F(IntegratedVelocityTest, NoActrangeThrowsError) { +TEST_F(ActuatorParseTest, IntvelocityNoActrangeThrowsError) { static constexpr char xml[] = R"( @@ -652,7 +652,7 @@ TEST_F(IntegratedVelocityTest, NoActrangeThrowsError) { EXPECT_THAT(error.data(), HasSubstr("invalid activation range for actuator")); } -TEST_F(IntegratedVelocityTest, DefaultsPropagate) { +TEST_F(ActuatorParseTest, IntvelocityDefaultsPropagate) { static constexpr char xml[] = R"( @@ -676,7 +676,7 @@ TEST_F(IntegratedVelocityTest, DefaultsPropagate) { )"; std::array error; mjModel* model = LoadModelFromString(xml, error.data(), error.size()); - ASSERT_THAT(model, testing::NotNull()); + ASSERT_THAT(model, NotNull()); EXPECT_DOUBLE_EQ(model->actuator_gainprm[0], 5); EXPECT_DOUBLE_EQ(model->actuator_gainprm[mjNGAIN], 1); EXPECT_DOUBLE_EQ(model->actuator_actrange[0 + 0], 0); @@ -686,5 +686,82 @@ TEST_F(IntegratedVelocityTest, DefaultsPropagate) { mj_deleteModel(model); } + +// ------------- test adhesion parsing ----------------------------------------- + +TEST_F(ActuatorParseTest, AdhesionDefaultsPropagate) { + static constexpr char xml[] = R"( + + + + + + + + + + + + + + )"; + std::array error; + mjModel* model = LoadModelFromString(xml, error.data(), error.size()); + ASSERT_THAT(model, NotNull()); + EXPECT_EQ(model->actuator_ctrlrange[0], 0); + EXPECT_EQ(model->actuator_ctrlrange[1], 3); + mj_deleteModel(model); +} + +TEST_F(ActuatorParseTest, ErrorBadAdhesionDefaults) { + static constexpr char xml[] = R"( + + + + + + + + + + + + + + )"; + std::array error; + mjModel* model = LoadModelFromString(xml, error.data(), error.size()); + ASSERT_THAT(model, IsNull()); + EXPECT_THAT(error.data(), + HasSubstr("adhesion control range cannot be negative")); +} + +// make sure range requirement is not enforced at parse time +TEST_F(ActuatorParseTest, DampersDontRequireRange) { + static constexpr char xml[] = R"( + + + + + + + + + + + + + + + )"; + std::array error; + mjModel* model = LoadModelFromString(xml, error.data(), error.size()); + ASSERT_THAT(model, NotNull()); + EXPECT_EQ(model->actuator_ctrlrange[0], 0); + EXPECT_EQ(model->actuator_ctrlrange[1], 2); + mj_deleteModel(model); +} + + } // namespace } // namespace mujoco