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

Store include info in Element #509

Merged
merged 6 commits into from
Mar 23, 2021
Merged
Show file tree
Hide file tree
Changes from 4 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
38 changes: 36 additions & 2 deletions include/sdf/Element.hh
Original file line number Diff line number Diff line change
Expand Up @@ -378,11 +378,23 @@ namespace sdf

/// \brief Set the include filename to the passed in filename.
/// \param[in] _filename the filename to set the include filename to.
public: void SetInclude(const std::string &_filename);
public: void SetInclude(const std::string &_filename) SDF_DEPRECATED(11.0);

/// \brief Get the include filename.
/// \return The include filename.
public: std::string GetInclude() const;
public: std::string GetInclude() const SDF_DEPRECATED(11.0);

/// \brief Set the <include> element that was used to load this element.
/// This set by the parser on the first element of the included object (eg.
azeey marked this conversation as resolved.
Show resolved Hide resolved
/// Model, Actor, Light, etc). It is not passed down to children elements.
/// \param[in] _includeELem The <include> Element
public: void SetIncludeElement(sdf::ElementPtr _includeElem);

/// \brief Get the <include> element that was used to load this element.
/// \return The Element pointer to the <include> element, if this element
/// was loaded from an included file. Otherwise, nullptr. This is also
/// nullptr for Elements that cannot be included.
public: sdf::ElementPtr GetIncludeElement() const;

/// \brief Set the path to the SDF document where this element came from.
/// \param[in] _path Full path to SDF document.
Expand Down Expand Up @@ -479,6 +491,28 @@ namespace sdf
// The possible child elements
public: ElementPtr_V elementDescriptions;

/// \brief The <include> element that was used to load this entity. For
/// example, given the following SDFormat:
/// <sdf version='1.8'>
/// <world name='default'>
/// <include>
/// <uri>model_uri</uri>
/// <pose>1 2 3 0 0 0</pose>
/// </include>
/// </world>
/// </sdf>
/// The ElementPtr associated with the model loaded from `model_uri` will
/// have the includeElement set to
/// <include>
/// <uri>model_uri</uri>
/// <pose>1 2 3 0 0 0</pose>
/// </include>
///
/// This can be used to retrieve additional information available under the
/// <include> tag after the entity has been loaded. An example use case for
/// this is when saving a loaded world back to SDFormat.
public: ElementPtr includeElement;

/// name of the include file that was used to create this element
public: std::string includeFilename;

Expand Down
29 changes: 29 additions & 0 deletions src/Element.cc
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,11 @@ ElementPtr Element::Clone() const
clone->dataPtr->value = this->dataPtr->value->Clone();
}

if (this->dataPtr->includeElement)
{
clone->dataPtr->includeElement = this->dataPtr->includeElement->Clone();
}

return clone;
}

Expand Down Expand Up @@ -250,6 +255,18 @@ void Element::Copy(const ElementPtr _elem)
elem->SetParent(shared_from_this());
this->dataPtr->elements.push_back(elem);
}

if (_elem->dataPtr->includeElement)
{
if (!this->dataPtr->includeElement)
{
this->dataPtr->includeElement = _elem->dataPtr->includeElement->Clone();
}
else
{
this->dataPtr->includeElement->Copy(_elem->dataPtr->includeElement);
}
}
}

/////////////////////////////////////////////////
Expand Down Expand Up @@ -912,6 +929,18 @@ std::string Element::GetInclude() const
return this->dataPtr->includeFilename;
}

/////////////////////////////////////////////////
void Element::SetIncludeElement(sdf::ElementPtr _includeElem)
{
this->dataPtr->includeElement = _includeElem;
}
azeey marked this conversation as resolved.
Show resolved Hide resolved

/////////////////////////////////////////////////
sdf::ElementPtr Element::GetIncludeElement() const
{
return this->dataPtr->includeElement;
}

/////////////////////////////////////////////////
void Element::SetFilePath(const std::string &_path)
{
Expand Down
31 changes: 31 additions & 0 deletions src/Element_TEST.cc
Original file line number Diff line number Diff line change
Expand Up @@ -158,11 +158,28 @@ TEST(Element, Include)
{
sdf::Element elem;

SDF_SUPPRESS_DEPRECATED_BEGIN
azeey marked this conversation as resolved.
Show resolved Hide resolved
ASSERT_EQ(elem.GetInclude(), "");

elem.SetInclude("foo.txt");

ASSERT_EQ(elem.GetInclude(), "foo.txt");
SDF_SUPPRESS_DEPRECATED_END

auto includeElemToStore = std::make_shared<sdf::Element>();
includeElemToStore->SetName("include");
auto uriDesc = std::make_shared<sdf::Element>();
uriDesc->SetName("uri");
uriDesc->AddValue("string", "", true);
includeElemToStore->AddElementDescription(uriDesc);

includeElemToStore->GetElement("uri")->Set("foo.txt");
elem.SetIncludeElement(includeElemToStore);

auto includeElem = elem.GetIncludeElement();
ASSERT_NE(nullptr, includeElem);
ASSERT_TRUE(includeElem->HasElement("uri"));
EXPECT_EQ("foo.txt", includeElem->GetElement("uri")->Get<std::string>());
}

/////////////////////////////////////////////////
Expand Down Expand Up @@ -276,13 +293,19 @@ TEST(Element, Clone)
parent->SetFilePath("/path/to/file.sdf");
parent->SetOriginalVersion("1.5");

auto includeElemToStore = std::make_shared<sdf::Element>();
includeElemToStore->SetName("include");
parent->SetIncludeElement(includeElemToStore);

sdf::ElementPtr newelem = parent->Clone();

EXPECT_EQ("/path/to/file.sdf", newelem->FilePath());
EXPECT_EQ("1.5", newelem->OriginalVersion());
ASSERT_NE(newelem->GetFirstElement(), nullptr);
ASSERT_EQ(newelem->GetElementDescriptionCount(), 1UL);
ASSERT_EQ(newelem->GetAttributeCount(), 1UL);
ASSERT_NE(newelem->GetIncludeElement(), nullptr);
EXPECT_EQ("include", newelem->GetIncludeElement()->GetName());
}

/////////////////////////////////////////////////
Expand Down Expand Up @@ -438,11 +461,13 @@ TEST(Element, ToStringInclude)
{
sdf::Element elem;

SDF_SUPPRESS_DEPRECATED_BEGIN
ASSERT_EQ(elem.GetInclude(), "");

elem.SetInclude("foo.txt");

ASSERT_EQ(elem.GetInclude(), "foo.txt");
SDF_SUPPRESS_DEPRECATED_END

std::string stringval = elem.ToString("myprefix");
ASSERT_EQ(stringval, "myprefix<include filename='foo.txt'/>\n");
Expand Down Expand Up @@ -547,6 +572,10 @@ TEST(Element, Copy)
src->AddAttribute("test", "string", "foo", false, "foo description");
src->InsertElement(std::make_shared<sdf::Element>());

auto includeElemToStore = std::make_shared<sdf::Element>();
includeElemToStore->SetName("include");
src->SetIncludeElement(includeElemToStore);

dest->Copy(src);

EXPECT_EQ("/path/to/file.sdf", dest->FilePath());
Expand All @@ -568,6 +597,8 @@ TEST(Element, Copy)
ASSERT_EQ(param->GetDescription(), "foo description");

ASSERT_NE(dest->GetFirstElement(), nullptr);
ASSERT_NE(dest->GetIncludeElement(), nullptr);
EXPECT_EQ("include", dest->GetIncludeElement()->GetName());
}

/////////////////////////////////////////////////
Expand Down
27 changes: 16 additions & 11 deletions src/parser.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1078,11 +1078,12 @@ bool readXml(tinyxml2::XMLElement *_xml, ElementPtr _sdf,
if (std::string("include") == elemXml->Value())
{
std::string modelPath;
std::string uri;
tinyxml2::XMLElement *uriElement = elemXml->FirstChildElement("uri");

if (uriElement)
{
std::string uri = uriElement->GetText();
uri = uriElement->GetText();
modelPath = sdf::findFile(uri, true, true, _config);

// Test the model path
Expand Down Expand Up @@ -1313,14 +1314,18 @@ bool readXml(tinyxml2::XMLElement *_xml, ElementPtr _sdf,
}
}

includeSDF->Root()->GetFirstElement()->SetParent(_sdf);
_sdf->InsertElement(includeSDF->Root()->GetFirstElement());
// TODO: This was used to store the included filename so that when
// a world is saved, the included model's SDF is not stored in the
// world file. This highlights the need to make model inclusion
// a core feature of SDF, and not a hack that that parser handles
// includeSDF->Root()->GetFirstElement()->SetInclude(
// elemXml->Attribute("filename"));
auto includeSDFFirstElem = includeSDF->Root()->GetFirstElement();
includeSDFFirstElem->SetParent(_sdf);
auto includeDesc = _sdf->GetElementDescription("include");
if (includeDesc)
{
// Store the contents of the <include> tag as the includeElement of
// the entity that was loaded from the included URI.
auto includeInfo = includeDesc->Clone();
copyChildren(includeInfo, elemXml, false);
includeSDFFirstElem->SetIncludeElement(includeInfo);
}
_sdf->InsertElement(includeSDFFirstElem);

continue;
}
Expand Down Expand Up @@ -1439,8 +1444,8 @@ void copyChildren(ElementPtr _sdf,
}

// copy value
std::string value = elemXml->GetText();
if (!value.empty())
const char *value = elemXml->GetText();
if (value)
{
element->GetValue()->SetFromString(value);
}
Expand Down
85 changes: 85 additions & 0 deletions test/integration/nested_model.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1396,3 +1396,88 @@ TEST(NestedReference, PlacementFrameElement)
EXPECT_EQ(placementPose, pose.Inverse());
}
}

