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

DOC: clarify //pose/@relative_to #666

Merged
merged 20 commits into from
Sep 15, 2021

Conversation

FirefoxMetzger
Copy link
Contributor

Thanks to @azeey and @scpeters pointing me to relevant documentation my understanding of how the pose semantics are supposed to work has improved significantly. I thought I use this knowledge for some good and try to clarify the spec. Please double-check if I got it right.

Signed-off-by: FirefoxMetzger <sebastian@wallkoetter.net>
@github-actions github-actions bot added the 🏢 edifice Ignition Edifice label Aug 12, 2021
Signed-off-by: FirefoxMetzger <sebastian@wallkoetter.net>
Signed-off-by: FirefoxMetzger <sebastian@wallkoetter.net>
Signed-off-by: FirefoxMetzger <sebastian@wallkoetter.net>
@FirefoxMetzger
Copy link
Contributor Author

RTR from my side. This morning I found some more docs on scoping behavior and when implicit frames are created. Once this is clear the current pose semantics become much more obvious. I tried to reflect that in the docs of the spec.

Signed-off-by: FirefoxMetzger <sebastian@wallkoetter.net>
@FirefoxMetzger
Copy link
Contributor Author

Actually, I am not sure if the current description mentions all the entities that generate implicit frames. For example, if I have a //sensor and in particular a //sensor[@type=camera] then there are one (or two) poses that can be specified, which may or may not use @relative_to.

Does this mean that their names, too, must be unique in the current scope? Can an object be placed relative to a //sensor? What about other elements like //visual, or //collision?

@FirefoxMetzger
Copy link
Contributor Author

I think it's just a bad sample, but the CI pipeline really doesn't seem to stable for my PRs here ... it's been building for two days now.

Does this mean that their names, too, must be unique in the current scope? Can an object be placed relative to a //sensor? What about other elements like //visual, or //collision?

Regarding my earlier question, I think I figured it out for //collision since the test file model_frame_relative_to_joint.sdf declares a collision element with the same name as a link and - as far as I can tell - this is expected to be a valid SDF. So for //collision and //visual, I think it shouldn't create a new frame and consequentially you can't place objects relative to them.

Still not sure about a //sensor.

@azeey Any thoughts on my doc update otherwise?

Copy link
Member

@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.

I just started looking through this and will resume later when I have a chance. I've made a few comments for now.

Thanks for improving the documentation here!

sdf/1.7/frame.sdf Outdated Show resolved Hide resolved
sdf/1.7/frame.sdf Outdated Show resolved Hide resolved
sdf/1.7/frame.sdf Outdated Show resolved Hide resolved
sdf/1.7/frame.sdf Outdated Show resolved Hide resolved
sdf/1.7/model.sdf Outdated Show resolved Hide resolved
Framebearing - The element defines an implicit frame within it's
containing scope.

Signed-off-by: FirefoxMetzger <sebastian@wallkoetter.net>
Signed-off-by: FirefoxMetzger <sebastian@wallkoetter.net>
@scpeters
Copy link
Member

Actually, I am not sure if the current description mentions all the entities that generate implicit frames. For example, if I have a //sensor and in particular a //sensor[@type=camera] then there are one (or two) poses that can be specified, which may or may not use @relative_to.

The 1.7 proposal for pose frame semantics discusses explicit and implicit frames in section 1.2 and states that only link, joint, model and world have implicit frames

Does this mean that their names, too, must be unique in the current scope?

the name uniqueness rule in SDFormat 1.7 (see section 3.2 of the proposal) is rather broad, requiring unique names among all sibling elements, even if they don't have frames.

Can an object be placed relative to a //sensor? What about other elements like //visual, or //collision?

The name of a //sensor, //collision, or //visual cannot be used in a //pose/@relative_to or //frame/@attached_to attribute as they do not have implicit frames. These tags can use the //pose/@relative_to attribute to define their own pose, but it must use the name of an implicit frame.

@scpeters
Copy link
Member

Does this mean that their names, too, must be unique in the current scope? Can an object be placed relative to a //sensor? What about other elements like //visual, or //collision?

Regarding my earlier question, I think I figured it out for //collision since the test file model_frame_relative_to_joint.sdf declares a collision element with the same name as a link and - as far as I can tell - this is expected to be a valid SDF. So for //collision and //visual, I think it shouldn't create a new frame and consequentially you can't place objects relative to them.

this is correct

Still not sure about a //sensor.

it is similar to //collsiion and //visual; you can't place objects relative to a //sensor. You can, however, place a //frame where the sensor will be and then place the sensor relative to that //frame, so there is a different way to accomplish the desired posturing

Signed-off-by: FirefoxMetzger <sebastian@wallkoetter.net>
sdf/1.7/pose.sdf Outdated Show resolved Hide resolved
@scpeters
Copy link
Member

Ok, I've finished looking through your improvements to the 1.7 documentation and just have one last comment there

There are some changes in 1.8, particularly with regard to the :: delimiter token for referencing frames in nested scopes. This hasn't made its way into the documentation section yet, but it is described in the 1.8 composition proposal:

Signed-off-by: FirefoxMetzger <sebastian@wallkoetter.net>
@FirefoxMetzger
Copy link
Contributor Author

There are some changes in 1.8, particularly with regard to the :: delimiter token for referencing frames in nested scopes.

@scpeters Yep, I added docs for that in the v1.8 spec

New in v1.8: @relative_to may use frames of nested scopes within the same file. 
In this case, the frame is specified using `::` as delimiter to define the scope of the frame, e.g. `nested_model_A::nested_model_B::awesome_frame`.

Copy link
Member

@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.

just one last change and then I think it will be good to go

sdf/1.7/frame.sdf Show resolved Hide resolved
Copy link
Member

@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.

looks good to me. what do you think @azeey?

@chapulina chapulina requested a review from azeey August 30, 2021 18:54
sdf/1.7/pose.sdf Outdated Show resolved Hide resolved
sdf/1.7/pose.sdf Outdated Show resolved Hide resolved
sdf/1.7/frame.sdf Outdated Show resolved Hide resolved
sdf/1.8/pose.sdf Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Sep 14, 2021

Codecov Report

Merging #666 (b56b1ef) into sdf11 (82cb977) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##            sdf11     #666   +/-   ##
=======================================
  Coverage   88.08%   88.08%           
=======================================
  Files          73       73           
  Lines       11023    11023           
=======================================
  Hits         9710     9710           
  Misses       1313     1313           

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 82cb977...b56b1ef. Read the comment docs.

Signed-off-by: FirefoxMetzger <sebastian@wallkoetter.net>
Signed-off-by: FirefoxMetzger <sebastian@wallkoetter.net>
Signed-off-by: FirefoxMetzger <sebastian@wallkoetter.net>
Signed-off-by: FirefoxMetzger <sebastian@wallkoetter.net>
@FirefoxMetzger
Copy link
Contributor Author

@azeey Once the tests pass, I'm happy with this PR unless you have any further suggestions/comments.

Copy link
Collaborator

@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.

Just a couple more tweaks. Looks great!

sdf/1.7/frame.sdf Show resolved Hide resolved
sdf/1.8/frame.sdf Show resolved Hide resolved
sdf/1.8/pose.sdf Outdated Show resolved Hide resolved
Signed-off-by: FirefoxMetzger <sebastian@wallkoetter.net>
Signed-off-by: FirefoxMetzger <sebastian@wallkoetter.net>
Signed-off-by: FirefoxMetzger <sebastian@wallkoetter.net>
sdf/1.7/frame.sdf Show resolved Hide resolved
@azeey azeey merged commit d25bad7 into gazebosim:sdf11 Sep 15, 2021
of the element that contains the pose. For exceptions to this rule and
more details on the default behavior, see
http://sdformat.org/tutorials?tut=pose_frame_semantics and
http://sdformat.org/tutorials?tut=pose_frame_semantics
Copy link
Member

Choose a reason for hiding this comment

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

these links are now identical

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oops, they were supposed to be pointing to the 1.5 and 1.7 versions respectively.

Copy link
Contributor Author

@FirefoxMetzger FirefoxMetzger Sep 17, 2021

Choose a reason for hiding this comment

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

Yep, you pasted/put versioned linkes @azeey , but the CI chain is unhappy about the fragment character (#) in the link and breaks if it is there. I removed them but didn't realize that it makes the links the same.

We could patch it and remove the second link.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See: #702

@osrf-triage
Copy link

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-ignition-releases-2022-03-25-fortress-edifice-citadel/1343/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏢 edifice Ignition Edifice
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants