Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move construction of PoseRelativeToGraph and FrameAttachedToGraph to sdf::Root #1

Merged
merged 8 commits into from
Nov 18, 2020

Conversation

azeey
Copy link
Owner

@azeey azeey commented Oct 30, 2020

This builds on top of gazebosim#381 and simplifies the creation of PoseRelativeTo and FrameAttachedTo graphs by moving them to one place instead of being in sdf::World and sdf::Model. It also simplifies the logic of determining if a model is a standalone model that needs its own graph or a nested model that adds to an existing graph.

This PR also changes the pointer held by ScopedGraph from a std::weak_ptr to a std::shared_ptr. This means that a DOM object, such as sdf::Model, can have its poses resolved even if the containing sdf::Root or sdf::World has gone out of scope.

A couple more notes:

  • When copying any DOM object with the exception of sdf::Root, there is no need to call SetPoseRelativeToGraph or SetFrameAttachedToGraphon child elements because the copy constructors of those child elements copy the ScopedGraph from the corresponding child element from the copied-from object. Since a new graph is never created, the child elements should be pointing to the graph of the copied-from object when their copy constructor is finished.

  • When copying from sdf::Root, we have to decide whether we want a deep copy of the graph or not. If we do a shallow copy, the constructor that simply copies the dataPtr is sufficient. Otherwise, we have to allocate new memory, make a copy of the graph and call SetPoseRelativeToGraph and SetFrameAttachedToGraph on all child elements of sdf::Root.


This change is Reviewable

@azeey
Copy link
Owner Author

azeey commented Oct 30, 2020

/cc @scpeters, @EricCousineau-TRI

Copy link

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm: at the high-level, with some follow-up questions

Reviewed 8 of 8 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @azeey)


