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

Unflatten flattened models #504

Merged
merged 14 commits into from
Mar 18, 2021
Merged
Show file tree
Hide file tree
Changes from 9 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
6 changes: 6 additions & 0 deletions sdf/1.8/1_7.convert
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
<convert name="sdf">
Copy link
Collaborator

Choose a reason for hiding this comment

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

Blocking item: Should remove debug-based shims. (Just putting it here for review-based tracking)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed 3e0cd5d


<convert name="world">
<convert name="model">
<unnest/>
</convert>
</convert>

</convert> <!-- End SDF -->
302 changes: 302 additions & 0 deletions src/Converter.cc
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,17 @@ void Converter::ConvertDescendantsImpl(tinyxml2::XMLElement *_e,
}
}


/////////////////////////////////////////////////
// TODO(jenn) delete, used for debugging
std::string PrintElement(const tinyxml2::XMLElement *_elem)
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit Perhaps this is useful to keep around? Perhaps implement using a trace or debug logging level?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

PrintElement is now ElementToString put in XmlUtils.cc: 9ed2713

{
tinyxml2::XMLPrinter printer;
_elem->Accept(&printer);

return std::string(printer.CStr());
}

/////////////////////////////////////////////////
void Converter::ConvertImpl(tinyxml2::XMLElement *_elem,
tinyxml2::XMLElement *_convert)
Expand Down Expand Up @@ -229,13 +240,304 @@ void Converter::ConvertImpl(tinyxml2::XMLElement *_elem,
{
Remove(_elem, childElem);
}
else if (name == "unnest")
{
Unnest(_elem);
// TODO(jenn) delete debug statements
// std::cout << "\n\n_elem after unnest:\n"
// << PrintElement(_elem) << std::endl;
}
else if (name != "convert")
{
sdferr << "Unknown convert element[" << name << "]\n";
}
}
}

/////////////////////////////////////////////////
void Converter::Unnest(tinyxml2::XMLElement *_elem)
{
SDF_ASSERT(_elem != NULL, "SDF element is NULL");

tinyxml2::XMLDocument *doc = _elem->GetDocument();

tinyxml2::XMLElement *nextElem = nullptr, *firstUnnestedModel = nullptr;
for (tinyxml2::XMLElement *elem = _elem->FirstChildElement();
elem;
elem = nextElem)
{
// break loop if reached the first unnested model
if (firstUnnestedModel == elem) break;

nextElem = elem->NextSiblingElement();
std::string elemName = elem->Name();

// skip element if not one of the following or if missing name attribute
if ((elemName != "frame" && elemName != "joint"
&& elemName != "link" && elemName != "model")
|| !elem->Attribute("name"))
{
continue;
}

std::string attrName = elem->Attribute("name");

size_t found = attrName.find("::");
if (found == std::string::npos)
{
// recursive unnest
if (elemName == "model")
{
// std::cout << "before recursive:\n"
// << PrintElement(elem) << std::endl;
Unnest(elem);

// std::cout << "unnested model:\n" << PrintElement(elem) << std::endl;
break;
}

continue;
}

std::string newModelName = attrName.substr(0, found);
tinyxml2::XMLElement *newModel = doc->NewElement("model");
newModel->SetAttribute("name", newModelName.c_str());

if (FindNewModelElements(_elem, newModel, found + 2))
{
Unnest(newModel);
_elem->InsertEndChild(newModel);

// since newModel is inserted at the end, point back to the top element
nextElem = _elem->FirstChildElement();

if (!firstUnnestedModel)
firstUnnestedModel = newModel;
// std::cout << "newModel:\n" << PrintElement(newModel) << std::endl;
}
}
}

/////////////////////////////////////////////////
bool Converter::FindNewModelElements(tinyxml2::XMLElement *_elem,
tinyxml2::XMLElement *_newModel,
const size_t &_newNameIdx)
{
bool unnestedNewModel = false;
std::string newModelName = _newModel->Attribute("name");

tinyxml2::XMLElement *elem = _elem->FirstChildElement(), *nextElem = nullptr;
while (elem)
{
nextElem = elem->NextSiblingElement();

std::string elemName = elem->Name();
std::string elemAttrName;

if (elem->Attribute("name"))
elemAttrName = elem->Attribute("name");

if (elemAttrName.empty() ||
elemAttrName.compare(0, _newNameIdx - 2, newModelName) != 0 ||
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit _newNameIdx has semantics, but they aren't clear. Also, this term is repeated.

Perhaps state something like this?

const size_t kSeparatorLen = 2;  // "::"
size_t possibleParentNameIdx = _newNameIdx - kSeparatorLen;

Copy link
Collaborator

Choose a reason for hiding this comment

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

Alternatively, you may want to consider making str.compare(0, posibleParentNameIdx) a bit more robust, and use a more focused function to name splitting?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How's this? I've updated _newNameIdx to be _childNameIdx. Also, instead of using _newNameIdx - 2 it uses size_t newModelNameSize = newModelName.size(): 9ed2713

Copy link
Collaborator

Choose a reason for hiding this comment

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

Excellent, thanks!

(elemName != "frame" && elemName != "joint" &&
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit This set of if checks are repeated; intent isn't clear due to repetition.

Can you give it semantics and wrap it? e.g. bool IsInterfaceElement(elem) { return elemName == "asdf" || ...; }

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

elemName != "link" && elemName != "model"))
{
if (elemName != "gripper")
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Can this if condition go into the parent if statement? If not, can you add a comment why?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, I left a comment for why: dce6828

Did some testing and //gripper/@name doesn't get flattened but //gripper/gripper_link and //griper/palm_link do. The parent if statement returns true because //gripper/@name doesn't contain newModelName

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm surprised that //gripper/@name is not flattened. I'm guessing it's a bug. I like your approach of checking if //gripper/gripper_link has the model name prefix. To future proof this a little bit, when we determine the gripper needs to be moved to a child model, we can check if //gripper/@name has the model prefix and strip it. This would ensure that this would continue to work if someone fixes the flattening at some point.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How's this? 3e0cd5d

{
elem = nextElem;
continue;
}
}

// Child attribute name w/ newModelName prefix stripped except for //gripper
std::string childAttrName;
if (elemName != "gripper")
{
childAttrName = elemAttrName.substr(_newNameIdx);
elem->SetAttribute("name", childAttrName.c_str());
}

// strip new model prefix from //pose/@relative_to
tinyxml2::XMLElement *poseElem = elem->FirstChildElement("pose");
if (poseElem != nullptr && poseElem->Attribute("relative_to"))
{
std::string poseRelTo = poseElem->Attribute("relative_to");

if (poseRelTo.compare(0, _newNameIdx - 2, newModelName) == 0)
{
poseRelTo = poseRelTo.substr(_newNameIdx);
poseElem->SetAttribute("relative_to", poseRelTo.c_str());
}
}

if (elemName == "frame")
{
std::string attachedTo;

if (elem->Attribute("attached_to"))
{
attachedTo = elem->Attribute("attached_to");

SDF_ASSERT(attachedTo.compare(0, _newNameIdx - 2, newModelName) == 0,
"Error: Frame attribute 'attached_to' does not start with " +
newModelName);

// strip new model prefix from attached_to
attachedTo = attachedTo.substr(_newNameIdx);
elem->SetAttribute("attached_to", attachedTo.c_str());
}

// remove frame if childAttrName == __model__
if (childAttrName == "__model__" && elem->Attribute("attached_to"))
{
_newModel->SetAttribute("canonical_link", attachedTo.c_str());
_newModel->InsertFirstChild(poseElem);

_elem->DeleteChild(elem);
elem = poseElem;
}
} // frame

else if (elemName == "link")
{
// find & strip new model prefix of all //link/<element>/pose/@relative_to
for (tinyxml2::XMLElement *e = elem->FirstChildElement();
e;
e = e->NextSiblingElement())
{
poseElem = e->FirstChildElement("pose");
if (poseElem != nullptr)
{
std::string poseRelTo = poseElem->Attribute("relative_to");

SDF_ASSERT(poseRelTo.compare(0, _newNameIdx - 2, newModelName) == 0,
"Error: Pose attribute 'relative_to' does not start with " +
newModelName);

poseRelTo = poseRelTo.substr(_newNameIdx);
poseElem->SetAttribute("relative_to", poseRelTo.c_str());
}
}
} // link

else if (elemName == "joint")
{
// strip new model prefix from //joint/parent
tinyxml2::XMLElement *e = elem->FirstChildElement("parent");
std::string eText = e->GetText();

SDF_ASSERT(eText.compare(0, _newNameIdx - 2, newModelName) == 0,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rule of 3 on _newNameIdx - 2: Raising to defect.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

"Error: Joint's <parent> value does not start with " + newModelName);

e->SetText(eText.substr(_newNameIdx).c_str());

// strip new model prefix from //joint/child
e = elem->FirstChildElement("child");
eText = e->GetText();

SDF_ASSERT(eText.compare(0, _newNameIdx - 2, newModelName) == 0,
"Error: Joint's <child> value does not start with " + newModelName);

e->SetText(std::string(e->GetText()).substr(_newNameIdx).c_str());

// strip new model prefix from //xyz/@expressed_in
std::string axisStr = "axis";
while (true)
{
tinyxml2::XMLElement *axisElem =
elem->FirstChildElement(axisStr.c_str());
if (axisElem != nullptr)
{
if (axisElem->FirstChildElement("xyz")->Attribute("expressed_in"))
{
std::string expressIn =
axisElem->FirstChildElement("xyz")->Attribute("expressed_in");

SDF_ASSERT(expressIn.compare(0, _newNameIdx - 2, newModelName) == 0,
"Error: <xyz>'s attribute 'expressed_in' does not start with " +
newModelName);

expressIn = expressIn.substr(_newNameIdx);

axisElem->FirstChildElement("xyz")
->SetAttribute("expressed_in", expressIn.c_str());
}
}

if (axisStr == "axis2") break;

axisStr += "2";
}

// strip new model prefix from all //joint/sensor/pose/@relative_to
for (e = elem->FirstChildElement("sensor");
e;
e = e->NextSiblingElement("sensor"))
{
poseElem = e->FirstChildElement("pose");
if (poseElem != nullptr)
{
std::string poseRelTo = poseElem->Attribute("relative_to");

SDF_ASSERT(poseRelTo.compare(0, _newNameIdx - 2, newModelName) == 0,
"Error: Pose attribute 'relative_to' does not start with " +
newModelName);

poseRelTo = poseRelTo.substr(_newNameIdx);
poseElem->SetAttribute("relative_to", poseRelTo.c_str());
}
}
} // joint

else if (elemName == "gripper")
{
bool hasPrefix = true;

// strip prefix from all /model/gripper/gripper_link
tinyxml2::XMLElement *e = elem->FirstChildElement("gripper_link");
std::string eText;
while (e)
{
eText = e->GetText();

if (eText.compare(0, _newNameIdx - 2, newModelName) != 0)
{
hasPrefix = false;
break;
}

e->SetText(eText.substr(_newNameIdx).c_str());
e = e->NextSiblingElement("gripper_link");
}

// if //model/gripper/gripper_link does not have new model prefix
// don't add to new model
if (!hasPrefix)
{
elem = nextElem;
continue;
}

// strip prefix from //model/gripper/palm_link
e = elem->FirstChildElement("palm_link");
eText = e->GetText();

SDF_ASSERT(eText.compare(0, _newNameIdx - 2, newModelName) == 0,
"Error: Gripper's <palm_link> value does not start with "
+ newModelName);

e->SetText(eText.substr(_newNameIdx).c_str());
} // gripper

unnestedNewModel = true;
_newModel->InsertEndChild(elem);

elem = nextElem;
}

return unnestedNewModel;
}

