Skip to content

Commit

Permalink
SetParentElement to return bool, added tests to handle quat_wxyz
Browse files Browse the repository at this point in the history 
Signed-off-by: Aaron Chong <[email protected]>
  • Loading branch information
@aaronchongth
aaronchongth committed Sep 16, 2021
1 parent 22a6614 commit 2059da8
Show file tree
Hide file tree
Showing 7 changed files with 363 additions and 56 deletions.
4 changes: 3 additions & 1 deletion include/sdf/Param.hh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,9 @@ namespace sdf
/// \brief Set the parent Element of this Param.
/// \param[in] _parentElement Pointer to new parent Element. A nullptr can
/// provided to remove the current parent Element.
public: void SetParentElement(ElementPtr _parentElement);
/// \return True if the parent Element was set and the value was reparsed
/// successfully.
public: bool SetParentElement(ElementPtr _parentElement);

/// \brief Reset the parameter to the default value.
public: void Reset();
Expand Down
18 changes: 12 additions & 6 deletions src/Element.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,8 @@ void Element::AddValue(const std::string &_type,
this->dataPtr->value =
std::make_shared<Param>(this->dataPtr->name, _type, _defaultValue,
_required, _minValue, _maxValue, _description);
this->dataPtr->value->SetParentElement(shared_from_this());
SDF_ASSERT(this->dataPtr->value->SetParentElement(shared_from_this()),
"Cannot set value Param's parent Element to itself.");
}

/////////////////////////////////////////////////
Expand All @@ -164,7 +165,8 @@ ParamPtr Element::CreateParam(const std::string &_key,
{
ParamPtr param = std::make_shared<Param>(
_key, _type, _defaultValue, _required, _description);
param->SetParentElement(shared_from_this());
SDF_ASSERT(param->SetParentElement(shared_from_this()),
"Cannot set value Param's parent Element to itself.");
return param;
}

Expand Down Expand Up @@ -199,7 +201,8 @@ ElementPtr Element::Clone() const
aiter != this->dataPtr->attributes.end(); ++aiter)
{
auto clonedAttribute = (*aiter)->Clone();
clonedAttribute->SetParentElement(clone);
SDF_ASSERT(clonedAttribute->SetParentElement(clone),
"Cannot set Clone's attribute Param's parent Element to clone.");
clone->dataPtr->attributes.push_back(clonedAttribute);
}

Expand All @@ -220,7 +223,8 @@ ElementPtr Element::Clone() const
if (this->dataPtr->value)
{
clone->dataPtr->value = this->dataPtr->value->Clone();
clone->dataPtr->value->SetParentElement(clone);
SDF_ASSERT(clone->dataPtr->value->SetParentElement(clone),
"Cannot set clone Element's value Param's parent Element to clone.");
}

if (this->dataPtr->includeElement)
Expand Down Expand Up @@ -254,7 +258,8 @@ void Element::Copy(const ElementPtr _elem)
}
ParamPtr param = this->GetAttribute((*iter)->GetKey());
(*param) = (**iter);
param->SetParentElement(shared_from_this());
SDF_ASSERT(param->SetParentElement(shared_from_this()),
"Cannot set attribute Param's parent Element to itself.");
}

if (_elem->GetValue())
Expand All @@ -267,7 +272,8 @@ void Element::Copy(const ElementPtr _elem)
{
*(this->dataPtr->value) = *(_elem->GetValue());
}
this->dataPtr->value->SetParentElement(shared_from_this());
SDF_ASSERT(this->dataPtr->value->SetParentElement(shared_from_this()),
"Cannot set value Param's parent Element to itself.");
}

this->dataPtr->elementDescriptions.clear();
Expand Down
12 changes: 10 additions & 2 deletions src/Param.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -795,10 +795,18 @@ ElementPtr Param::GetParentElement() const
}

//////////////////////////////////////////////////
void Param::SetParentElement(ElementPtr _parentElement)
bool Param::SetParentElement(ElementPtr _parentElement)
{
auto prevParentElement = this->dataPtr->parentElement;

this->dataPtr->parentElement = _parentElement;
this->Reparse();
if (!this->Reparse())
{
this->dataPtr->parentElement = prevParentElement;
return false;
}

return true;
}

//////////////////////////////////////////////////
Expand Down
68 changes: 58 additions & 10 deletions src/Param_TEST.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -497,20 +497,20 @@ TEST(Param, SettingParentElement)
sdf::ElementPtr parentElement = std::make_shared<sdf::Element>();

sdf::Param doubleParam("key", "double", "1.0", false, "description");
doubleParam.SetParentElement(parentElement);
ASSERT_TRUE(doubleParam.SetParentElement(parentElement));

This comment has been minimized.

Sign in to view

Copy link
@scpeters

scpeters Sep 20, 2021

Member

nit: ASSERT_* statements will abort the test if they fail. I find them useful when checking that pointers aren't nullptr before dereferencing them in subsequent parts of the test, but I generally prefer EXPECT_* if a failed expectation will not cause the test to crash

so consider if ASSERT_TRUE is needed for these new expectations