src/Root.cc, line 120 at r1 (raw file):

  : dataPtr(new RootPrivate(*_root.dataPtr))
{
  // TODO(addisu) Do we need to make deep copies of the graphs?

Er, when does this copy constructor get called in the first place?
And does it expect deep or shallow copies in those contexts?

My gut says, yeah, this seems like it should be a deep copy, if the original object can be mutated, and thus violate any invariants / synchronization.

However, that seems like more effort than it's worth.

So to really ask the question:
Who really needs the copy constructor?
Can we just get rid of the copy constructor / copy assignment, and just rely instead on move assignments and unique_ptr / shared_ptr?


src/World.cc, line 624 at r1 (raw file):

  for (auto &light : this->dataPtr->lights)
  {
    light.SetXmlParentName("world");

nit This has a funny smell to it.

Can you justify this choice?

@codecov-io
Copy link

Codecov Report

Merging #1 into drill_down will increase coverage by 0.13%.
The diff coverage is 94.66%.

Impacted file tree graph

@@              Coverage Diff               @@
##           drill_down       #1      +/-   ##
==============================================
+ Coverage       87.59%   87.72%   +0.13%     
==============================================
  Files              61       61              
  Lines            9502     9467      -35     
==============================================
- Hits             8323     8305      -18     
+ Misses           1179     1162      -17     
Impacted Files Coverage Δ
src/Model.cc 89.56% <ø> (+3.20%) ⬆️
src/Root.cc 94.30% <91.11%> (-1.35%) ⬇️
src/ScopedGraph.hh 98.76% <100.00%> (-0.06%) ⬇️
src/World.cc 96.26% <100.00%> (+1.58%) ⬆️

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 f4def9c...75f4b12. Read the comment docs.

Copy link
Owner Author

@azeey azeey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @EricCousineau-TRI)


src/Root.cc, line 120 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Er, when does this copy constructor get called in the first place?
And does it expect deep or shallow copies in those contexts?

My gut says, yeah, this seems like it should be a deep copy, if the original object can be mutated, and thus violate any invariants / synchronization.

However, that seems like more effort than it's worth.

So to really ask the question:
Who really needs the copy constructor?
Can we just get rid of the copy constructor / copy assignment, and just rely instead on move assignments and unique_ptr / shared_ptr?

Since there is no public API to mutate the graphs, a shallow copy would be okay. That's what I'm doing in this PR for sdf::World and sdf::Model. But if we want to allow mutation of the graph down the road, a deep copy would be needed. In that case it should happen in sdf::Root only since this is now equivalent to the __root__ vertex in the graph. Making a copy of a sub-graph , eg. in sdf::Model, might be too complicated.

I haven't seen the copy constructor for sdf::Root used anywhere yet so I would be okay with explicitly deleting the copy constructor. However, I think it would be the only DOM object without one, so it will be a departure from our pattern.


src/World.cc, line 624 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit This has a funny smell to it.

Can you justify this choice?

This is nothing new. It was taken from https://github.com/osrf/sdformat/blob/144f82cf8b994abd2a566a7bbe6ae873a9d92403/src/World.cc#L166. sdf::Light can exist both as //world/light and //link/light (https://github.com/osrf/sdformat/blob/144f82cf8b994abd2a566a7bbe6ae873a9d92403/src/Link.cc#L390), so it needs to know what it should resolve to in the absence of a resolveTo argument.

Copy link

@scpeters scpeters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


src/Root.cc, line 94 at r1 (raw file):

/////////////////////////////////////////////////
template <typename T>
ScopedGraph<PoseRelativeToGraph> addPoseRealtiveToGraph(

change Realtive to Relative in function name


src/Root_TEST.cc, line 289 at r1 (raw file):

    root2 = root1;
    testFrame1(root1);
    testFrame1(root2);

nit: you could test both before the copy:

testFrame1(root1);
testFrame2(root2);

// root1 is copied into root2 via the assignment operator
root2 = root1;
testFrame1(root2);

src/Root_TEST.cc, line 312 at r1 (raw file):

    // root1 is moved into root2 via the move assignment operator.
    root2 = std::move(root1);
    testFrame1(root2);

nit: you could test both before the move:

testFrame1(root1);
testFrame2(root2);

// root1 is moved into root2 via the move assignment operator.
root2 = std::move(root1);
testFrame1(root2);

Copy link
Owner Author

@azeey azeey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


src/Root.cc, line 94 at r1 (raw file):

Previously, scpeters (Steve Peters) wrote…

change Realtive to Relative in function name

Done 4997432.


src/Root_TEST.cc, line 289 at r1 (raw file):

Previously, scpeters (Steve Peters) wrote…

nit: you could test both before the copy:

testFrame1(root1);
testFrame2(root2);

// root1 is copied into root2 via the assignment operator
root2 = root1;
testFrame1(root2);

Done in 033cada


src/Root_TEST.cc, line 312 at r1 (raw file):

Previously, scpeters (Steve Peters) wrote…

nit: you could test both before the move:

testFrame1(root1);
testFrame2(root2);

// root1 is moved into root2 via the move assignment operator.
root2 = std::move(root1);
testFrame1(root2);

Done in 033cada.

Copy link

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 5 of 5 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @azeey and @scpeters)


src/Root.cc, line 120 at r1 (raw file):

Previously, azeey (Addisu Z. Taddese) wrote…

Since there is no public API to mutate the graphs, a shallow copy would be okay. That's what I'm doing in this PR for sdf::World and sdf::Model. But if we want to allow mutation of the graph down the road, a deep copy would be needed. In that case it should happen in sdf::Root only since this is now equivalent to the __root__ vertex in the graph. Making a copy of a sub-graph , eg. in sdf::Model, might be too complicated.

I haven't seen the copy constructor for sdf::Root used anywhere yet so I would be okay with explicitly deleting the copy constructor. However, I think it would be the only DOM object without one, so it will be a departure from our pattern.

I think it's a pretty unique DOM object, so I'm A-OK with departing from that for now.

Can you delete the copy ctor?


src/World.cc, line 624 at r1 (raw file):

Previously, azeey (Addisu Z. Taddese) wrote…

This is nothing new. It was taken from https://github.com/osrf/sdformat/blob/144f82cf8b994abd2a566a7bbe6ae873a9d92403/src/World.cc#L166. sdf::Light can exist both as //world/light and //link/light (https://github.com/osrf/sdformat/blob/144f82cf8b994abd2a566a7bbe6ae873a9d92403/src/Link.cc#L390), so it needs to know what it should resolve to in the absence of a resolveTo argument.

Hm... I guess that validates by means of precedent. But it still feels dirty.

Can there be some "lightly" actionable takeaway, like a TODO or an issue?

Copy link

@scpeters scpeters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


src/World.cc, line 624 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Hm... I guess that validates by means of precedent. But it still feels dirty.

Can there be some "lightly" actionable takeaway, like a TODO or an issue?

it's the same thing we do for all DOM objects that provide a SemanticPose() API but aren't frames in the graph (see Link::SetPoseRelativeToGraph which calls SetXmlParentName on Collision, Light, Sensor, and Visual)

it doesn't "smell" to me, so you may have to suggest some wording for the TODO / issue because I'm not sure what I would write

Copy link
Owner Author

@azeey azeey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


src/Root.cc, line 120 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

I think it's a pretty unique DOM object, so I'm A-OK with departing from that for now.

Can you delete the copy ctor?

Done in d5e405a.

Copy link

@scpeters scpeters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 3 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @azeey and @EricCousineau-TRI)