/////////////////////////////////////////////////
void Converter::Rename(tinyxml2::XMLElement *_elem,
tinyxml2::XMLElement *_renameElem)
Expand Down
16 changes: 16 additions & 0 deletions src/Converter.hh
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@
#include <tinyxml2.h>

#include <string>
#include <vector>
#include <tuple>

#include <sdf/sdf_config.h>
#include "sdf/system_util.hh"
Expand Down Expand Up @@ -103,6 +105,20 @@ namespace sdf
private: static void Remove(tinyxml2::XMLElement *_elem,
tinyxml2::XMLElement *_removeElem);

/// \brief Unnest an element (conversion from SDFormat <= 1.7 to 1.8)
/// \param[in] _elem The element to unnest
private: static void Unnest(tinyxml2::XMLElement *_elem);

/// \brief Finds all elements related to the unnested model
/// \param[in] _elem The element to unnest
/// \param[in] _newModel The new unnested model element
/// \param[in] _newNameIdx The index of the new name for child elements
/// \return True if unnested new model elements
private:
static bool FindNewModelElements(tinyxml2::XMLElement *_elem,
tinyxml2::XMLElement *_newModel,
const size_t &_newNameIdx);

private: static const char *GetValue(const char *_valueElem,
const char *_valueAttr,
tinyxml2::XMLElement *_elem);
Expand Down
Loading