Allow only one of actor/light/model for <include> tags #433

brawner · 2020-12-09T01:15:00Z

This is a first take on updating sdformat and libsdformat to allow included files to only contain one element of type actor/light/model.

The related issue (#395) explains in better detail, but essentially because the include tag can override attributes and elements like //include/name, //include/pose it should only be asked to include sdf files with one element to avoid ambiguity.

Added models for testing included in 3271f33
Modified spec and logic in 3a3a947

Fixes: #395

brawner · 2020-12-09T01:24:55Z

Force pushed to fix linter issues

azeey

Looks great! I left mostly minor nitpick comments and asked for an additional test.

test/integration/model/model_actor_light/model.sdf

test/integration/nested_multiple_elements_error.cc

azeey · 2020-12-11T20:35:24Z

test/integration/nested_multiple_elements_error.cc

+    sdf::filesystem::append(g_modelsPath, "nested_multiple_models_error");
+
+  sdf::Root root;
+  sdf::Errors errors = root.Load(worldFile);


Technically, this (nested_multiple_models_error/model.sdf) is not a world file, it's a model that is including other resources. Could you add a test where you have something like this:

<?xml version="1.0" ?> <sdf version="1.8"> <world name="world_multiple_models_error"> <include>  <uri>multiple_models</uri> <pose>1 1 1 1.570796326794895 0 0</pose> </include> </world> </sdf>

I think the include mechanism is the same for both worlds and models, but it would be nice to cover both in our tests.

Added nested_multiple_elements_error_world

azeey · 2020-12-11T20:36:18Z

test/integration/nested_multiple_elements_error.cc

+}
+
+//////////////////////////////////////////////////
+TEST(IncludesTest, nested_multiple_models_error)


nit: I believe test names use CamelCase.

test/integration/model/multiple_lights/model.sdf

azeey · 2020-12-11T22:56:22Z

test/integration/model/nested_multiple_elements_error_world/model.config

@@ -0,0 +1,6 @@
+<?xml version="1.0"?>


I don't think you need this file

Removed and moved the sdf file to the sdf directory under nested_multilpe_elements_error_world.sdf

azeey · 2020-12-11T22:58:05Z

test/integration/model/nested_multiple_elements_error_world/model.sdf

@@ -0,0 +1,15 @@
+<?xml version="1.0" ?>
+<sdf version="1.8">


I think this file can be a standalone file in /test/sdf since it represents a world. A model.config file in a directory is used for models.

azeey · 2020-12-15T18:19:03Z

