Reviewable status: 0 of 5 files reviewed, 1 unresolved discussion (waiting on @azeey)

src/parser.cc, line 1584 at r1 (raw file):

            includeSDFFirstElem->SetIncludeElement(includeInfo);
          }
          bool toMerge = _sdf->GetName() == "model" &&

It's not clear to me that this imposes the encapsulation checks.

My preference is that we uphold the invariant that a model should always be valid as a standalone component, and we should actively fail on models that are not fully standalone.

More concretely, a models should always be includable via @merge={false,true}, with the only differences:

• @merge=false: Names could now collide; should check.
• @merge=true: Names will be nested (only model name could collide w/ existing elements).

Is this upheld by the current code?
If not, is there a (possibly elegant / minimal) solution to uphold encapsulation?

Reviewable status: 0 of 5 files reviewed, 1 unresolved discussion (waiting on @azeey)

src/parser.cc, line 1584 at r1 (raw file):

More specifically, an included model should not depend on frames, links, joints, etc. that are in the including context.

Reviewable status: 0 of 5 files reviewed, 1 unresolved discussion (waiting on @EricCousineau-TRI)

src/parser.cc, line 1584 at r1 (raw file):

you can construct a throwaway sdf::Model to call the validation functions on a model to be merge-included, but that may be a bit much. Otherwise, the only functions parser.hh that take an sdf::ElementPtr as an input are for checking name uniqueness and for not misusing ::

https://github.com/ignitionrobotics/sdformat/blob/6251baaddea3e3216c09fadb38cc6747b83bf1bf/include/sdf/parser.hh#L409-L428

Reviewable status: 0 of 5 files reviewed, 1 unresolved discussion (waiting on @azeey)

src/parser.cc, line 1584 at r1 (raw file):

Working: I'll see if using a throwaway sdf::Model would be feasible without becoming a maintenance burden.

Reviewable status: 0 of 11 files reviewed, 1 unresolved discussion (waiting on @azeey and @EricCousineau-TRI)

src/parser.cc, line 237 at r3 (raw file):

    ElementPtr proxyModelFrame = _parent->AddElement("frame");
    const std::string proxyModelFrameName = model->Name() + "__model__";

FYI I changed this from __<model name>__model__ to <model name>__model__ because when using the former and generating an SDFormat output file, we end up having a frame with an invalid name since names that start and end with __ are reserved by SDFormat.

Reviewed 2 of 5 files at r1, 5 of 8 files at r2, 1 of 3 files at r3, all commit messages.
Reviewable status: 8 of 11 files reviewed, 8 unresolved discussions (waiting on @azeey, @EricCousineau-TRI, and @jennuine)

src/parser.cc, line 210 at r3 (raw file):

      Error unsupportedError(
          ErrorCode::MERGE_INCLUDE_UNSUPPORTED,
          "Merge-include can not supported when parent element is " +

nit: rephrase to "Merge-include does not support parent elements of type " + _parent->GetName()

src/parser.cc, line 237 at r3 (raw file):

    ElementPtr proxyModelFrame = _parent->AddElement("frame");
    const std::string proxyModelFrameName = model->Name() + "__model__";

it's an edge case, but I think this frame name could be invalid if the model name starts with __:

<model name="__valid_model_name">
...
</model>

would give a frame name of __valid_model_name__model__

src/parser.cc, line 333 at r3 (raw file):

      if ((elem->GetName() == "link") || (elem->GetName() == "model") ||
          (elem->GetName() == "joint") || (elem->GetName() == "frame") ||
          (elem->GetName() == "plugin") ||

consider adding "gripper" as well, as I just noticed that it is a named element as well

https://github.com/ignitionrobotics/sdformat/blob/sdformat11_11.2.2/sdf/1.8/model.sdf#L38
https://github.com/ignitionrobotics/sdformat/blob/sdformat11_11.2.2/sdf/1.8/gripper.sdf#L5

src/parser.cc, line 353 at r3 (raw file):

    firstElem->SetParent(_parent);
    _parent->InsertElement(firstElem);
  }

nit: I feel like this function would be a bit more readable if these short else if and else logic blocks came first. Currently I have to scroll past all the merge logic just to see the different branches. Perhaps it could start with:

if (!_merge)
{
  firstElem->SetParent(_parent);
  _parent->InsertElement(firstElem);
  return;
}
else if (firstElem->GetName() == "model")
{
  Error unsupportedError(
      ErrorCode::MERGE_INCLUDE_UNSUPPORTED,
      "Merge-include is only supported for included models");
  _sourceLoc.SetSourceLocationOnError(unsupportedError);
  _errors.push_back(unsupportedError);
  return;
}

then the code from the original if (_merge && firstElem->GetName() == "model") branch can be placed here, with or without a containing else block, since the other branches return;

actually the other error branch can be appended as well as else if (_parent->GetName() != "model")

test/integration/includes.cc, line 496 at r3 (raw file):

{
  sdf::ParserConfig config;
  // Useing the "file://" URI scheme to allow multiple search paths

Useing -> Using

test/integration/model/merge_robot/model.sdf, line 275 at r3 (raw file):

    <joint name='right_wheel_joint' type='revolute'>
      <pose relative_to='__model__'>1 0 0 0 0 0</pose>

there is special logic for handling __model__ in the attributes, but I didn't see any test expectations confirming these values

to be thorough, you could add a //frame with attribute values of __model__ and a joint with //axis/xyz/@expressed_in == "__model__" to test that logic as well

test/sdf/model_invalid_frame_only.sdf, line 5 at r3 (raw file):

  <model name="model_invalid_frame_only">
    <frame name="F1"/>
    <frame name="F2"/> 

nit: trailing whitespace on this line

Reviewable status: 8 of 11 files reviewed, 12 unresolved discussions (waiting on @azeey and @EricCousineau-TRI)

test/integration/includes.cc, line 591 at r3 (raw file):

    std::stringstream buffer;
    sdf::testing::RedirectConsoleStream redir(
        sdf::Console::Instance()->GetMsgStream(), &buffer);

Would adding this fix the windows CI failure?:
https://github.com/ignitionrobotics/sdformat/blob/a797e18eeb012012f02f194178b16b5bf847fe39/test/integration/deprecated_specs.cc#L77-L84

test/integration/model/merge_robot/model.config, line 4 at r3 (raw file):

<model>
  <name>robot</name>
  <sdf version="1.7">model.sdf</sdf>

1.7 -> 1.9?

test/integration/model/merge_robot/model.sdf, line 2 at r3 (raw file):

<?xml version='1.0' ?>
<sdf version='1.8' xmlns:custom="http://example.org/schema">

1.8 -> 1.9?

test/sdf/model_invalid_frame_only.sdf, line 2 at r3 (raw file):

<?xml version="1.0" ?>
<sdf version='1.7'>

1.7 -> 1.9?

Reviewable status: 4 of 11 files reviewed, 9 unresolved discussions (waiting on @azeey, @EricCousineau-TRI, @jennuine, and @scpeters)

src/parser.cc, line 210 at r3 (raw file):

Done in 923cc72

src/parser.cc, line 237 at r3 (raw file):

That's a good point. I'm not sure how we can avoid that unless we also reserve all names that start with double underscore. Alternatively, we can modify the poses of each element and not use a frame.

src/parser.cc, line 333 at r3 (raw file):

Done. 923cc72

src/parser.cc, line 353 at r3 (raw file):

Done. I agree it's a bit more readable. 923cc72

test/integration/includes.cc, line 496 at r3 (raw file):

Done. 923cc72

test/integration/includes.cc, line 591 at r3 (raw file):

Yeah, I think that's the fix. Thanks! 923cc72

test/integration/model/merge_robot/model.config, line 4 at r3 (raw file):

Per VC, I think this should remain 1.7 since it doesn't use any syntax from 1.9

test/integration/model/merge_robot/model.sdf, line 2 at r3 (raw file):

Per VC, I think this should remain 1.7 since it doesn't use any syntax from 1.9

test/integration/model/merge_robot/model.sdf, line 275 at r3 (raw file):

Done. Added more tests in 923cc72. Instead creating a new joint, I modified the existing one. Let me know if that's okay.

test/sdf/model_invalid_frame_only.sdf, line 2 at r3 (raw file):

Per VC, I think this should remain 1.7 since it doesn't use any syntax from 1.9

test/sdf/model_invalid_frame_only.sdf, line 5 at r3 (raw file):

Done. 923cc72

Reviewed 4 of 7 files at r5, all commit messages.
Reviewable status: 8 of 11 files reviewed, 7 unresolved discussions (waiting on @azeey, @EricCousineau-TRI, @jennuine, and @scpeters)

test/integration/model/merge_robot/model.sdf, line 2 at r3 (raw file):

it looks like sensor_frame is using //pose/@degrees, so I think it should be 1.9

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

test/integration/includes.cc, line 671 at r5 (raw file):

    EXPECT_EQ(5, *errors[0].LineNumber());
    EXPECT_TRUE(buffer.str().find("Error parsing XML in file") !=
                std::string::npos);

suggest adding << buffer.str() between closing ) and ; to aid in debugging the windows test failure

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

test/integration/includes.cc, line 671 at r5 (raw file):

Done in e7dc8be.

test/integration/model/merge_robot/model.sdf, line 2 at r3 (raw file):

Ah, right. I've updated it to 1.9 in e7dc8be

Reviewable status: 7 of 21 files reviewed, 6 unresolved discussions (waiting on @azeey, @EricCousineau-TRI, and @scpeters)

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

  // deprecation warnings on memeber variables when simply instantiating this
  // class.
  // TODO(anyone) Remove the constructor and destructor once the depreceted

nit: depreceted -> deprecated

src/InterfaceElements_TEST.cc, line 2 at r8 (raw file):

/*
 * Copyright (C) 2018 Open Source Robotics Foundation

nit: 2018 -> 2021

src/parser.cc, line 237 at r3 (raw file):

or merged__<model name>__model__?

Reviewable status: 7 of 21 files reviewed, 4 unresolved discussions (waiting on @azeey, @EricCousineau-TRI, and @scpeters)

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

Done. de373d0

src/InterfaceElements_TEST.cc, line 2 at r8 (raw file):

Done. de373d0

src/parser.cc, line 237 at r3 (raw file):

Ok, changed to merged__<model name>__model__ in de373d0

Reviewed 2 of 11 files at r6.
Reviewable status: 9 of 21 files reviewed, 1 unresolved discussion (waiting on @azeey and @EricCousineau-TRI)

src/parser.cc, line 237 at r3 (raw file):

OK

test/integration/model/merge_robot/model.sdf, line 275 at r3 (raw file):

OK

Reviewed 5 of 11 files at r6, 1 of 2 files at r7, 3 of 3 files at r8, 3 of 3 files at r9, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @EricCousineau-TRI)