Add support for merge-include in the Interface API #768

azeey · 2021-12-02T18:36:58Z

🎉 New feature

Closes #658

Summary

Builds on top of #764 to add support for merge-includes for custom parsers.

TODO

Handle placement frames.

Test it

Run the INTEGRATION_interface_api test.

Checklist

Signed all commits for DCO
Added tests
~~[] Added example and/or tutorial~~
Updated documentation (as needed)
Updated migration guide (as needed)
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

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

…antics

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

codecov-commenter · 2021-12-02T19:12:12Z

Codecov Report

Merging #768 (60708f0) into sdf12 (1d833c1) will increase coverage by 0.05%.
The diff coverage is 99.00%.

@@            Coverage Diff             @@
##            sdf12     #768      +/-   ##
==========================================
+ Coverage   90.71%   90.76%   +0.05%     
==========================================
  Files          78       78              
  Lines       12447    12535      +88     
==========================================
+ Hits        11291    11378      +87     
- Misses       1156     1157       +1

Impacted Files	Coverage Δ
include/sdf/InterfaceElements.hh	`66.66% <ø> (ø)`
src/Utils.hh	`94.73% <ø> (ø)`
src/Utils.cc	`83.57% <88.88%> (+0.36%)`	⬆️
src/FrameSemantics.cc	`84.03% <100.00%> (+1.01%)`	⬆️
src/InterfaceElements.cc	`100.00% <100.00%> (ø)`
src/InterfaceModel.cc	`100.00% <100.00%> (ø)`
src/Model.cc	`91.98% <100.00%> (+0.52%)`	⬆️
src/World.cc	`94.56% <100.00%> (ø)`
src/parser.cc	`87.58% <100.00%> (ø)`

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 1d833c1...60708f0. Read the comment docs.

EricCousineau-TRI · 2021-12-02T19:47:34Z

Per VC, in conjunction w/ #764, having frame graph construction be based solely on Interface API may help simplify both this and that PR.

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

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

…antics

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

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

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

scpeters · 2022-01-14T17:38:39Z

there are still a few test failures

EricCousineau-TRI

LGTM with minor nits.

Took me a while to fully grok some of the nested include tests, but they do test the full gamut - thanks for writing those!

EricCousineau-TRI · 2022-01-19T15:22:51Z

src/FrameSemantics.cc

