Implement SDFormat 1.8 Interface API #475

azeey · 2021-01-22T17:36:02Z

This implements the SDFormat 1.8 Interface API as proposed in http://sdformat.org/tutorials?tut=composition_proposal&cat=pose_semantics_docs&#1-5-minimal-libsdformat-interface-types-for-non-sdformat-models.

Needs #439 and #474.

Closes #430

This change is

The error message assumes model uri's always start with "model://". This is no longer the case. Signed-off-by: Addisu Z. Taddese <[email protected]>

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

The error message assumes model uri's always start with "model://". This is no longer the case. Signed-off-by: Addisu Z. Taddese <[email protected]>

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

//include/uri used to only take directories that contain a model.config file and the sdformat file. However, allowing //include/uri to take file names can be supported without loss of functionality. Signed-off-by: Addisu Z. Taddese <[email protected]>

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

scpeters

Reviewed 1 of 11 files at r5, 8 of 27 files at r7, 8 of 19 files at r8.
Reviewable status: 32 of 37 files reviewed, 22 unresolved discussions (waiting on @azeey, @EricCousineau-TRI, and @jennuine)

include/sdf/InterfaceElements.hh, line 41 at r7 (raw file):

Previously, jennuine (Jenn Nguyen) wrote…

  /// not end with a file extension (it will not end with an extension if it refers

+1

include/sdf/InterfaceElements.hh, line 59 at r7 (raw file):

Previously, jennuine (Jenn Nguyen) wrote…

Should this be //include/name (removing @) since it is an element not an attribute? Also, should we specify what localModelName would be if //include/name is not set?

+1 to removing @

it currently says This is nullopt if //include/name is not set. I believe the name is then inferred from the URI in some way?

include/sdf/InterfaceElements.hh, line 73 at r8 (raw file):

  /// \brief The relative-to frame of the pose as specified in
  /// `//include/pose/@relative_to`. This is nullopt if
  /// `//include/pose/@relative_to` is set.

is not set?

scpeters

Reviewed 1 of 19 files at r8.
Reviewable status: 33 of 37 files reviewed, 23 unresolved discussions (waiting on @azeey, @EricCousineau-TRI, and @jennuine)

src/Utils.cc, line 223 at r8 (raw file):

      auto poseElem = includeElem->GetElement("pose");
      include.includeRawPose = poseElem->Get<ignition::math::Pose3d>();
      include.includePoseRelativeTo = poseElem->Get<std::string>("relative_to");

not sure if you should check HasAttribute("relative_to") here

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

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

azeey

Reviewable status: 28 of 38 files reviewed, 20 unresolved discussions (waiting on @azeey, @EricCousineau-TRI, @jennuine, and @scpeters)

include/sdf/InterfaceElements.hh, line 73 at r8 (raw file):

Previously, scpeters (Steve Peters) wrote…

is not set?

Done in 154b845

include/sdf/InterfaceJoint.hh, line 40 at r7 (raw file):

Previously, scpeters (Steve Peters) wrote…

good question. I don't think so, because joint frames are always attached to the child link:

http://sdformat.org/tutorials?tut=pose_frame_semantics&cat=specification&#implicit-and-explicit-frames

Yeah, the only reason we need Joints in the Interface API is to build the frame graphs and the parent link doesn't play a role in that.

include/sdf/Model.hh, line 305 at r7 (raw file):

Previously, jennuine (Jenn Nguyen) wrote…

Does this TODO need to be addressed for this PR?

I don't know how to address this right now, so I'm thinking of punting. The problem is, if the canonical link is an interface link, as opposed to a regular link, there is no way for this function to return the interface link. I suppose the return type can be std::pair<std::variant<const Link *, InterfaceLink>, std::string>, but that feels unwieldy.

include/sdf/World.hh, line 46 at r8 (raw file):

Previously, scpeters (Steve Peters) wrote…

nit: struct to fix some windows warnings

Done in 154b845

src/Utils.cc, line 223 at r8 (raw file):

Previously, scpeters (Steve Peters) wrote…

not sure if you should check HasAttribute("relative_to") here

Added the check in 154b845

test/integration/interface_api.cc, line 17 at r7 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Can you add a brief overview to this file? e.g.

/// \file
/// Provides a simple usage of the interface API by parsing models described in TOML (https://toml.io/en/).

Done in d832ed3

test/integration/interface_api.cc, line 39 at r7 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit For clarity, maybe worth splitting TOML parsing logic into separate file under same folder?

Done in d832ed3