/////////////////////////////////////////////////
TEST(NestedModel, IncludeElements)
{
const std::string modelRootPath = sdf::filesystem::append(
PROJECT_SOURCE_PATH, "test", "integration", "model");

sdf::ParserConfig config;
config.SetFindCallback(
[&](const std::string &_file)
{
return sdf::filesystem::append(modelRootPath, _file);
});

auto checkIncludeElem = [](sdf::ElementPtr _includeElem,
const std::string &_uri, const std::string &_name)
{
ASSERT_NE(nullptr, _includeElem);
ASSERT_TRUE(_includeElem->HasElement("uri"));
EXPECT_EQ(_uri, _includeElem->Get<std::string>("uri"));

ASSERT_TRUE(_includeElem->HasElement("name"));
EXPECT_EQ(_name, _includeElem->Get<std::string>("name"));

ASSERT_TRUE(_includeElem->HasElement("pose"));
EXPECT_EQ(ignition::math::Pose3d(1, 2, 3, 0, 0, 0),
_includeElem->Get<ignition::math::Pose3d>("pose"));

ASSERT_TRUE(_includeElem->HasElement("placement_frame"));
EXPECT_EQ("link", _includeElem->Get<std::string>("placement_frame"));

ASSERT_TRUE(_includeElem->HasElement("plugin"));
auto pluginElem = _includeElem->GetElement("plugin");
ASSERT_TRUE(pluginElem->HasAttribute("name"));
EXPECT_EQ("test_plugin", pluginElem->Get<std::string>("name"));
ASSERT_TRUE(pluginElem->HasAttribute("filename"));
EXPECT_EQ("test_plugin_file", pluginElem->Get<std::string>("filename"));
};

// Test with //world/include
{
std::ostringstream stream;
stream << R"(
<sdf version='1.8'>
azeey marked this conversation as resolved.
Show resolved Hide resolved
<world name='default'>
<include>
<uri>box</uri>
<name>test_box</name>
<pose>1 2 3 0 0 0</pose>
<placement_frame>link</placement_frame>
<plugin name='test_plugin' filename='test_plugin_file'/>
</include>
<model name='test2'>
<include>
<uri>test_model</uri>
<name>test_model</name>
<pose>1 2 3 0 0 0</pose>
<placement_frame>link</placement_frame>
<plugin name='test_plugin' filename='test_plugin_file'/>
</include>
</model>
</world>
</sdf>)";
sdf::Root root;
sdf::Errors errors = root.LoadSdfString(stream.str(), config);
EXPECT_TRUE(errors.empty()) << errors;

auto *world = root.WorldByIndex(0);
ASSERT_NE(nullptr, world);
auto *box = world->ModelByIndex(0);
ASSERT_NE(nullptr, box);

checkIncludeElem(
box->Element()->GetIncludeElement(), "box", "test_box");

auto *test2 = world->ModelByIndex(1);
ASSERT_NE(nullptr, test2);
EXPECT_EQ(nullptr, test2->Element()->GetIncludeElement());

auto *testModel2 = test2->ModelByIndex(0);
ASSERT_NE(nullptr, testModel2);
checkIncludeElem(
testModel2->Element()->GetIncludeElement(), "test_model", "test_model");
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 not fully 1000% convinced that this preserves the information across load str -> element -> dump str.

Can you add a quick idempotent check that the include markup is preserved as-is?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Per f2f, this implies that the //include tags would be preserved, which should not be the default libsdformat behavior.

Perhaps there should be an option (API + cmdline) to preserve includes?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I created an issue to capture this: #522. How about we tackle that in a follow up PR?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds great!

}
}