From 42f6216d599c60bd2d67ee84bd649e138329f77f Mon Sep 17 00:00:00 2001 From: Russ Tedrake Date: Sat, 28 Sep 2024 07:05:53 -0400 Subject: [PATCH] Avoid duplicate warnings in multibody XML parsers The multibody parsers, in particular, had a habit of being very noisy, since if an unsupported element or attribute is used once it is likely used many times. This actually made it hard to visually inspect the output to see the unique set of unsupported tags. This change significantly improves the output for examples like in #20444. --- multibody/parsing/detail_tinyxml2_diagnostic.cc | 5 ++++- multibody/parsing/detail_tinyxml2_diagnostic.h | 3 +++ .../parsing/test/detail_mujoco_parser_test.cc | 4 ---- .../test/detail_tinyxml2_diagnostic_test.cc | 17 +++++++++++++---- .../parsing/test/detail_urdf_parser_test.cc | 6 ++---- 5 files changed, 22 insertions(+), 13 deletions(-) diff --git a/multibody/parsing/detail_tinyxml2_diagnostic.cc b/multibody/parsing/detail_tinyxml2_diagnostic.cc index 708e1d4b9391..167bf64c919f 100644 --- a/multibody/parsing/detail_tinyxml2_diagnostic.cc +++ b/multibody/parsing/detail_tinyxml2_diagnostic.cc @@ -36,7 +36,10 @@ DiagnosticDetail TinyXml2Diagnostic::MakeDetail( void TinyXml2Diagnostic::Warning( const XMLNode& location, const std::string& message) const { - diagnostic_->Warning(MakeDetail(location, message)); + // Avoid warnings with exactly the same message string. + if (warnings_.insert(message).second) { + diagnostic_->Warning(MakeDetail(location, message)); + } } void TinyXml2Diagnostic::Error( diff --git a/multibody/parsing/detail_tinyxml2_diagnostic.h b/multibody/parsing/detail_tinyxml2_diagnostic.h index 37d4b40c883a..083088c2bcf0 100644 --- a/multibody/parsing/detail_tinyxml2_diagnostic.h +++ b/multibody/parsing/detail_tinyxml2_diagnostic.h @@ -1,6 +1,7 @@ #pragma once #include +#include #include @@ -56,6 +57,8 @@ class TinyXml2Diagnostic { const drake::internal::DiagnosticPolicy* diagnostic_{}; const DataSource* data_source_{}; const std::string file_extension_; + // Keep a history to avoid duplicate warnings. + mutable std::unordered_set warnings_; }; } // namespace internal diff --git a/multibody/parsing/test/detail_mujoco_parser_test.cc b/multibody/parsing/test/detail_mujoco_parser_test.cc index f96f6d3485b2..952266f6d29e 100644 --- a/multibody/parsing/test/detail_mujoco_parser_test.cc +++ b/multibody/parsing/test/detail_mujoco_parser_test.cc @@ -1735,16 +1735,12 @@ TEST_F(MujocoParserTest, ContactWarnings) { AddModelFromString(xml, "test"); - EXPECT_THAT(TakeWarning(), MatchesRegex( - ".*pair.*not have.*geom1.*geom2.*ignored.*")); EXPECT_THAT(TakeWarning(), MatchesRegex( ".*pair.*not have.*geom1.*geom2.*ignored.*")); EXPECT_THAT(TakeWarning(), MatchesRegex(".*pair.*unknown geom1.*ignored.*")); EXPECT_THAT(TakeWarning(), MatchesRegex(".*pair.*unknown geom2.*ignored.*")); EXPECT_THAT(TakeWarning(), MatchesRegex( ".*exclude.*not have.*body1.*body2.*ignored.*")); - EXPECT_THAT(TakeWarning(), MatchesRegex( - ".*exclude.*not have.*body1.*body2.*ignored.*")); } // TODO(rpoyner-tri) consider how to convert these to diagnostics. diff --git a/multibody/parsing/test/detail_tinyxml2_diagnostic_test.cc b/multibody/parsing/test/detail_tinyxml2_diagnostic_test.cc index e6ee913799fe..b66579d244d8 100644 --- a/multibody/parsing/test/detail_tinyxml2_diagnostic_test.cc +++ b/multibody/parsing/test/detail_tinyxml2_diagnostic_test.cc @@ -28,6 +28,7 @@ class TinyXml2DiagnosticTest : public test::DiagnosticPolicyTestBase { + )"""; } @@ -121,19 +122,27 @@ TEST_F(TinyXml2DiagnosticContentsTest, PolicyForNode) { } TEST_F(TinyXml2DiagnosticContentsTest, Unsuppported) { - diagnostic_.WarnUnsupportedElement( - GetFirstChildNamed("warning"), "unsupported_element"); + const XMLElement& warning_node = GetFirstChildNamed("warning"); + diagnostic_.WarnUnsupportedElement(warning_node, "unsupported_element"); EXPECT_EQ(TakeWarning(), ".stuff:6: warning: The tag 'unsupported_element'" " found as a child of 'warning' is currently unsupported and will" " be ignored."); - diagnostic_.WarnUnsupportedAttribute( - GetFirstChildNamed("warning"), "unsupported_attribute"); + diagnostic_.WarnUnsupportedAttribute(warning_node, "unsupported_attribute"); EXPECT_EQ(TakeWarning(), ".stuff:5: warning: The attribute" " 'unsupported_attribute' found in a 'warning' tag is currently" " unsupported and will be ignored."); + + // Repeat the warning for the second instance of unsupported_element. This + // should *not* produce an additional warning, since it would produce a + // message that was identical except for the location in the file. + const XMLElement* repeated_warning_node = + warning_node.NextSiblingElement("warning"); + ASSERT_NE(repeated_warning_node, nullptr); + diagnostic_.WarnUnsupportedAttribute(*repeated_warning_node, + "unsupported_attribute"); } } // namespace diff --git a/multibody/parsing/test/detail_urdf_parser_test.cc b/multibody/parsing/test/detail_urdf_parser_test.cc index 8059071cf7fd..9f62f8cbf16b 100644 --- a/multibody/parsing/test/detail_urdf_parser_test.cc +++ b/multibody/parsing/test/detail_urdf_parser_test.cc @@ -764,9 +764,7 @@ TEST_F(UrdfParserTest, TestAtlasMinimalContact) { const std::string full_name = FindRunfile( "drake_models/atlas/atlas_minimal_contact.urdf").abspath; AddModelFromUrdfFile(full_name, ""); - for (int k = 0; k < 30; k++) { - EXPECT_THAT(TakeWarning(), MatchesRegex(".*safety_controller.*ignored.*")); - } + EXPECT_THAT(TakeWarning(), MatchesRegex(".*safety_controller.*ignored.*")); EXPECT_THAT(TakeWarning(), MatchesRegex(".*attached to a fixed joint.*")); plant_.Finalize(); @@ -798,7 +796,7 @@ TEST_F(UrdfParserTest, TestRegisteredSceneGraph) { // Test that registration with scene graph results in visual geometries. AddModelFromUrdfFile(full_name, ""); // Mostly ignore warnings here; they are tested in detail elsewhere. - EXPECT_GT(warning_records_.size(), 30); + EXPECT_GT(warning_records_.size(), 1); warning_records_.clear(); plant_.Finalize(); EXPECT_NE(plant_.num_visual_geometries(), 0);