test/integration/interface_api.cc, line 433 at r7 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit This shows ordering, but doesn't show (a) success or (b) what happens with "conflicting" / "intersecting" parsers (e.g. last one wins)?

Can you capture that here?

Done in 90cafa2

scpeters

Reviewed 3 of 19 files at r8, 8 of 8 files at r9.
Reviewable status: all files reviewed, 19 unresolved discussions (waiting on @azeey, @EricCousineau-TRI, and @jennuine)

include/sdf/ParserConfig.hh, line 148 at r7 (raw file):

Previously, azeey (Addisu Z. Taddese) wrote…

Done in 154b845

"as as"

src/FrameSemantics.cc, line 511 at r8 (raw file):

      else
      {
        // Search for the vertex in case the canoncal link is an InterfaceLink

canoncal -> canonical

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

azeey

Reviewable status: 33 of 38 files reviewed, 18 unresolved discussions (waiting on @azeey, @EricCousineau-TRI, @jennuine, and @scpeters)

include/sdf/InterfaceElements.hh, line 82 at r7 (raw file):

Previously, azeey (Addisu Z. Taddese) wrote…

Working: Good point!

Done in c0af826

include/sdf/ParserConfig.hh, line 148 at r7 (raw file):

Previously, scpeters (Steve Peters) wrote…

"as as"

Fixed in 93f5977

src/FrameSemantics.cc, line 511 at r8 (raw file):

Previously, scpeters (Steve Peters) wrote…

canoncal -> canonical

Done in 93f5977

scpeters

Reviewed 5 of 5 files at r10.
Reviewable status: all files reviewed, 18 unresolved discussions (waiting on @azeey, @EricCousineau-TRI, and @jennuine)

a discussion (no related file):
this all looks good to me; we just need to figure out the windows compiler warnings:

perhaps we can disable C4251 warnings project-wide in this PR with a TODO to revisit with a more proper solution

scpeters · 2021-03-25T04:17:54Z

this all looks good to me; we just need to figure out the windows compiler warnings:

perhaps we can disable C4251 warnings project-wide in this PR with a TODO to revisit with a more proper solution

example from ign-rendering:

https://github.com/ignitionrobotics/ign-rendering/blob/ignition-rendering4_4.7.0/ogre2/src/CMakeLists.txt#L5-L12

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

azeey · 2021-03-25T04:30:47Z

a discussion (no related file):

Previously, scpeters (Steve Peters) wrote…

this all looks good to me; we just need to figure out the windows compiler warnings:

perhaps we can disable C4251 warnings project-wide in this PR with a TODO to revisit with a more proper solution

Ignoring C4251 for NestedInclude in 1676e5d since all the warnings seem to come from there.

azeey · 2021-03-25T04:46:53Z

a discussion (no related file):

Previously, azeey (Addisu Z. Taddese) wrote…

Ignoring C4251 for NestedInclude in 1676e5d since all the warnings seem to come from there.

That worked!

jennuine

LGTM! May want to wait for @scpeters to confirm

scpeters

Reviewed 1 of 1 files at r11.
Reviewable status: all files reviewed, 18 unresolved discussions (waiting on @azeey, @EricCousineau-TRI, and @jennuine)

a discussion (no related file):

Previously, azeey (Addisu Z. Taddese) wrote…

That worked!

🎊

EricCousineau-TRI

Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @azeey and @jennuine)