Copy link

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 3 files at r3, 3 of 3 files at r4.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @azeey)


src/World.cc, line 624 at r1 (raw file):

Previously, scpeters (Steve Peters) wrote…

it's the same thing we do for all DOM objects that provide a SemanticPose() API but aren't frames in the graph (see Link::SetPoseRelativeToGraph which calls SetXmlParentName on Collision, Light, Sensor, and Visual)

it doesn't "smell" to me, so you may have to suggest some wording for the TODO / issue because I'm not sure what I would write

OK I only have a high-level comment atm, which is "see what's necessary to avoid awkward plumbing.", but I'm fine with it not being encoded as a TODO just yet.

Thanks!

Copy link

@scpeters scpeters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 3 of 3 files at r4.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @azeey)

@azeey azeey merged commit 0a8a7e0 into drill_down Nov 18, 2020
@azeey azeey deleted the graph_in_root branch November 18, 2020 21:25
azeey added a commit that referenced this pull request Dec 1, 2020
* Expect nested_explicit_canonical_link.sdf is valid

Split nested_invalid_explicit_canonical_link.sdf
into nested_explicit_canonical_link.sdf and
nested_without_links_invalid.sdf and update
UNIT_ign_TEST and an integration test.

Signed-off-by: Steve Peters <[email protected]>

* Support :: syntax in *NameExists and *ByName APIs

This extends the `Model::*NameExists` and `Model::*ByName` APIs
(like LinkNameExists and LinkByName) that allow passing
nested names that can begin with a sequence of nested model
names separated by :: and may end with the name of an object
of the specified type, such as "outer_model::inner_model::inner_joint".

For now, if a nested model is not found that matches the nested name
preceding the final ::, then it checks for objects in the current model
that match the entire name. This extra check should be disabled
when "::" is reserved and not allowed in frame names.

Signed-off-by: Steve Peters <[email protected]>

* Changelog and Migration guide

Signed-off-by: Steve Peters <[email protected]>

* Ensure CanonicalLink pointer is valid

Signed-off-by: Steve Peters <[email protected]>

* Add ScopedGraph header

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

* Frame attached to tests passing

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

* FrameSemantics_TEST passing

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

* Pass ChildScope with new scope name

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

* Add command line tool to generate graph

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

* Avoid using the RawPose of the DOM object when the object is a frame (implicit or explicit)

Instead use the edges in the pose graph. This allows us to update only
the pose graph when handling placement frames.

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

* Handle placement frames with new PoseRelativeTo construction scheme

The PoseRelativeTo used to be constructed in each nested model's Load
funcition. Now, it's only constructed at the outer most model. Because
of this decoupling and Since the model is `const` when the graph is
constructed, placement frames are handled differently. Instead of
updating the raw pose of the model, the edge connecting the model frame
to it's relative_to frame is modified to take into account the placement
frame.

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

* Add __root__ scope

