-
Notifications
You must be signed in to change notification settings - Fork 94
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
Allow "drilling down" into nested model frames #381
Conversation
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]>
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]>
Signed-off-by: Steve Peters <[email protected]>
Signed-off-by: Steve Peters <[email protected]>
Signed-off-by: Addisu Z. Taddese <[email protected]>
Signed-off-by: Addisu Z. Taddese <[email protected]>
Signed-off-by: Addisu Z. Taddese <[email protected]>
Signed-off-by: Addisu Z. Taddese <[email protected]>
Signed-off-by: Addisu Z. Taddese <[email protected]>
Signed-off-by: Addisu Z. Taddese <[email protected]>
…(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]>
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]>
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]>
Signed-off-by: Addisu Z. Taddese <[email protected]>
Signed-off-by: Addisu Z. Taddese <[email protected]>
Per f2f:
|
Signed-off-by: Addisu Z. Taddese <[email protected]>
The `__root__` vertex still exits, but has either a `__model__` scope or a `world` scope. Signed-off-by: Addisu Z. Taddese <[email protected]>
…ect to set the scope Signed-off-by: Addisu Z. Taddese <[email protected]>
Signed-off-by: Addisu Z. Taddese <[email protected]>
…raphPose Signed-off-by: Addisu Z. Taddese <[email protected]>
Signed-off-by: Addisu Z. Taddese <[email protected]>
Signed-off-by: Addisu Z. Taddese <[email protected]>
Signed-off-by: Addisu Z. Taddese <[email protected]>
Signed-off-by: Addisu Z. Taddese <[email protected]>
Signed-off-by: Addisu Z. Taddese <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #381 +/- ##
==========================================
+ Coverage 87.40% 87.71% +0.31%
==========================================
Files 60 61 +1
Lines 9351 9467 +116
==========================================
+ Hits 8173 8304 +131
+ Misses 1178 1163 -15
Continue to review full report at Codecov.
|
Signed-off-by: Addisu Z. Taddese <[email protected]>
There was a problem hiding this 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 r15.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @azeey)
src/FrameSemantics.cc, line 311 at r14 (raw file):
Previously, azeey (Addisu Z. Taddese) wrote…
The
__root__
vertex identifies the scope that contains a root level model. In the PoseRelativeTo graph, this vertex is connected to the model with an edge that holds the value of//model/pose
. However, in the FrameAttachedTo graph, the vertex is disconnected because nothing is attached to it. Since only links and static models are allowed to be disconnected in this graph, I choseSTATIC_MODEL
as its frame type. I could potentially add a different frame type, but that adds more complexity to thevalidateFrameAttachedToGraph
code.
ok, that sounds fine. do you mind adding that justification as a comment in the code?
src/FrameSemantics.cc, line 1348 at r14 (raw file):
Previously, azeey (Addisu Z. Taddese) wrote…
Done in 5c42e39. I also made the
outDegree != 0
anelse if
branch so it isn't taken if thescopeFrameType
is notWORLD
orSTATIC_MODEL
.
I see that you made this change in validateFrameAttachedToGraph
, but I don't see the change here in validatePoseRelativeToGraph
Signed-off-by: Addisu Z. Taddese <[email protected]>
Signed-off-by: Addisu Z. Taddese <[email protected]>
Signed-off-by: Addisu Z. Taddese <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 60 of 64 files reviewed, 1 unresolved discussion (waiting on @azeey, @EricCousineau-TRI, and @scpeters)
include/sdf/Model.hh, line 191 at r16 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Not sure if this came up on #355:
Methods like
FrameCount()
may now be "asymmetric" w.r.t.FrameByName()
- i.e. the number of frames reachable byFrameByName()
exceeds those indicated byFrameCount()
.I think that's fine, but we should clarify that in a comment here, saying that it's only the immediate (not nested frames), and make an explicit mention that
FrameByName
can reach more.(Same for link, joint, and nested models)
Thought about this when seeing the unittest mods about flattened models.
Done. Added clarifying comments in f544805.
src/FrameSemantics.cc, line 311 at r14 (raw file):
Previously, scpeters (Steve Peters) wrote…
ok, that sounds fine. do you mind adding that justification as a comment in the code?
Done in 661a502.
src/FrameSemantics.cc, line 1348 at r14 (raw file):
Previously, scpeters (Steve Peters) wrote…
I see that you made this change in
validateFrameAttachedToGraph
, but I don't see the change here invalidatePoseRelativeToGraph
Ah, I missed that one. Done in 661a502.
test/integration/nested_model.cc, line 327 at r16 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
nit
xpectations
is misspelled
Done in d06275b.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 4 of 4 files at r17, 1 of 1 files at r18.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @azeey and @EricCousineau-TRI)
include/sdf/Root.hh, line 113 at r17 (raw file):
/// of this Root object. /// \remark ModelNameExists() can find nested models that are not immediate /// children of this Root object.
Root::ModelNameExists
doesn't actually find nested models; this was enabled for Model::ModelNameExists
in #355, but only for the Model
class methods
include/sdf/World.hh, line 156 at r17 (raw file):
/// \param[in] _name Name of the model. /// To get a model nested in other models, prefix the model name /// with the sequence of nested model names, delimited by "::".
World::ModelByName
doesn't actually find nested models; this was enabled for Model::ModelByName
in #355, but only for the Model
class methods
include/sdf/Root.hh, line 113 at r17 (raw file): Previously, scpeters (Steve Peters) wrote…
hmm, I hadn't realized that. Do we want to keep it this way or update |
There was a problem hiding this 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, 3 unresolved discussions (waiting on @azeey and @EricCousineau-TRI)
include/sdf/Root.hh, line 113 at r17 (raw file):
Previously, azeey (Addisu Z. Taddese) wrote…
hmm, I hadn't realized that. Do we want to keep it this way or update
Root::ModelNameExists
andWorld::ModelNameExists
to also work for nested models?
changing the Root
and World
APIs wasn't necessary to support explicitly nested canonical links, so I didn't address it in #355. I think it would be reasonable to make all the *ByName
and *NameExists
APIs work like this, but I'd make an issue for it and not try to include it here
…tions Signed-off-by: Addisu Z. Taddese <[email protected]>
There was a problem hiding this 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, 3 unresolved discussions (waiting on @EricCousineau-TRI and @scpeters)
include/sdf/Root.hh, line 113 at r17 (raw file):
Previously, scpeters (Steve Peters) wrote…
changing the
Root
andWorld
APIs wasn't necessary to support explicitly nested canonical links, so I didn't address it in #355. I think it would be reasonable to make all the*ByName
and*NameExists
APIs work like this, but I'd make an issue for it and not try to include it here
Okay. Created a ticket: #412 and reverted the change in 8b0665a.
include/sdf/World.hh, line 156 at r17 (raw file):
Previously, scpeters (Steve Peters) wrote…
World::ModelByName
doesn't actually find nested models; this was enabled forModel::ModelByName
in #355, but only for theModel
class methods
Reverted in 8b0665a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r19.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @azeey and @EricCousineau-TRI)
include/sdf/Root.hh, line 113 at r17 (raw file):
/// \brief Get the number of models that are immediate (not nested) children /// of this Root object.
I think this part of the change was ok, since it clarified that the API refers to not-nested children. The \remark
was the part that I object to.
Signed-off-by: Addisu Z. Taddese <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 62 of 64 files reviewed, 2 unresolved discussions (waiting on @EricCousineau-TRI and @scpeters)
include/sdf/Root.hh, line 113 at r17 (raw file):
Previously, scpeters (Steve Peters) wrote…
/// \brief Get the number of models that are immediate (not nested) children /// of this Root object.
I think this part of the change was ok, since it clarified that the API refers to not-nested children. The
\remark
was the part that I object to.
Okay, kept that part of the change in 6390633.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r20.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @EricCousineau-TRI)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @azeey)
include/sdf/Model.hh, line 191 at r16 (raw file):
Previously, azeey (Addisu Z. Taddese) wrote…
Done. Added clarifying comments in f544805.
OK Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 5 of 8 files at r14, 2 of 4 files at r17, 1 of 1 files at r18, 2 of 2 files at r20.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @azeey)
test/integration/nested_model.cc, line 628 at r20 (raw file):
{ const std::string testFile = sdf::filesystem::append(PROJECT_SOURCE_PATH, "test", "integration",
BTW Did you have a chance to confirm that loading a flattened out model in SDFormat 1.8 fails?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @azeey)
test/integration/nested_model.cc, line 628 at r20 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
BTW Did you have a chance to confirm that loading a flattened out model in SDFormat 1.8 fails?
No I haven't. Just so I understand, it should pass if the file has version=1.7, but fail if 1.8?
…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]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 57 of 65 files reviewed, all discussions resolved (waiting on @azeey, @EricCousineau-TRI, and @scpeters)
test/integration/nested_model.cc, line 628 at r20 (raw file):
Previously, azeey (Addisu Z. Taddese) wrote…
No I haven't. Just so I understand, it should pass if the file has version=1.7, but fail if 1.8?
Yup! Feel free to do that as follow-up if you'd like this to merge first!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 57 of 65 files reviewed, all discussions resolved (waiting on @azeey, @EricCousineau-TRI, and @scpeters)
test/integration/nested_model.cc, line 628 at r20 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Yup! Feel free to do that as follow-up if you'd like this to merge first!
Okay, yeah, I'll go ahead and merge the PR and create an issue for this.
test/integration/nested_model.cc, line 628 at r20 (raw file): Previously, azeey (Addisu Z. Taddese) wrote…
|
This a draft PR that adds functionality to allow drilling down into nested models with the
::
syntax. This also makes included models work as nested modelsResolves #293, #284
This builds on top of #355
This change is