adding element default check #542

marcoag · 2021-04-19T09:49:24Z

Signed-off-by: Marco A. Gutierrez [email protected]

🎉 New feature

Related to #287

Summary

There's already a check for the Attribute class so adding a default attribute and check functions for the Element class. To keep consistency with the code from in the Attribute class, we set it to true if the element was set false if it was created by default.

Since there's many cases where elements are added the base case considers that the element was not set by default so the value is true. When an attribute is being added by default, the SetSet(bool _value) should be called and the set attribute set to false, as done in parser.cc.

Thinks to take into account:

elementDescriptions will have the set attribute to true

Checklist

Signed all commits for DCO
Added tests
Added example and/or tutorial
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)

Signed-off-by: Marco A. Gutierrez <[email protected]>

azeey

Looks good! I have a few naming related comments and a request for additional testing.

include/sdf/Element.hh

src/Element_TEST.cc

Signed-off-by: Marco A. Gutierrez <[email protected]>

src/Element.cc

include/sdf/Element.hh

Signed-off-by: Marco A. Gutierrez <[email protected]>

azeey

Looks like codecheck is failing on github actions. Can you fix the style errors?

src/parser.cc

azeey · 2021-05-11T23:23:52Z

test/integration/default_elements.cc

+  sdf::ElementPtr element_ptr = root.Element();
+  EXPECT_TRUE(root.Element()->GetExplicitlySetInFile());
+
+  element_ptr = element_ptr->GetFirstElement();
+  EXPECT_TRUE(element_ptr->GetExplicitlySetInFile());
+
+  element_ptr = element_ptr->GetFirstElement();
+  EXPECT_TRUE(element_ptr->GetExplicitlySetInFile());
+
+  sdf::ElementPtr road_ptr = element_ptr->GetFirstElement();
+  EXPECT_FALSE(road_ptr->GetExplicitlySetInFile());
+
+  road_ptr = road_ptr->GetNextElement();
+  EXPECT_FALSE(road_ptr->GetExplicitlySetInFile());
+
+  element_ptr = element_ptr->GetNextElement();
+  EXPECT_TRUE(element_ptr->GetExplicitlySetInFile());
+
+  sdf::ElementPtr spherical_coords_ptr = element_ptr->GetFirstElement();
+  EXPECT_FALSE(spherical_coords_ptr->GetExplicitlySetInFile());
+
+  spherical_coords_ptr = element_ptr->GetNextElement();
+  EXPECT_FALSE(spherical_coords_ptr->GetExplicitlySetInFile());
+
+  spherical_coords_ptr = element_ptr->GetNextElement();
+  EXPECT_FALSE(spherical_coords_ptr->GetExplicitlySetInFile());
+
+  spherical_coords_ptr = element_ptr->GetNextElement();
+  EXPECT_FALSE(spherical_coords_ptr->GetExplicitlySetInFile());
+
+  spherical_coords_ptr = element_ptr->GetNextElement();
+  EXPECT_FALSE(spherical_coords_ptr->GetExplicitlySetInFile());
+
+  element_ptr = element_ptr->GetNextElement();
+  EXPECT_FALSE(element_ptr->GetExplicitlySetInFile());
+
+  element_ptr = element_ptr->GetNextElement();
+  EXPECT_FALSE(element_ptr->GetExplicitlySetInFile());
+
+  element_ptr = element_ptr->GetNextElement();
+  EXPECT_FALSE(element_ptr->GetExplicitlySetInFile());
+
+  element_ptr = element_ptr->GetNextElement();
+  EXPECT_FALSE(element_ptr->GetExplicitlySetInFile());
+
+  sdf::ElementPtr physics_ptr = element_ptr->GetFirstElement();
+  EXPECT_FALSE(physics_ptr->GetExplicitlySetInFile());
+
+  physics_ptr = physics_ptr->GetNextElement();
+  EXPECT_FALSE(physics_ptr->GetExplicitlySetInFile());
+
+  physics_ptr = physics_ptr->GetNextElement();
+  EXPECT_FALSE(physics_ptr->GetExplicitlySetInFile());
+
+  element_ptr = element_ptr->GetNextElement();
+  EXPECT_FALSE(element_ptr->GetExplicitlySetInFile());
+
+  sdf::ElementPtr scene_ptr = element_ptr->GetFirstElement();
+  EXPECT_FALSE(scene_ptr->GetExplicitlySetInFile());
+
+  scene_ptr = scene_ptr->GetNextElement();
+  EXPECT_FALSE(scene_ptr->GetExplicitlySetInFile());
+
+  scene_ptr = scene_ptr->GetNextElement();
+  EXPECT_FALSE(scene_ptr->GetExplicitlySetInFile());