ASSERT_NE(nullptr, doubleParam.GetParentElement());
EXPECT_EQ(parentElement, doubleParam.GetParentElement());

// Set a new parent Element
sdf::ElementPtr newParentElement = std::make_shared<sdf::Element>();

doubleParam.SetParentElement(newParentElement);
ASSERT_TRUE(doubleParam.SetParentElement(newParentElement));
ASSERT_NE(nullptr, doubleParam.GetParentElement());
EXPECT_EQ(newParentElement, doubleParam.GetParentElement());

// Remove the parent Element
doubleParam.SetParentElement(nullptr);
ASSERT_TRUE(doubleParam.SetParentElement(nullptr));
EXPECT_EQ(nullptr, doubleParam.GetParentElement());
}

Expand All @@ -520,7 +520,7 @@ TEST(Param, CopyConstructor)
sdf::ElementPtr parentElement = std::make_shared<sdf::Element>();

sdf::Param doubleParam("key", "double", "1.0", false, "description");
doubleParam.SetParentElement(parentElement);
ASSERT_TRUE(doubleParam.SetParentElement(parentElement));

ASSERT_NE(nullptr, doubleParam.GetParentElement());
EXPECT_EQ(parentElement, doubleParam.GetParentElement());
Expand All @@ -536,7 +536,7 @@ TEST(Param, EqualOperator)
sdf::ElementPtr parentElement = std::make_shared<sdf::Element>();

sdf::Param doubleParam("key", "double", "1.0", false, "description");
doubleParam.SetParentElement(parentElement);
ASSERT_TRUE(doubleParam.SetParentElement(parentElement));

ASSERT_NE(nullptr, doubleParam.GetParentElement());
EXPECT_EQ(parentElement, doubleParam.GetParentElement());
Expand All @@ -556,7 +556,7 @@ TEST(Param, DestroyParentElementAfterConstruct)

param = std::make_shared<sdf::Param>(
"key", "double", "1.0", false, "description");
param->SetParentElement(parentElement);
ASSERT_TRUE(param->SetParentElement(parentElement));
}

ASSERT_NE(nullptr, param);
Expand Down Expand Up @@ -637,12 +637,13 @@ TEST(Param, ReparsingAfterSetPose)
poseElem->AddValue("pose", "0 0 0 0 0 0", true);
poseElem->AddAttribute("relative_to", "string", "", false);
poseElem->AddAttribute("degrees", "bool", "false", false);
poseElem->AddAttribute("rotation_format", "string", "euler_rpy", false);

sdf::ParamPtr degreesAttrib = poseElem->GetAttribute("degrees");
ASSERT_NE(nullptr, degreesAttrib);

// Setting parent with @degrees as false, value will remain the same.
poseParam.SetParentElement(poseElem);
ASSERT_TRUE(poseParam.SetParentElement(poseElem));
EXPECT_TRUE(poseParam.Get<Pose>(value));
EXPECT_EQ(Pose(2, 3, 4, 0.5, 0.6, 0.7), value);

Expand All @@ -657,6 +658,29 @@ TEST(Param, ReparsingAfterSetPose)
EXPECT_TRUE(poseParam.Reparse());
EXPECT_TRUE(poseParam.Get<Pose>(value));
EXPECT_EQ(Pose(2, 3, 4, 0.5, 0.6, 0.7), value);

// Changing parent @rotation_format to euler_rpy, value remains the same
sdf::ParamPtr rotationFormatAttrib =
poseElem->GetAttribute("rotation_format");
ASSERT_NE(nullptr, rotationFormatAttrib);
ASSERT_TRUE(rotationFormatAttrib->Set<std::string>("euler_rpy"));
EXPECT_TRUE(poseParam.Reparse());
EXPECT_TRUE(poseParam.Get<Pose>(value));
EXPECT_EQ(Pose(2, 3, 4, 0.5, 0.6, 0.7), value);

// Changing parent @rotation_format to quat_wxyz, reparse will pass, but value
// remains the same as before, as it was explicitly set
ASSERT_TRUE(rotationFormatAttrib->Set<std::string>("quat_wxyz"));
EXPECT_TRUE(poseParam.Reparse());
EXPECT_TRUE(poseParam.Get<Pose>(value));
EXPECT_EQ(Pose(2, 3, 4, 0.5, 0.6, 0.7), value);

// Changing parent @rotation_format to something invalid, reparse will pass,
// value remains the same as before, as it was explicitly set
ASSERT_TRUE(rotationFormatAttrib->Set<std::string>("invalid_format"));
EXPECT_TRUE(poseParam.Reparse());
EXPECT_TRUE(poseParam.Get<Pose>(value));
EXPECT_EQ(Pose(2, 3, 4, 0.5, 0.6, 0.7), value);
}

/////////////////////////////////////////////////
Expand All @@ -683,12 +707,13 @@ TEST(Param, ReparsingAfterSetFromStringPose)
poseElem->AddValue("pose", "0 0 0 0 0 0", true);
poseElem->AddAttribute("relative_to", "string", "", false);
poseElem->AddAttribute("degrees", "bool", "false", false);
poseElem->AddAttribute("rotation_format", "string", "euler_rpy", false);