The __root__ vertex is the root node of both world and model
PoseRelativeTo graphs. It corresponds to the `<sdf>` tag in SDFormat
files. Having this node makes it possible to keep the a model's pose and
(possibly accounting for placement_frame) information in the edge from
the _root__ vertex to the model vertex.

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

* Revert changes in loadUniqueRepeated

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

* Make ScopedGraph use pointer semantics

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

* Cleanup

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

* Remove `__root__` scope name

The `__root__` vertex still exits, but has either a `__model__` scope or
a `world` scope.

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

* SemanticPose::Resolve and JointAxis::ResolveXyz to a SemanticPose object to set the scope

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

* Add missing file

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

* Remove aliasing edges, there is no need for them. Also remove updateGraphPose

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

* Refactor

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

* Cleanup

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

* Add Resolve function that takes DOM objects

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

* Add test for placement attribute with nested link

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

* Codecheck

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

* Fix macOS build error

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

* Add nested model pose relative_to test

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

* Add nested model frame attached_to test

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

* Add test for joints that reference entities in nested models

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

* Add API documentation for ScopedGraph, refactor

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

* Add test for placement_frame element that references nested frames

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

* Add test for empty models that contain a nested static model

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

* Resolve SemanticPose objects relative to other SemanticPose objects

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

* Add missing file

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

* Revert the SemanticPose::Resolve API that takes SemanticPose objects

This proved to be problematic since the graph contained in the input
SemanticPose can be at a different scope than the graph in the current
object. Thus it becomes necessary to find the least common ancestor
vertex between two vertices in the graph. This is feasible, but it would
be best to do it in a separate PR.

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

* Remove addNestedModel

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

* Add more documentation, cleanup

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

* Validate __root__ vertices

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

* Add more error cases in validate* functions

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

* Remove obsolete test

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

* Prevent users from referencing `__root__` in SDFormat XML.

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

* More expectations in LoadJointNestedParentChild

Test `Joint::Resolve*Link` methods in LoadJointNestedParentChild
test case.

Signed-off-by: Steve Peters <[email protected]>

* Address Reviewer feedback

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

* Add and update doxygen for SetFrameAttachedToGraph and SetPoseRelativeToGraph

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

* Add doxygen to new SemanticPose constructor

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

* Remove unnecessary code

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

* Refactor code that handles PlacementFrame into a function

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

* Address reviewer feedback

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

* Add failing test showing bug in checkFrameAttachedToNames

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

* Fix how frame attached_to names are checked for existence

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

* Add unit test for the output stream of sdf::Errors

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

* Return void from Model::SetPoseRelativeToGraph

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

* Add test for world level nested references

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

* Add test for ign graph command

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

* Be more selective about checking for usage of __root__

Instead of checking if any attribute has a value of "__root__", only the
following attributes are checked:
  * //frame/[@attached_to]
  * //pose/[@relative_to]
  * //model/[@placement_frame]
  * //model/[@canonical_link]
  * //sensor/imu/orientation_reference_frame/custom_rpy/[@parent_frame]

In addition, the elements //joint/parent, //joint/child and
//include/placement_frame are checked for validity

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

* Address reviewer feedback

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

* Scoped -> scoped

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

* Address reviewer feedback

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

* Change scopeName to scopeContextName

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

* Add warning to the help message of ign sdf -g command

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

* Remove extraneous error comments

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

* Address reviewer feedback

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

* Resolve additional merge conflicts

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

* Cleanup

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

* Address reviewer feedback

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

* State that *Count functions only count immediate child elements

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

* Address reviewer feedback

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

* Typo

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

* Revert documentation updates to Root::*ByName and World::*ByName functions

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

* Keep some of the changes from the previous commit

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

* Move construction of PoseRelativeToGraph and FrameAttachedToGraph to sdf::Root (#1)

* Move graph creation to sdf::Root from sdf::World and sdf::Model.

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

* Convert ScopedGraph to hold a strong reference to the underlying graph

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

* Add copy/move constructors to sdf::Root

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

* Additional test of sdf::Root objects before copy and move

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

* Fix typo

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

* Explicitly delete copy constructor and assignment.

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

Co-authored-by: Steve Peters <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants