Unflatten flattened models #504

jennuine · 2021-03-03T22:18:31Z

Closes #411

Summary

Implemented conversion from SDFormat 1.7 to 1.8 which takes the flattened XML and infers when a new model should be created/unflattened.

Checklist

Signed all commits for DCO
Added tests
Updated documentation (as needed)
Updated migration guide (as needed)
Code check passed (In source directory, run sh tools/code_check.sh)
All tests passed (See test coverage)
While waiting for a review on your PR, please help review
another open pull request
to support the maintainers

Note to maintainers: Remember to use Squash-Merge

Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>

azeey

Did a first pass. Looks pretty good! I left some comments related to performance and handling more SDFormat tags in //model.

sdf/1.8/1_7.convert

src/Converter.cc

azeey

There are some test failures in INTEGRATION_nested_model. Could you update the todos and test expectations in NestedModel.PartiallyFlattened can be to account for the changes in this PR?

src/Converter.cc

Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>

jennuine · 2021-03-09T20:53:15Z

There are some test failures in INTEGRATION_nested_model. Could you update the todos and test expectations in NestedModel.PartiallyFlattened can be to account for the changes in this PR?

Done a1006ad

Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>

codecov-io · 2021-03-10T01:15:10Z

Codecov Report

Merging #504 (33dda02) into master (3fb5312) will increase coverage by 0.09%.
The diff coverage is 94.16%.

@@            Coverage Diff             @@
##           master     #504      +/-   ##
==========================================
+ Coverage   87.81%   87.90%   +0.09%     
==========================================
  Files          66       66              
  Lines        9321     9458     +137     
==========================================
+ Hits         8185     8314     +129     
- Misses       1136     1144       +8

Impacted Files	Coverage Δ
src/Converter.cc	`92.27% <94.16%> (+0.70%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3fb5312...33dda02. Read the comment docs.

test/integration/nested_model.cc

src/Converter.cc

Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>

azeey · 2021-03-11T17:04:45Z

src/Converter.cc

+        (elemName != "frame" && elemName != "joint" &&
+        elemName != "link" && elemName != "model"))
+    {
+      if (elemName != "gripper")


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

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

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.

How's this? 3e0cd5d

src/Converter.cc

EricCousineau-TRI

Looks good thus far! Added some nits and some minor blockers in addition to Addisu's comments.

EricCousineau-TRI · 2021-03-11T18:32:06Z

src/Converter.cc

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


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

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

EricCousineau-TRI · 2021-03-11T18:32:46Z

sdf/1.8/1_7.convert

@@ -1,3 +1,9 @@
 <convert name="sdf">


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

Removed 3e0cd5d

EricCousineau-TRI · 2021-03-11T18:33:56Z

src/Converter.cc

+
+    if (elemAttrName.empty() ||
+        elemAttrName.compare(0, _newNameIdx - 2, newModelName) != 0 ||
+        (elemName != "frame" && elemName != "joint" &&


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" || ...; }

EricCousineau-TRI · 2021-03-11T18:35:41Z

src/Converter.cc

+      elemAttrName = elem->Attribute("name");
+
+    if (elemAttrName.empty() ||
+        elemAttrName.compare(0, _newNameIdx - 2, newModelName) != 0 ||


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;

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

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

Excellent, thanks!

EricCousineau-TRI · 2021-03-11T18:36:54Z

src/Converter.cc

+      tinyxml2::XMLElement *e = elem->FirstChildElement("parent");
+      std::string eText = e->GetText();
+
+      SDF_ASSERT(eText.compare(0, _newNameIdx - 2, newModelName) == 0,


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

EricCousineau-TRI · 2021-03-11T18:38:23Z

src/Converter_TEST.cc

+  xmlDoc.Clear();
+  xmlDoc.Parse(xmlString.c_str());
+
+  sdf::Converter::Convert(&xmlDoc, &convertXmlDoc);


For readability, could you also add a mutliline test on generated XML?

While the below EXPECT* statements reallllly drill into semantics, it makes it a tad harder to quickly assess that the spec example is fully represented w/ this implementation.

Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>

azeey

This is really close! Just a few more minor comments

src/Converter.hh

azeey · 2021-03-18T16:35:52Z

src/Converter.cc

+        (elemName != "frame" && elemName != "joint" &&
+        elemName != "link" && elemName != "model"))
+    {
+      if (elemName != "gripper")


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.

src/Converter.cc

src/Converter_TEST.cc

test/integration/converter.cc

Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>

azeey

This looks great! Thanks for iterating.

EricCousineau-TRI · 2021-03-19T14:58:23Z

Indeed! Thank you!

jennuine added 2 commits March 3, 2021 14:06

initial unnesting conversion

a95b182

Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>

Merge branch 'master' into jennuine/unnesting

320d4b1

Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>

jennuine requested a review from azeey March 3, 2021 22:18

github-actions bot added the 🏢 edifice Ignition Edifice label Mar 3, 2021

unnested links & added integration test

e128b71

Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>

jennuine marked this pull request as ready for review March 5, 2021 01:32

azeey requested changes Mar 8, 2021

View reviewed changes

azeey reviewed Mar 8, 2021

View reviewed changes

src/Converter.cc Show resolved Hide resolved

updated nested model test

a1006ad

Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>

jennuine added 3 commits March 9, 2021 13:01

Merge branch 'master' into jennuine/unnesting

de8a697

Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>

strip prefix for //link/<element>/pose & //joint/sensor/pose

1435eab

Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>

strip prefix for //model/include/pose & //model/gripper

8cd7f4d

Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>

azeey reviewed Mar 10, 2021

View reviewed changes

test/integration/nested_model.cc Show resolved Hide resolved

test/integration/nested_model.cc Show resolved Hide resolved

test/integration/nested_model.cc Show resolved Hide resolved

src/Converter.cc Outdated Show resolved Hide resolved

src/Converter.cc Show resolved Hide resolved

jennuine added 2 commits March 10, 2021 18:12

Removed use of copied element to unnest

cceaaa4

Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>

Fixed elem iteration (removed DeepClone)

33dda02

Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>

azeey reviewed Mar 11, 2021

View reviewed changes

EricCousineau-TRI reviewed Mar 11, 2021

View reviewed changes

changed unnest to unflatten & added expectations back

dce6828

Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>

jennuine changed the title ~~Unnesting flattened models~~ Unflatten flattened models Mar 12, 2021

jennuine added 2 commits March 12, 2021 16:32

updated tests

9ed2713

Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>

readded some tests back

73cbe30

Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>

azeey added this to In progress in SDFormat Development TRI (Moved to https://github.com/orgs/ignitionrobotics/projects/6) via automation Mar 17, 2021

azeey moved this from In progress to In Review in SDFormat Development TRI (Moved to https://github.com/orgs/ignitionrobotics/projects/6) Mar 17, 2021

jennuine mentioned this pull request Mar 17, 2021

Error when delimiter "::" found in element name in SDFormat 1.8 #515

Merged

7 tasks

chapulina added the beta Targeting beta release of upcoming collection label Mar 18, 2021

azeey reviewed Mar 18, 2021

View reviewed changes

jennuine added 2 commits March 18, 2021 13:33

Merge branch 'master' into jennuine/unnesting

8dfcd7f

Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>

renamed function & strip //gripper/@name if prefix present

3e0cd5d

Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>

azeey approved these changes Mar 18, 2021

View reviewed changes

jennuine merged commit 1333f59 into master Mar 18, 2021

SDFormat Development TRI (Moved to https://github.com/orgs/ignitionrobotics/projects/6) automation moved this from In Review to Done Mar 18, 2021

jennuine deleted the jennuine/unnesting branch March 18, 2021 23:10

nkoenig removed this from Done in SDFormat Development TRI (Moved to https://github.com/orgs/ignitionrobotics/projects/6) Jun 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unflatten flattened models #504

Unflatten flattened models #504

jennuine commented Mar 3, 2021 •

edited

Loading

azeey left a comment

azeey left a comment

jennuine commented Mar 9, 2021

codecov-io commented Mar 10, 2021 •

edited

Loading

azeey Mar 11, 2021

jennuine Mar 12, 2021

azeey Mar 18, 2021

jennuine Mar 18, 2021

EricCousineau-TRI left a comment

EricCousineau-TRI Mar 11, 2021

jennuine Mar 13, 2021

EricCousineau-TRI Mar 11, 2021

jennuine Mar 18, 2021

EricCousineau-TRI Mar 11, 2021

jennuine Mar 13, 2021

EricCousineau-TRI Mar 11, 2021

EricCousineau-TRI Mar 11, 2021

jennuine Mar 13, 2021

EricCousineau-TRI Mar 19, 2021

EricCousineau-TRI Mar 11, 2021

jennuine Mar 13, 2021

EricCousineau-TRI Mar 11, 2021

jennuine Mar 13, 2021

azeey left a comment

azeey Mar 18, 2021

azeey left a comment

EricCousineau-TRI commented Mar 19, 2021

Unflatten flattened models #504

Unflatten flattened models #504

Conversation

jennuine commented Mar 3, 2021 • edited Loading

Summary

Checklist

azeey left a comment

Choose a reason for hiding this comment

azeey left a comment

Choose a reason for hiding this comment

jennuine commented Mar 9, 2021

codecov-io commented Mar 10, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azeey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azeey left a comment

Choose a reason for hiding this comment

EricCousineau-TRI commented Mar 19, 2021

jennuine commented Mar 3, 2021 •

edited

Loading

codecov-io commented Mar 10, 2021 •

edited

Loading