sdf::ParamPtr degreesAttrib = poseElem->GetAttribute("degrees");
ASSERT_NE(nullptr, degreesAttrib);

// Setting parent with @degrees as false, value will remain the same.
poseParam.SetParentElement(poseElem);
ASSERT_TRUE(poseParam.SetParentElement(poseElem));
EXPECT_TRUE(poseParam.Get<Pose>(value));
EXPECT_EQ(Pose(2, 3, 4, 0.5, 0.6, 0.7), value);

Expand All @@ -702,6 +727,29 @@ TEST(Param, ReparsingAfterSetFromStringPose)
EXPECT_TRUE(poseParam.Reparse());
EXPECT_TRUE(poseParam.Get<Pose>(value));
EXPECT_EQ(Pose(2, 3, 4, IGN_DTOR(0.5), IGN_DTOR(0.6), IGN_DTOR(0.7)), value);

// Changing parent @rotation_format to euler_rpy, value remains the same
sdf::ParamPtr rotationFormatAttrib =
poseElem->GetAttribute("rotation_format");
ASSERT_NE(nullptr, rotationFormatAttrib);
ASSERT_TRUE(rotationFormatAttrib->Set<std::string>("euler_rpy"));
EXPECT_TRUE(poseParam.Reparse());
EXPECT_TRUE(poseParam.Get<Pose>(value));
EXPECT_EQ(Pose(2, 3, 4, IGN_DTOR(0.5), IGN_DTOR(0.6), IGN_DTOR(0.7)), value);

// Changing parent @rotation_format to quat_wxyz, reparse will fail, value
// remains the same as before
ASSERT_TRUE(rotationFormatAttrib->Set<std::string>("quat_wxyz"));
EXPECT_FALSE(poseParam.Reparse());
EXPECT_TRUE(poseParam.Get<Pose>(value));
EXPECT_EQ(Pose(2, 3, 4, IGN_DTOR(0.5), IGN_DTOR(0.6), IGN_DTOR(0.7)), value);

// Changing parent @rotation_format to something invalid, reparse will fail,
// value remains the same as before
ASSERT_TRUE(rotationFormatAttrib->Set<std::string>("invalid_format"));
EXPECT_FALSE(poseParam.Reparse());
EXPECT_TRUE(poseParam.Get<Pose>(value));
EXPECT_EQ(Pose(2, 3, 4, IGN_DTOR(0.5), IGN_DTOR(0.6), IGN_DTOR(0.7)), value);
}

/////////////////////////////////////////////////
Expand All @@ -717,7 +765,7 @@ TEST(Param, IgnoresParentElementAttribute)
// With parent
sdf::Param doubleParam("key", "double", "1.0", false, "description");
sdf::ElementPtr parentElement = std::make_shared<sdf::Element>();
doubleParam.SetParentElement(parentElement);
ASSERT_TRUE(doubleParam.SetParentElement(parentElement));
EXPECT_FALSE(doubleParam.IgnoresParentElementAttribute());
}

Expand All @@ -736,7 +784,7 @@ TEST(Param, IgnoresParentElementAttribute)
// With parent using Set and SetFromString
sdf::Param doubleParam("key", "double", "1.0", false, "description");
sdf::ElementPtr parentElement = std::make_shared<sdf::Element>();
doubleParam.SetParentElement(parentElement);
ASSERT_TRUE(doubleParam.SetParentElement(parentElement));
ASSERT_TRUE(doubleParam.Set<double>(23.4));
EXPECT_TRUE(doubleParam.IgnoresParentElementAttribute());

Expand Down
34 changes: 11 additions & 23 deletions src/parser.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1405,36 +1405,24 @@ bool readXml(tinyxml2::XMLElement *_xml, ElementPtr _sdf,
{
sdf::ElementPtr poseElem = topLevelElem->GetElement("pose");

const char *relativeTo = poseElemXml->Attribute("relative_to");
if (relativeTo)
auto setAttribute =
[&poseElem, &poseElemXml](const std::string &_attribName)
{
poseElem->GetAttribute("relative_to")->SetFromString(relativeTo);
}
else
{
poseElem->GetAttribute("relative_to")->Reset();
}

const char *degrees = poseElemXml->Attribute("degrees");
{
if (degrees)
{
poseElem->GetAttribute("degrees")->SetFromString(degrees);
}
const char *attrib = poseElemXml->Attribute(_attribName.c_str());
if (attrib)
poseElem->GetAttribute(_attribName)->SetFromString(attrib);
else
{
poseElem->GetAttribute("degrees")->Reset();
}
}
poseElem->GetAttribute(_attribName)->Reset();
};

setAttribute("relative_to");
setAttribute("degrees");
setAttribute("rotation_format");

if (poseElemXml->GetText())
{
poseElem->GetValue()->SetFromString(poseElemXml->GetText());
}
else
{
poseElem->GetValue()->Reset();
}
}

if (isModel && elemXml->FirstChildElement("static"))
Expand Down
Loading

0 comments on commit 2059da8

Please sign in to comment.