Fix deeply nested merge-include for custom parsed files #1293

azeey · 2023-06-22T05:18:34Z

🦟 Bug fix

Note: Since there are relatively large changes in Model.cc, I've tried to organize the commits in logical order. It might be easier to review by commit rather than the whole change at once.

Summary

This addresses a bug where custom parsed models were not being processed if they happen to be nested two levels or more deep where one or more of those levels is also a merge-include. This was mainly because the include tags (which remain as include tags when they point to a custom parsed file) of a merge-included child model were not moved/merged into the parent model. Currently, we only merge the elements link, model, joint, frame, gripper and plugin:

sdformat/src/parser.cc

Lines 368 to 372 in b363e0e

    
           if ((elem->GetName() == "link") || (elem->GetName() == "model") || 
        
               (elem->GetName() == "joint") || (elem->GetName() == "frame") || 
        
               (elem->GetName() == "gripper") || (elem->GetName() == "plugin") || 
        
               (elem->GetName().find(':') != std::string::npos)) 
        
           {

However simply fixing by including include in that list exposed another problem. When processing merge-included files, we currently create a throwaway sdf::Root object, load the merge-included file as a standalone model and check that it's a valid model. However, if that model itself contains a custom-parsed model, the custom parser will end up being called during this validation step. Later on, when we create the final sdf::Root object, the custom parser would end up being called again. This would cause problems if the custom parser is storing some state about the models it processed, which is likely the case.

The approach I've taken here is to treat the custom-parser case as a special case and assume the DOM API will be used. And instead of trying to expand any include tag in parser.cc, we leave it as an include tag and process it in the Model or World classes. This would ensure that the custom parser will only be called once.

/cc @EricCousineau-TRI

Checklist

Signed all commits for DCO
Added tests
Updated documentation (as needed)
Updated migration guide (as needed)
Consider updating Python bindings (if the library has them)
codecheck passed (See contributing)
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 and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

codecov · 2023-06-22T05:48:10Z

Codecov Report

Merging #1293 (adaf2af) into sdf13 (7f63d02) will increase coverage by 0.07%.
The diff coverage is 94.79%.

❗ Current head adaf2af differs from pull request most recent head e44c813. Consider uploading reports for the commit e44c813 to get more accurate results

@@            Coverage Diff             @@
##            sdf13    #1293      +/-   ##
==========================================
+ Coverage   87.60%   87.68%   +0.07%     
==========================================
  Files         128      128              
  Lines       16823    16989     +166     
==========================================
+ Hits        14737    14896     +159     
- Misses       2086     2093       +7

Files Changed	Coverage Δ
src/Model.cc	`97.38% <94.44%> (-0.65%)`	⬇️
src/World.cc	`95.60% <94.82%> (-0.31%)`	⬇️
src/Utils.cc	`88.95% <100.00%> (+1.30%)`	⬆️
src/Utils.hh	`97.56% <100.00%> (+0.33%)`	⬆️
src/parser.cc	`86.85% <100.00%> (+0.05%)`	⬆️

scpeters

just noting some -Wpedantic complaints; I'm still reviewing

test/integration/interface_api.cc

src/parser.cc

test/integration/interface_api.cc

test/integration/model/merge_include_with_interface_api_1.sdf

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

This removes a check on the validity of a link pointer returned by a recursive call to Model::CanonicalLinkAndRelativeName because the pointer can be nullptr when the canonical link is an interface link. See gazebosim#544 Signed-off-by: Addisu Z. Taddese <[email protected]>

This is in preparation for the next change which adds model merging. Signed-off-by: Addisu Z. Taddese <[email protected]>

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

src/Model.cc

src/World.cc

scpeters · 2023-08-09T05:18:40Z

src/World.cc

-  Errors frameLoadErrors = loadUniqueRepeated<Frame>(_sdf, "frame",
-      this->dataPtr->frames);
-  errors.insert(errors.end(), frameLoadErrors.begin(), frameLoadErrors.end());
-
  // Check frames for name collisions and modify and warn if so.


not something new here, but I just noticed that this logic block always sets WARNING and doesn't check if (sdfVersion < gz::math::SemanticVersion(1, 7)) and set DUPLICATE_NAME for newer versions, like is done in Model.cc

Good catch. Should we address it in a separate PR?

yeah, that's fine

opened a draft PR in #1311

src/Model.cc

src/parser.cc

…rge_include

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

scpeters · 2023-08-21T07:35:59Z

src/World.cc

-  Errors frameLoadErrors = loadUniqueRepeated<Frame>(_sdf, "frame",
-      this->dataPtr->frames);
-  errors.insert(errors.end(), frameLoadErrors.begin(), frameLoadErrors.end());
-
  // Check frames for name collisions and modify and warn if so.


yeah, that's fine

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

github-actions bot added 🌱 garden Ignition Garden 🎵 harmonic Gazebo Harmonic labels Jun 22, 2023

azeey changed the title ~~Fix deeply nested merge include~~ Fix deeply nested merge-include for custom parsed files Jun 22, 2023

azeey self-assigned this Jun 22, 2023

EricCousineau-TRI mentioned this pull request Jul 13, 2023

[parsing] Add support for using merge-includes with custom parsed models RobotLocomotion/drake#16727

Merged

azeey force-pushed the fix_deeply_nested_merge_include branch from 34f71d7 to ed0477f Compare July 24, 2023 16:39

azeey marked this pull request as ready for review July 24, 2023 16:43

azeey requested a review from scpeters as a code owner July 24, 2023 16:43

scpeters reviewed Jul 24, 2023

View reviewed changes

test/integration/interface_api.cc Outdated Show resolved Hide resolved

test/integration/interface_api.cc Outdated Show resolved Hide resolved

scpeters reviewed Jul 26, 2023

View reviewed changes

src/parser.cc Outdated Show resolved Hide resolved

test/integration/interface_api.cc Show resolved Hide resolved

test/integration/interface_api.cc Show resolved Hide resolved

test/integration/model/merge_include_with_interface_api_1.sdf Outdated Show resolved Hide resolved

azeey added the beta Targeting beta release of upcoming collection label Jul 31, 2023

azeey added 5 commits August 4, 2023 15:55

Add failing tests

17bac81

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

Rework how we load elements in World::Load and Model::Load

b39ee4a

This is in preparation for the next change which adds model merging. Signed-off-by: Addisu Z. Taddese <[email protected]>

Handle deeply nested merged models when custom parsers are present

30b1fe9

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

Address review feedback.

e852813

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

azeey force-pushed the fix_deeply_nested_merge_include branch from 6786456 to e852813 Compare August 4, 2023 20:55

scpeters reviewed Aug 9, 2023

View reviewed changes

azeey added 2 commits August 18, 2023 21:35

Merge remote-tracking branch 'origin/sdf13' into fix_deeply_nested_me…

11f3b1d

…rge_include

Address reviewer feedback

b4c3522

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

scpeters approved these changes Aug 21, 2023

View reviewed changes

scpeters mentioned this pull request Aug 21, 2023

World: handle name collisions like Model #1311

Merged

8 tasks

azeey removed the 🎵 harmonic Gazebo Harmonic label Aug 22, 2023

Add comment regarding implicitFrameNames.

e44c813

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

scpeters approved these changes Aug 22, 2023

View reviewed changes

azeey merged commit e55378e into gazebosim:sdf13 Aug 22, 2023

azeey deleted the fix_deeply_nested_merge_include branch August 22, 2023 14:24

scpeters mentioned this pull request Aug 23, 2023

Merge sdf13 ➡️ main #1313

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix deeply nested merge-include for custom parsed files #1293

Fix deeply nested merge-include for custom parsed files #1293

azeey commented Jun 22, 2023 •

edited

Loading

codecov bot commented Jun 22, 2023 •

edited

Loading

scpeters left a comment

scpeters Aug 9, 2023

azeey Aug 19, 2023

scpeters Aug 21, 2023

scpeters Aug 21, 2023

scpeters Aug 21, 2023

	if ((elem->GetName() == "link") \|\| (elem->GetName() == "model") \|\|
	(elem->GetName() == "joint") \|\| (elem->GetName() == "frame") \|\|
	(elem->GetName() == "gripper") \|\| (elem->GetName() == "plugin") \|\|
	(elem->GetName().find(':') != std::string::npos))
	{

Fix deeply nested merge-include for custom parsed files #1293

Fix deeply nested merge-include for custom parsed files #1293

Conversation

azeey commented Jun 22, 2023 • edited Loading

🦟 Bug fix

Summary

Checklist

codecov bot commented Jun 22, 2023 • edited Loading

Codecov Report

scpeters left a comment

Choose a reason for hiding this comment

scpeters Aug 9, 2023

Choose a reason for hiding this comment

azeey Aug 19, 2023

Choose a reason for hiding this comment

scpeters Aug 21, 2023

Choose a reason for hiding this comment

scpeters Aug 21, 2023

Choose a reason for hiding this comment

scpeters Aug 21, 2023

Choose a reason for hiding this comment

azeey commented Jun 22, 2023 •

edited

Loading

codecov bot commented Jun 22, 2023 •

edited

Loading