Can this be refactored into a recursive function? Or is there a particular order of traversal of the elements that prevents generalization?

I believe it can't be generalized as I don't see a trivial way to figure out weather a certain element is expected to be ExplicitlySetInFile or not

include/sdf/Element.hh

src/Element.cc

Signed-off-by: Marco A. Gutierrez <[email protected]>

… sibling elements Signed-off-by: Addisu Z. Taddese <[email protected]>

Add failing test showing that SetExplicitlySetInFile incorrectly sets the flag on sibling elements

Signed-off-by: Marco A. Gutierrez <[email protected]>

aaronchongth

Sorry for the delay, this looks great! I haven't taken a detailed look at what default elements are getting added in during parsing of empty_road_sph_coords.sdf and empty_axis.sdf, but would it be a good idea to have the missing default elements added in as comments in the sdf file so we know what to expect? For example the physics element (I might be placing it wrong here),

<sdf ...>
<!-- <physics .../> -->
<world name="default">
  ...
</world>

Other than that, just a few minor comments about the tests and test files below, thanks!

src/Element_TEST.cc

test/integration/default_elements.cc

test/sdf/empty_road_sph_coords.sdf

Signed-off-by: Marco A. Gutierrez <[email protected]>

aaronchongth

Thanks for making the changes!

EricCousineau-TRI · 2021-06-03T18:10:40Z

FYI @SeanCurtis-TRI

…zebosim#542)" This reverts commit 9876a60. Signed-off-by: Addisu Z. Taddese <[email protected]>

There's already a way to do this for the Attribute class so adding a similar function for the Element class. To keep consistency with the code from in the Attribute class, we set it to true if the element was set, and false if it was created by default. Signed-off-by: Marco A. Gutierrez <[email protected]> Co-authored-by: Addisu Z. Taddese <[email protected]> Signed-off-by: Jenn Nguyen <[email protected]>

adding element default check

d50c89d

Signed-off-by: Marco A. Gutierrez <[email protected]>

github-actions bot added the 🏰 citadel Ignition Citadel label Apr 19, 2021

azeey reviewed Apr 23, 2021

View reviewed changes

include/sdf/Element.hh Outdated Show resolved Hide resolved

include/sdf/Element.hh Outdated Show resolved Hide resolved

include/sdf/Element.hh Outdated Show resolved Hide resolved

src/Element_TEST.cc Outdated Show resolved Hide resolved

Marco A. Gutierrez added 2 commits May 7, 2021 10:21

renaming variables and integration test

233a922

Signed-off-by: Marco A. Gutierrez <[email protected]>

checking for error on load

0a3dca6

Signed-off-by: Marco A. Gutierrez <[email protected]>

marcoag force-pushed the marcoag/add_default_check branch from 4c88ec6 to 0a3dca6 Compare May 7, 2021 02:23

Merge branch 'sdf9' into marcoag/add_default_check

18c9bb2

azeey requested changes May 10, 2021

View reviewed changes

src/Element.cc Outdated Show resolved Hide resolved

include/sdf/Element.hh Outdated Show resolved Hide resolved

azeey requested a review from aaronchongth May 10, 2021 22:46

Marco A. Gutierrez added 3 commits May 11, 2021 08:40

fix typo and added test for child defult elements

c1953c8

Signed-off-by: Marco A. Gutierrez <[email protected]>

rename set flag since it's only related to parsing

f21a958

Signed-off-by: Marco A. Gutierrez <[email protected]>

adding test for default elements in copy and clone

441713b

Signed-off-by: Marco A. Gutierrez <[email protected]>

azeey requested changes May 11, 2021

View reviewed changes

code clean, extra tests for SetExplicitlySetInFile

52cd85f

Signed-off-by: Marco A. Gutierrez <[email protected]>

marcoag requested a review from azeey May 12, 2021 08:06

Marco A. Gutierrez and others added 5 commits May 12, 2021 16:25

removing whitespaces

051cd48

Signed-off-by: Marco A. Gutierrez <[email protected]>

Add failing test showing that SetExplicitlySetInFile sets the flag on…

23e9447

… sibling elements Signed-off-by: Addisu Z. Taddese <[email protected]>

Merge pull request #1 from azeey/suggestion_add_default_check

a8699ac

Add failing test showing that SetExplicitlySetInFile incorrectly sets the flag on sibling elements

refactor SetExplicitlySetInFile to avoid sibblings

369ea0d

Signed-off-by: Marco A. Gutierrez <[email protected]>

style fixes

bbaef61

Signed-off-by: Marco A. Gutierrez <[email protected]>

aaronchongth requested changes May 17, 2021

View reviewed changes

Marco A. Gutierrez added 3 commits May 17, 2021 06:52

adding default elements as comments

64f226f

Signed-off-by: Marco A. Gutierrez <[email protected]>

test for adding child to default element

4fcfa21

Signed-off-by: Marco A. Gutierrez <[email protected]>

rename ptr variables for clarity

761f7bc

Signed-off-by: Marco A. Gutierrez <[email protected]>

marcoag requested a review from aaronchongth May 17, 2021 07:54

aaronchongth approved these changes May 17, 2021

View reviewed changes

azeey approved these changes May 18, 2021

View reviewed changes

azeey merged commit 9876a60 into gazebosim:sdf9 May 18, 2021

marcoag deleted the marcoag/add_default_check branch May 19, 2021 07:50

azeey mentioned this pull request May 20, 2021

Making PrintValues() and ToString() able to not print default elements #575

Merged

8 tasks

aaronchongth mentioned this pull request Jun 2, 2021

New pose proposal (#252) #581

Closed

8 tasks

scpeters mentioned this pull request Jun 28, 2021

Gazebo crashes with Edifice binaries gazebosim/gz-sim#891

Closed

azeey added a commit to azeey/sdformat that referenced this pull request Jun 28, 2021

Revert "Add API for determining if an element was set by the user (ga…

268b581

…zebosim#542)" This reverts commit 9876a60. Signed-off-by: Addisu Z. Taddese <[email protected]>

chapulina mentioned this pull request Jun 28, 2021

Fix ABI break #605

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adding element default check #542

adding element default check #542

marcoag commented Apr 19, 2021 •

edited

Loading

azeey left a comment

azeey left a comment

azeey May 11, 2021

marcoag May 12, 2021

aaronchongth left a comment

aaronchongth left a comment

EricCousineau-TRI commented Jun 3, 2021

adding element default check #542

adding element default check #542

Conversation

marcoag commented Apr 19, 2021 • edited Loading

🎉 New feature

Summary

Checklist

azeey left a comment

Choose a reason for hiding this comment

azeey left a comment

Choose a reason for hiding this comment

azeey May 11, 2021

Choose a reason for hiding this comment

marcoag May 12, 2021

Choose a reason for hiding this comment

aaronchongth left a comment

Choose a reason for hiding this comment

aaronchongth left a comment

Choose a reason for hiding this comment

EricCousineau-TRI commented Jun 3, 2021

marcoag commented Apr 19, 2021 •

edited

Loading