a discussion (no related file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Do you happen to have an integration PR with Drake based on this work?

OK Dismissing myself here - we can spike test Drake integ after Edifice release!

include/sdf/InterfaceElements.hh, line 82 at r7 (raw file):

Previously, azeey (Addisu Z. Taddese) wrote…

Done in c0af826

OK Thanks!

include/sdf/InterfaceElements.hh, line 115 at r7 (raw file):

Previously, azeey (Addisu Z. Taddese) wrote…

Done in d773f70

OK Sweet!

src/FrameSemantics.cc, line 539 at r7 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit Dumb high-level Q:

Does this mean that all relevant posturing is now maintained for both proper elements + interface elements?

Did we already discuss implications of having the interface element "shadows" for proper elements, and then the graph only needs to reflect interface elements?

OK Dismissing myself for now.

src/Utils.cc, line 80 at r7 (raw file):

Previously, azeey (Addisu Z. Taddese) wrote…

I don't know why cppcheck issues the warning. The function is declared in Utils.hh. This happens on CI too, so it's not just on my system (https://github.com/osrf/sdformat/runs/2188294409#step:4:2013)

OK SGTM! (not worth blocking on)

src/Utils.cc, line 214 at r7 (raw file):

Previously, azeey (Addisu Z. Taddese) wrote…

Done in d773f70

OK Thanks!

test/test_config.h.in, line 30 at r7 (raw file):

Previously, azeey (Addisu Z. Taddese) wrote…

This one I was actually able to fix. The test directory needed to be passed to cppcheck. Fixed in 62e6078

OK Yay!

test/integration/interface_api.cc, line 334 at r7 (raw file):

Previously, azeey (Addisu Z. Taddese) wrote…

Done in a6a975f. Slightly different than yours though using std::string::compare.

OK Thanks!

test/integration/interface_api.cc, line 431 at r7 (raw file):

Previously, azeey (Addisu Z. Taddese) wrote…

Done in a6a975f

OK Yay! Thanks!

test/integration/interface_api.cc, line 573 at r7 (raw file):

Previously, azeey (Addisu Z. Taddese) wrote…

"exp" meant expected. I renamed all of them in a6a975f

OK Thanks!

test/integration/interface_api.cc, line 578 at r7 (raw file):

Previously, azeey (Addisu Z. Taddese) wrote…

"dp" -> "doublePendulum" in a6a975f

OK Ah, I see - thanks! (a comment to that effect would've also helped - unless I missed it 😅)

test/integration/interface_api.cc, line 709 at r7 (raw file):

Previously, azeey (Addisu Z. Taddese) wrote…

If you meant the file name suffix, I added that in a6a975f. The diff can be difficult to parse because I modified the test to check that the reposture callback is called recursively.

OK SGTM! I see the ext check!

test/integration/toml_parser.hh, line 46 at r11 (raw file):

}

std::string keyType(const std::string &_key)

BTW Not a huge deal, but this will lead to linker errors if this is included across multiple linked translation units.

Consider using inline judiciously, or just put a warning at the top.

EricCousineau-TRI

Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @azeey and @jennuine)

a discussion (no related file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

OK Dismissing myself here - we can spike test Drake integ after Edifice release!

"spike test" => "review your PR on Drake" 🤦

azeey

Dismissed @scpeters from a discussion.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @jennuine)

a discussion (no related file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

"spike test" => "review your PR on Drake" 🤦

Here's the Drake Prototype. Tests are passing for me locally: RobotLocomotion/drake@master...azeey:sdf_interface_api

a discussion (no related file):

Previously, scpeters (Steve Peters) wrote…

🎊

Done.

azeey

Dismissed @jennuine from 8 discussions.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @jennuine and @scpeters)

test/integration/toml_parser.hh, line 46 at r11 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

BTW Not a huge deal, but this will lead to linker errors if this is included across multiple linked translation units.

Consider using inline judiciously, or just put a warning at the top.

Thanks. Let me do that in a follow up PR.

azeey · 2021-03-25T06:38:23Z

Since the Drake prototype is passing, I'll go ahead and merge this.

EricCousineau-TRI · 2021-03-25T14:36:21Z

Woot! Awesome!!!

EricCousineau-TRI · 2021-03-25T18:11:01Z

include/sdf/Model.hh, line 305 at r7 (raw file):

Previously, azeey (Addisu Z. Taddese) wrote…

I left a comment in reviewable: https://reviewable.io/reviews/osrf/sdformat/475#-MUP_KAbfzuwemLFmXer-r7-305

Per VC, Addisu will create an issue.

…parsers (#562) #475 introduced a regression where included URDFs were being silently ignored when there are no custom parsers available. This returns the behavior back to URDFs being parsed natively by SDFormat if there are no custom parsers. Signed-off-by: Addisu Z. Taddese <[email protected]>

azeey · 2021-05-10T15:34:08Z

include/sdf/Model.hh, line 305 at r7 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Per VC, Addisu will create an issue.

#544

azeey added 29 commits December 11, 2020 18:14

Remove outdated error message

c4e2ba4

The error message assumes model uri's always start with "model://". This is no longer the case. Signed-off-by: Addisu Z. Taddese <[email protected]>

Add ParserConfig class to encapsulate file path settings

89dc847

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

Use semi-colon as path separator in windows

7badb17

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

Add header to CMakeLists

b4f1da3

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

Merge remote-tracking branch 'upstream/master' into parser_config

e3e05f3

Address reviewer feedback

f9e3ef2

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

Rename DefaultConfig to GlobalConfig

deda523

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

Fix style

b9bb22b

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

Remove outdated error message

66d2967

The error message assumes model uri's always start with "model://". This is no longer the case. Signed-off-by: Addisu Z. Taddese <[email protected]>

Add ParserConfig class to encapsulate file path settings

d06e21d

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

Use semi-colon as path separator in windows

ad2f9a3

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

Add header to CMakeLists

db1de1d

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

Address reviewer feedback

81d7998

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

Rename DefaultConfig to GlobalConfig

a00553d

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

Fix style

e8e7237

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

Allow files paths for include URIs

84f80f4

//include/uri used to only take directories that contain a model.config file and the sdformat file. However, allowing //include/uri to take file names can be supported without loss of functionality. Signed-off-by: Addisu Z. Taddese <[email protected]>

WIP

b84443b

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

Update test expectations after merge

61eec3f

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

WIP: TOML parser with array of tables

b8a8f6b

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

WIP: Map of Maps TOML data structure

2f498cc

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

Add ign-utils dependency

b9a4be2

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

WIP: Add NestedIncludeData test

a8bfcf4

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

Add InterfaceFrame

46a4d68

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

Merge remote-tracking branch 'upstream/master' into parser_config

ddafb64

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

Add InterfaceJoint

e358786

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

Merge remote-tracking branch 'upstream/master' into parser_config

74eacf0

Fix test expectations after merge

f088be8

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

Merge remote-tracking branch 'upstream/master' into parser_config

5084a6d

Merge remote-tracking branch 'upstream/master' into interface_api

4099c44

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

azeey self-assigned this Jan 22, 2021

scpeters requested changes Mar 25, 2021

View reviewed changes

azeey added 3 commits March 24, 2021 21:44

Address reviewer feedback

154b845

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

Move the toml parser into its own file

d832ed3

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

Test a condition where multiple custom parsers can successfully parse…

90cafa2

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

azeey commented Mar 25, 2021

View reviewed changes

scpeters requested changes Mar 25, 2021

View reviewed changes

azeey added 2 commits March 24, 2021 22:32

Pass the entire //include element to custom parsers

c0af826

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

Fix typos

93f5977

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

azeey commented Mar 25, 2021

View reviewed changes

scpeters requested changes Mar 25, 2021

View reviewed changes

Ignore C4251 for NestedInclude

1676e5d

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

jennuine approved these changes Mar 25, 2021

View reviewed changes

scpeters approved these changes Mar 25, 2021

View reviewed changes

EricCousineau-TRI reviewed Mar 25, 2021

View reviewed changes

azeey commented Mar 25, 2021

View reviewed changes

azeey merged commit d154b94 into gazebosim:master Mar 25, 2021

azeey mentioned this pull request May 4, 2021

Fix bug where included URDFs are not parsed when there are no custom parsers #562

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement SDFormat 1.8 Interface API #475

Implement SDFormat 1.8 Interface API #475

azeey commented Jan 22, 2021 •

edited by scpeters

Loading

scpeters left a comment

scpeters left a comment

azeey left a comment

scpeters left a comment

azeey left a comment

scpeters left a comment

scpeters commented Mar 25, 2021

azeey commented Mar 25, 2021

azeey commented Mar 25, 2021

jennuine left a comment

scpeters left a comment

EricCousineau-TRI left a comment

EricCousineau-TRI left a comment

azeey left a comment

azeey left a comment

azeey commented Mar 25, 2021

EricCousineau-TRI commented Mar 25, 2021

EricCousineau-TRI commented Mar 25, 2021

azeey commented May 10, 2021

Implement SDFormat 1.8 Interface API #475

Implement SDFormat 1.8 Interface API #475

Conversation

azeey commented Jan 22, 2021 • edited by scpeters Loading

scpeters left a comment

Choose a reason for hiding this comment

scpeters left a comment

Choose a reason for hiding this comment

azeey left a comment

Choose a reason for hiding this comment

scpeters left a comment

Choose a reason for hiding this comment

azeey left a comment

Choose a reason for hiding this comment

scpeters left a comment

Choose a reason for hiding this comment

scpeters commented Mar 25, 2021

azeey commented Mar 25, 2021

azeey commented Mar 25, 2021

jennuine left a comment

Choose a reason for hiding this comment

scpeters left a comment

Choose a reason for hiding this comment

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

azeey left a comment

Choose a reason for hiding this comment

azeey left a comment

Choose a reason for hiding this comment

azeey commented Mar 25, 2021

EricCousineau-TRI commented Mar 25, 2021

EricCousineau-TRI commented Mar 25, 2021

azeey commented May 10, 2021

azeey commented Jan 22, 2021 •

edited by scpeters

Loading