src/parser.cc

        {
-          topLevelElem = includeSDF->Root()->GetElement("light");
+          _errors.push_back({ErrorCode::ELEMENT_INVALID,


Per our discussion the other day, I think we should make this a warning for libsdformat11. Since we don't have a way to include warnings in sdf::Errors, we would just print a warning message via sdfwarn.

After some rebasing, I now have this added.

brawner · 2020-12-15T21:29:56Z

I broke out the changes to sdf::Root in #444

codecov-io · 2020-12-15T22:19:04Z

Codecov Report

Merging #433 (89ed957) into master (28e16c8) will increase coverage by 0.18%.
The diff coverage is 94.44%.

@@            Coverage Diff             @@
##           master     #433      +/-   ##
==========================================
+ Coverage   87.52%   87.71%   +0.18%     
==========================================
  Files          62       63       +1     
  Lines        9542     9721     +179     
==========================================
+ Hits         8352     8527     +175     
- Misses       1190     1194       +4

Impacted Files	Coverage Δ
src/parser.cc	`79.41% <94.44%> (+0.27%)`	⬆️
src/Capsule.cc	`96.61% <0.00%> (ø)`
src/Actor.cc	`86.09% <0.00%> (+0.29%)`	⬆️
src/Geometry.cc	`93.64% <0.00%> (+0.64%)`	⬆️
src/Cylinder.cc	`95.91% <0.00%> (+2.81%)`	⬆️

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 28e16c8...89ed957. Read the comment docs.

Signed-off-by: Stephen Brawner <[email protected]>

azeey · 2020-12-22T23:28:56Z

Please use squash and merge.

brawner · 2020-12-22T23:35:09Z

Thanks for the review @azeey, squashed and merged!

#619) #433 and #444 made it so that only one root level model, actor or light can be contained in an SDFormat file. So as to not break existing behavior, only a warning was issued if more than one is found. These have now been converted to errors. This PR also removes deprecated functions for accessing multiple model, actor or light elements from sdf::Root. Signed-off-by: Addisu Z. Taddese <[email protected]>

brawner requested a review from azeey December 9, 2020 01:15

brawner force-pushed the brawner/include-one-model-sdfs branch 2 times, most recently from b84e9e5 to 3a3a947 Compare December 9, 2020 01:24

azeey requested changes Dec 11, 2020

View reviewed changes

brawner marked this pull request as ready for review December 11, 2020 22:32

brawner force-pushed the brawner/include-one-model-sdfs branch from 9e58b53 to 11d8996 Compare December 11, 2020 22:32

azeey reviewed Dec 11, 2020

View reviewed changes

azeey reviewed Dec 15, 2020

View reviewed changes

brawner force-pushed the brawner/include-one-model-sdfs branch 3 times, most recently from 64f8ccc to 5a424bd Compare December 15, 2020 21:19

brawner mentioned this pull request Dec 15, 2020

Set sdf::Root to contain only one model/actor/light #444

Merged

brawner force-pushed the brawner/include-one-model-sdfs branch from 3a79445 to 89ed957 Compare December 15, 2020 22:00

azeey added this to the SDFormat 1.8 / libsdformat11 milestone Dec 17, 2020

brawner added 7 commits December 21, 2020 13:34

Add sdf files for testing

8def4bc

Signed-off-by: Stephen Brawner <[email protected]>

Check that included file only contains one of actor/light/model

80b462a

Signed-off-by: Stephen Brawner <[email protected]>

Address PR feedback and add test

4e9f4b9

Signed-off-by: Stephen Brawner <[email protected]>

Update spec to disallow multiple actors/lights/models under sdf root

37539cf

Signed-off-by: Stephen Brawner <[email protected]>

Update sdfs and parser for warnings

cdb682e

Signed-off-by: Stephen Brawner <[email protected]>

Linter cleanup

bfb2c81

Signed-off-by: Stephen Brawner <[email protected]>

Slight cleanup

7a019d3

Signed-off-by: Stephen Brawner <[email protected]>

brawner force-pushed the brawner/include-one-model-sdfs branch from 89ed957 to 7a019d3 Compare December 21, 2020 21:37

brawner requested a review from azeey December 21, 2020 21:39

azeey approved these changes Dec 22, 2020

View reviewed changes

brawner merged commit e0e09f9 into master Dec 22, 2020

brawner deleted the brawner/include-one-model-sdfs branch December 22, 2020 23:33

azeey mentioned this pull request Jul 9, 2021

Emit an error instead of a warning when a file has multiple root level model, actor or light elements #619

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow only one of actor/light/model for <include> tags #433

Allow only one of actor/light/model for <include> tags #433

brawner commented Dec 9, 2020 •

edited

Loading

brawner commented Dec 9, 2020

azeey left a comment

azeey Dec 11, 2020

brawner Dec 11, 2020

azeey Dec 11, 2020

azeey Dec 11, 2020

brawner Dec 11, 2020

azeey Dec 11, 2020

brawner Dec 11, 2020

azeey Dec 15, 2020

brawner Dec 15, 2020

brawner commented Dec 15, 2020

codecov-io commented Dec 15, 2020 •

edited

Loading

azeey commented Dec 22, 2020

brawner commented Dec 22, 2020

Allow only one of actor/light/model for <include> tags #433

Allow only one of actor/light/model for <include> tags #433

Conversation

brawner commented Dec 9, 2020 • edited Loading

brawner commented Dec 9, 2020

azeey 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

brawner commented Dec 15, 2020

codecov-io commented Dec 15, 2020 • edited Loading

Codecov Report

azeey commented Dec 22, 2020

brawner commented Dec 22, 2020

brawner commented Dec 9, 2020 •

edited

Loading

codecov-io commented Dec 15, 2020 •

edited

Loading