Add support for merge-include in the Interface API #768
Conversation
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
azeey
added this to In progress
in SDFormat Development TRI (Moved to https://github.com/orgs/gazebosim/projects/2)
via automation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
EricCousineau-TRI
moved this from In progress
to In Review
in SDFormat Development TRI (Moved to https://github.com/orgs/gazebosim/projects/2)
|
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 <addisu@openrobotics.org>
…ace elements Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
…eToGraph Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
|
there are still a few test failures |
src/FrameSemantics.cc
Outdated
| 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 |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
I've confirmed that __model__ is not allowed in //joint/child and added a test in 09b6421.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
There was a problem hiding this comment.
@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/FrameSemantics.cc
Outdated
| 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 |
There was a problem hiding this comment.
I've confirmed that __model__ is not allowed in //joint/child and added a test in 09b6421.
There was a problem hiding this comment.
(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
This reverts commit 09b6421. Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
I renamed it to |
|
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 |
j-rivero
removed this from Done
in SDFormat Development TRI (Moved to https://github.com/orgs/gazebosim/projects/2)
🎉 New feature
Closes #658
Summary
Builds on top of #764 to add support for merge-includes for custom parsers.
TODO
Test it
Run the
INTEGRATION_interface_apitest.Checklist
[] Added example and/or tutorialcodecheckpassed (See contributing)Note to maintainers: Remember to use Squash-Merge