Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid duplicate warnings in multibody XML parsers #21966

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion multibody/parsing/detail_tinyxml2_diagnostic.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
3 changes: 3 additions & 0 deletions multibody/parsing/detail_tinyxml2_diagnostic.h
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#pragma once

#include <string>
#include <unordered_set>

#include <tinyxml2.h>

Expand Down Expand Up @@ -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<std::string> warnings_;
};

} // namespace internal
Expand Down
4 changes: 0 additions & 4 deletions multibody/parsing/test/detail_mujoco_parser_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
17 changes: 13 additions & 4 deletions multibody/parsing/test/detail_tinyxml2_diagnostic_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ class TinyXml2DiagnosticTest : public test::DiagnosticPolicyTestBase {
<warning unsupported_attribute='10'>
<unsupported_element/>
</warning>
<warning unsupported_attribute='20'/>
</stuff>
)""";
}
Expand Down Expand Up @@ -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(),
"<literal-string>.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(),
"<literal-string>.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
Expand Down
6 changes: 2 additions & 4 deletions multibody/parsing/test/detail_urdf_parser_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand Down Expand Up @@ -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);
Expand Down