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.

Sign up for GitHub

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

Remove empty //inertial/pose/@relative_to during 1_7->1.8 conversion #720

Merged
merged 7 commits into from
Sep 29, 2021
Merged

Remove empty //inertial/pose/@relative_to during 1_7->1.8 conversion #720

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
34d9587
Add empty //inertial/pose/@relative_to in test sdf
scpeters Sep 29, 2021
ca77b90
Add remove_empty functionality to Converter
scpeters Sep 29, 2021
4dd9a25
1_7->1.8: remove empty inertial pose relative_to
scpeters Sep 29, 2021
4df5968
Converter_TEST: add assertions
scpeters Sep 29, 2021
be159db
Converter: use strlen
scpeters Sep 29, 2021
e6eb293
Improve docs, rename argument to _removeOnlyEmpty
scpeters Sep 29, 2021
9588e0f
Remove empty elements if they have no attributes
scpeters Sep 29, 2021
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
6 changes: 6 additions & 0 deletions sdf/1.8/1_7.convert
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,10 @@
</convert>
</convert>

<convert descendant_name="inertial">
<convert name="pose">
<remove_empty attribute="relative_to" />
</convert>
</convert>

</convert> <!-- End SDF -->
23 changes: 18 additions & 5 deletions src/Converter.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,10 @@ void Converter::ConvertImpl(tinyxml2::XMLElement *_elem,
{
Remove(_elem, childElem);
}
else if (name == "remove_empty")
{
Remove(_elem, childElem, true);
}
else if (name == "unflatten")
{
Unflatten(_elem);
Expand Down Expand Up @@ -640,10 +644,11 @@ void Converter::Add(tinyxml2::XMLElement *_elem, tinyxml2::XMLElement *_addElem)

/////////////////////////////////////////////////
void Converter::Remove(tinyxml2::XMLElement *_elem,
tinyxml2::XMLElement *_removeElem)
tinyxml2::XMLElement *_removeElem,
bool _removeEmpty)
{
SDF_ASSERT(_elem != NULL, "SDF element is NULL");
SDF_ASSERT(_removeElem != NULL, "Move element is NULL");
SDF_ASSERT(_removeElem != NULL, "remove element is NULL");

const char *attributeName = _removeElem->Attribute("attribute");
const char *elementName = _removeElem->Attribute("element");
Expand All @@ -657,16 +662,24 @@ void Converter::Remove(tinyxml2::XMLElement *_elem,

if (attributeName)
{
_elem->DeleteAttribute(attributeName);
const char * attributeValue = _elem->Attribute(attributeName);
if (!_removeEmpty || (attributeValue && attributeValue[0] == '\0'))
azeey marked this conversation as resolved.
Show resolved Hide resolved
{
_elem->DeleteAttribute(attributeName);
}
}
else
{
tinyxml2::XMLElement *childElem = _elem->FirstChildElement(elementName);

while (childElem)
{
_elem->DeleteChild(childElem);
childElem = _elem->FirstChildElement(elementName);
auto nextSibling = childElem->NextSiblingElement(elementName);
if (!_removeEmpty || (childElem->NoChildren() && !childElem->GetText()))
{
_elem->DeleteChild(childElem);
}
childElem = nextSibling;
}
}
}
Expand Down
6 changes: 4 additions & 2 deletions src/Converter.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,9 +100,11 @@ namespace sdf

/// \brief Remove an element.
/// \param[in] _elem The element that has the _removeElem child.
/// \param[in] _removeElem The element to remove.
/// \param[in] _removeElem The metadata about what to remove.
/// \param[in] _removeEmpty If true, only remove empty nodes.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you document how this works for attributes and elements? From what I can understand, for attributes, it removes just the attribute if it's empty. For elements, it removes the element if it has an empty value and has no children, but it can have attributes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it be better to change the behavior for removing elements if they also have no attributes? We aren't yet using that functionality, so I couldn't decide. For now, I'll just document it as you suggest

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated documentation and renamed the parameter to _removeOnlyEmpty in e6eb293

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to chain these? By that I mean, would it be possible to remove the attributes first and then remove the element? If so, then I'd say let's make it so that it removes elements only if they have no attributes. This would make it possible to remove just the deprecated attributes first and remove the element if it doesn't contain anything else. For example:

<test_elem attr1="" attr2="some_val"/>
<test_elem attr1="" />

If we deprecate the use of //test_elem and know that //test_elem[@attr1=""] is the default, we can do

<remove_empty attribute='attr1'/>
<remove_empty element='test_elem'/>

and it will only delete the second //test_elem and we would be able to issue a warning about the first //test_elem

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, I'll rework the function logic accordingly

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done in 9588e0f

private: static void Remove(tinyxml2::XMLElement *_elem,
tinyxml2::XMLElement *_removeElem);
tinyxml2::XMLElement *_removeElem,
bool _removeEmpty = false);

/// \brief Unflatten an element (conversion from SDFormat <= 1.7 to 1.8)
/// \param[in] _elem The element to unflatten
Expand Down
171 changes: 169 additions & 2 deletions src/Converter_TEST.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,8 @@ std::string getRepeatedXmlString()
std::stringstream stream;
stream << "<elemA attrA='A'>"
<< " <elemB attrB='B'>"
<< " <elemC attrC='C'>"
<< " <elemC attrC='C' attrEmpty=''>"
<< " <elemD></elemD>"
<< " <elemD>D</elemD>"
<< " <elemD>D</elemD>"
<< " <elemD>D</elemD>"
Expand Down Expand Up @@ -618,6 +619,118 @@ TEST(Converter, RemoveDescendantElement)
ASSERT_TRUE(convertedElem == nullptr);
}

////////////////////////////////////////////////////
/// Ensure that Converter::Remove function is working
/// Test removing empty elements only
TEST(Converter, RemoveEmptyElement)
{
// Set up an xml string for testing
std::string xmlString = getRepeatedXmlString();

// Verify the xml
tinyxml2::XMLDocument xmlDoc;
xmlDoc.Parse(xmlString.c_str());
tinyxml2::XMLElement *childElem = xmlDoc.FirstChildElement();
ASSERT_NE(nullptr, childElem);
EXPECT_STREQ(childElem->Name(), "elemA");
childElem = childElem->FirstChildElement();
ASSERT_NE(nullptr, childElem);
EXPECT_STREQ(childElem->Name(), "elemB");
childElem = childElem->FirstChildElement();
ASSERT_NE(nullptr, childElem);
EXPECT_STREQ(childElem->Name(), "elemC");
childElem = childElem->FirstChildElement();
ASSERT_NE(nullptr, childElem);
EXPECT_STREQ(childElem->Name(), "elemD");

// Test removing empty elements
// Set up a convert file
std::stringstream convertStream;
convertStream << "<convert name='elemA'>"
<< " <convert name='elemB'>"
<< " <convert name='elemC'>"
<< " <remove_empty element='elemD'/>"
<< " </convert>"
<< " </convert>"
<< "</convert>";
tinyxml2::XMLDocument convertXmlDoc;
convertXmlDoc.Parse(convertStream.str().c_str());
sdf::Converter::Convert(&xmlDoc, &convertXmlDoc);

tinyxml2::XMLElement *convertedElem = xmlDoc.FirstChildElement();
azeey marked this conversation as resolved.
Show resolved Hide resolved
EXPECT_STREQ(convertedElem->Name(), "elemA");
convertedElem = convertedElem->FirstChildElement();
ASSERT_NE(nullptr, convertedElem);
EXPECT_STREQ(convertedElem->Name(), "elemB");
EXPECT_NE(nullptr, convertedElem->FirstChildElement("elemC"));
convertedElem = convertedElem->FirstChildElement("elemC");
ASSERT_NE(nullptr, convertedElem);
auto elemD = convertedElem->FirstChildElement("elemD");
ASSERT_NE(elemD, nullptr);
while (elemD)
{
std::string elemValue = elemD->GetText();
EXPECT_EQ(elemValue, "D");
elemD = elemD->NextSiblingElement("elemD");
}
}

////////////////////////////////////////////////////
/// Ensure that Converter::Remove function is working with descendant_name
/// Test removing empty elements only
TEST(Converter, RemoveEmptyDescendantElement)
{
// Set up an xml string for testing
std::string xmlString = getRepeatedXmlString();

// Verify the xml
tinyxml2::XMLDocument xmlDoc;
xmlDoc.Parse(xmlString.c_str());
tinyxml2::XMLElement *childElem = xmlDoc.FirstChildElement();
ASSERT_NE(nullptr, childElem);
EXPECT_STREQ(childElem->Name(), "elemA");
childElem = childElem->FirstChildElement();
ASSERT_NE(nullptr, childElem);
EXPECT_STREQ(childElem->Name(), "elemB");
childElem = childElem->FirstChildElement();
ASSERT_NE(nullptr, childElem);
EXPECT_STREQ(childElem->Name(), "elemC");
childElem = childElem->FirstChildElement();
ASSERT_NE(nullptr, childElem);
EXPECT_STREQ(childElem->Name(), "elemD");

// Test removing empty elements
// Set up a convert file
std::stringstream convertStream;
convertStream << "<convert name='elemA'>"
<< " <convert descendant_name='elemC'>"
<< " <remove_empty element='elemD'/>"
<< " </convert>"
<< "</convert>";
tinyxml2::XMLDocument convertXmlDoc;
convertXmlDoc.Parse(convertStream.str().c_str());
sdf::Converter::Convert(&xmlDoc, &convertXmlDoc);

tinyxml2::XMLElement *convertedElem = xmlDoc.FirstChildElement();
azeey marked this conversation as resolved.
Show resolved Hide resolved

EXPECT_STREQ(convertedElem->Name(), "elemA");
convertedElem = convertedElem->FirstChildElement();
ASSERT_NE(nullptr, convertedElem);
EXPECT_STREQ(convertedElem->Name(), "elemB");
EXPECT_NE(nullptr, convertedElem->FirstChildElement("elemC"));
convertedElem = convertedElem->FirstChildElement("elemC");
ASSERT_NE(nullptr, convertedElem);
auto elemD = convertedElem->FirstChildElement("elemD");
ASSERT_NE(elemD, nullptr);
while (elemD)
{
std::string elemValue = elemD->GetText();
EXPECT_EQ(elemValue, "D");
elemD = elemD->NextSiblingElement("elemD");
}
}

////////////////////////////////////////////////////
TEST(Converter, RemoveDescendantNestedElement)
{
// Set up an xml string for testing
Expand Down Expand Up @@ -673,6 +786,7 @@ TEST(Converter, RemoveDescendantNestedElement)

EXPECT_STREQ(xmlDocOut.CStr(), expectedXmlDocOut.CStr());
}

////////////////////////////////////////////////////
/// Ensure that Converter ignores descendants of <plugin> or namespaced elements
TEST(Converter, DescendantIgnorePluginOrNamespacedElements)
Expand Down Expand Up @@ -805,7 +919,7 @@ TEST(Converter, RemoveAttr)
ASSERT_NE(nullptr, childElem);
EXPECT_STREQ(childElem->Name(), "elemD");

// Test adding element
// Test removing attrC attributes
// Set up a convert file
std::stringstream convertStream;
convertStream << "<convert name='elemA'>"
Expand All @@ -832,6 +946,59 @@ TEST(Converter, RemoveAttr)
ASSERT_NE(nullptr, convertedElem);
}

////////////////////////////////////////////////////
/// Ensure that Converter::Remove function is working
/// Test removing empty attributes only
TEST(Converter, RemoveEmptyAttr)
{
// Set up an xml string for testing
std::string xmlString = getRepeatedXmlString();

// Verify the xml
tinyxml2::XMLDocument xmlDoc;
xmlDoc.Parse(xmlString.c_str());
tinyxml2::XMLElement *childElem = xmlDoc.FirstChildElement();
ASSERT_NE(nullptr, childElem);
EXPECT_STREQ(childElem->Name(), "elemA");
childElem = childElem->FirstChildElement();
ASSERT_NE(nullptr, childElem);
EXPECT_STREQ(childElem->Name(), "elemB");
childElem = childElem->FirstChildElement();
ASSERT_NE(nullptr, childElem);
EXPECT_STREQ(childElem->Name(), "elemC");
childElem = childElem->FirstChildElement();
ASSERT_NE(nullptr, childElem);
EXPECT_STREQ(childElem->Name(), "elemD");

// Test adding element
// Set up a convert file
std::stringstream convertStream;
convertStream << "<convert name='elemA'>"
<< " <convert name='elemB'>"
<< " <convert name='elemC'>"
<< " <remove_empty attribute='attrC'/>"
<< " <remove_empty attribute='attrEmpty'/>"
<< " </convert>"
<< " </convert>"
<< "</convert>";
tinyxml2::XMLDocument convertXmlDoc;
convertXmlDoc.Parse(convertStream.str().c_str());
sdf::Converter::Convert(&xmlDoc, &convertXmlDoc);

tinyxml2::XMLElement *convertedElem = xmlDoc.FirstChildElement();
EXPECT_STREQ(convertedElem->Name(), "elemA");
convertedElem = convertedElem->FirstChildElement();
ASSERT_NE(nullptr, convertedElem);
EXPECT_STREQ(convertedElem->Name(), "elemB");
EXPECT_NE(nullptr, convertedElem->FirstChildElement("elemC"));
convertedElem = convertedElem->FirstChildElement("elemC");
ASSERT_NE(nullptr, convertedElem);
EXPECT_NE(nullptr, convertedElem->Attribute("attrC"));
EXPECT_EQ(nullptr, convertedElem->Attribute("attrEmpty"));
convertedElem = convertedElem->FirstChildElement("elemD");
ASSERT_NE(nullptr, convertedElem);
}

////////////////////////////////////////////////////
TEST(Converter, RemoveNoElement)
{
Expand Down
3 changes: 3 additions & 0 deletions test/sdf/model_frame_relative_to_joint.sdf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@
<model name="model_frame_relative_to_joint">
<link name="P">
<pose>1 0 0 0 0 0</pose>
<inertial>
<pose relative_to="">0 0 0 0 0 0</pose>
</inertial>
<collision name="P1">
<pose>0 0 10 0 0 0</pose>
<geometry>
Expand Down