+      for (const auto &item : model->Joints())
+      {
+        // TODO(azeey) In theory, the child could be "__model__" in which case,
+        // we'd have to use the proxy frame here, but not sure if "__model__" is


nit "not sure" seems like it could be confirmed; worth adding a test?

(But to be fair, an unscoped __model__ does seem like an awkward child)

I've confirmed that __model__ is not allowed in //joint/child and added a test in 09b6421.

looking at the error messaged added to the test in 09b6421, I believe the error is raised by the checkJointParentChildNames function in parser.cc. To be honest, I wouldn't take this as strict proof that __model__ is not allowed in //joint/child. I think our proposal is ambiguous about this, and if we modified the checkJointParentChildNames function to explicitly allow __model__, I think it would work. The same logic applies to __model__ as a parent frame as well.

If we want to disallow __model__ in //joint/parent and //joint/child, then let's be explicit about it

I just tested this and found out that the logic in checkJointParentChildNames also prevents a scoped __model__ from working. Since we allow __model__ in @relative_to and @attached_to, I think it makes sense that it should be allowed in both //joint/parent and //joint/child. I think it would be best for me to create another PR making that change and update this PR afterward. Thoughts?

sounds like a good plan

Reverted 09b6421 and made changes in 1991fbf to account for using __model__ in //joint/child in merge-included interface joints. Note, //joint/parent is not relevant because interface joints don't retain that information as it's not needed for building frame graphs.

include/sdf/InterfaceModel.hh

src/Model.cc

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

…rface_api

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

azeey

@scpeters I've addressed the test failures, PTAL. There's a CMake warning due to an infra issue (see gazebo-tooling/release-tools#625 (comment))

src/Model.cc

include/sdf/InterfaceModel.hh

azeey · 2022-01-20T22:31:07Z

src/FrameSemantics.cc

+      for (const auto &item : model->Joints())
+      {
+        // TODO(azeey) In theory, the child could be "__model__" in which case,
+        // we'd have to use the proxy frame here, but not sure if "__model__" is


I've confirmed that __model__ is not allowed in //joint/child and added a test in 09b6421.

scpeters

(But to be fair, an unscoped model does seem like an awkward child)

my github interface is weird right now; I can't reply to that other thread, but I had one more comment: the checkJointParentChildNames logic also disallows an unscoped __model__ as a joint parent, which strikes me as less awkward than as a child. So I'd rather make an affirmative decision about this rather than inferring intent from the existing code

src/FrameSemantics.cc

…rface_api

This reverts commit 09b6421. Signed-off-by: Addisu Z. Taddese <[email protected]>

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

scpeters

I've reviewed all but FrameSemantics.cc and the test. I'll finish reviewing next week. It's looking good. I just noticed one minor thing

src/Model.cc

scpeters

I've finished reviewing; I just have a few nits and then I'll be ready to approve

nit: I think the test/integration/model/joint_model_parent_or_child.toml file could be renamed? I'm not sure what the parent_or_child part of the name means

test/integration/interface_api.cc

test/sdf/merge_include_with_interface_api_frame_semantics.sdf

test/integration/interface_api.cc

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

azeey · 2022-02-02T17:37:45Z

I think the test/integration/model/joint_model_parent_or_child.toml file could be renamed? I'm not sure what the parent_or_child part of the name means

I renamed it to joint_child_model_frame.toml. At first, I thought I would be able to test the case where __model__ was in //joint/parent, but I realized that interface joints don't care about the parent.

osrf-triage · 2022-04-13T21:30:54Z

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-ignition-releases-2022-04-13-fortress-edifice/1367/1

azeey and others added 7 commits November 30, 2021 17:11

Refactor FrameSemantics.cc

6da93ae

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

Merge branch 'sdf12' into refactor_frame_semantics

d11402a

Merge remote-tracking branch 'upstream/sdf12' into refactor_frame_sem…

867422a

…antics

Fix MSVC compiler warnings

a46517a

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

Reviewer feedback

f2b2f47

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

Merge branch 'sdf12' into refactor_frame_semantics

56d7216

Add support for merge-include in the Interface API

e9507ad

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

azeey requested a review from EricCousineau-TRI December 2, 2021 18:36

azeey requested a review from scpeters as a code owner December 2, 2021 18:36

azeey self-assigned this Dec 2, 2021

github-actions bot added 🌱 garden Ignition Garden 🏯 fortress Ignition Fortress labels Dec 2, 2021

Handle placement frames

addddb9

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

azeey marked this pull request as draft December 6, 2021 19:17

azeey added 8 commits December 14, 2021 12:58

Use wrappers to create a common API on regular DOM objects and interf…

f3b0f93

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

Merge remote-tracking branch 'upstream/sdf12' into refactor_frame_sem…

f805d0b

…antics

Change name of helper build*Graph function

5b5c171

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

Merge branch 'refactor_frame_semantics' into merge_include_interface_api

bdbd692

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

Use new reworked refactor code for merge include

db4319e

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

Fix style, rename buildPoseRelativeToGraph to wrapperBuildPoseRelativ…

3ec800e

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

Merge branch 'refactor_frame_semantics' into merge_include_interface_api

97d8293

Style, documentation

1f9df5f

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

EricCousineau-TRI mentioned this pull request Jan 10, 2022

sdformat: Should allow merge include URDF files to be included via include tags RobotLocomotion/drake#15950

Open

Add comments

1818729

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

azeey marked this pull request as ready for review January 11, 2022 23:39

EricCousineau-TRI approved these changes Jan 19, 2022

View reviewed changes

azeey added 2 commits January 19, 2022 18:11

Fix test and compiler warning

399340c

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

Merge remote-tracking branch 'upstream/sdf12' into merge_include_inte…

a56a773

…rface_api

azeey mentioned this pull request Jan 20, 2022

Skip pxr for sdformat gazebo-tooling/release-tools#625

Merged

azeey added 2 commits January 20, 2022 16:27

Verify that //joint/child cannot be __model__

09b6421

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

Change ParserSupportsMergeInclude to bool, address reviewer feedback

1907e5d

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

azeey commented Jan 20, 2022

View reviewed changes

Merge branch 'sdf12' into merge_include_interface_api

c778b71

scpeters reviewed Jan 26, 2022

View reviewed changes

src/FrameSemantics.cc Outdated Show resolved Hide resolved

azeey mentioned this pull request Jan 27, 2022

Allow model frames (__model__) to be used as joint parent or child #833

Merged

5 tasks

azeey added 4 commits January 28, 2022 11:42

Merge remote-tracking branch 'upstream/sdf12' into merge_include_inte…

2ec9e98

…rface_api

Revert "Verify that //joint/child cannot be __model__"

2d16a82

This reverts commit 09b6421. Signed-off-by: Addisu Z. Taddese <[email protected]>

Handle __model__ as //joint/child in merge included interface joint

1991fbf

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

Remove unused header

74f06b7

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

scpeters reviewed Jan 29, 2022

View reviewed changes

src/Model.cc Outdated Show resolved Hide resolved

scpeters reviewed Feb 1, 2022

View reviewed changes

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

test/sdf/merge_include_with_interface_api_frame_semantics.sdf Show resolved Hide resolved

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

Address reviewer feedback

60708f0

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

scpeters approved these changes Feb 3, 2022

View reviewed changes

azeey merged commit 53b9d8e into gazebosim:sdf12 Feb 3, 2022

azeey deleted the merge_include_interface_api branch February 3, 2022 18:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for merge-include in the Interface API #768

Add support for merge-include in the Interface API #768

azeey commented Dec 2, 2021 •

edited

Loading

codecov-commenter commented Dec 2, 2021 •

edited

Loading

EricCousineau-TRI commented Dec 2, 2021

scpeters commented Jan 14, 2022

EricCousineau-TRI left a comment

EricCousineau-TRI Jan 19, 2022

azeey Jan 20, 2022

EricCousineau-TRI Jan 24, 2022

scpeters Jan 25, 2022

azeey Jan 26, 2022

azeey Jan 27, 2022

scpeters Jan 27, 2022

azeey Jan 28, 2022

azeey left a comment

azeey Jan 20, 2022

scpeters left a comment

scpeters left a comment

scpeters left a comment

azeey commented Feb 2, 2022

osrf-triage commented Apr 13, 2022

Add support for merge-include in the Interface API #768

Add support for merge-include in the Interface API #768

Conversation

azeey commented Dec 2, 2021 • edited Loading

🎉 New feature

Summary

TODO

Test it

Checklist

codecov-commenter commented Dec 2, 2021 • edited Loading

Codecov Report

EricCousineau-TRI commented Dec 2, 2021

scpeters commented Jan 14, 2022

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

azeey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scpeters left a comment

Choose a reason for hiding this comment

scpeters left a comment

Choose a reason for hiding this comment

scpeters left a comment

Choose a reason for hiding this comment

azeey commented Feb 2, 2022

osrf-triage commented Apr 13, 2022

azeey commented Dec 2, 2021 •

edited

Loading

codecov-commenter commented Dec 2, 2021 •

edited

Loading