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

Parsing stages for nested canonical links #40

Merged
merged 2 commits into from
Sep 24, 2020
Merged

Conversation

scpeters
Copy link
Member

@scpeters scpeters commented Sep 2, 2020

Add parsing stage for identifying the canonical
link of a model with 3 steps:

  1. If //model/@canonical_link is specified, choose that link.
  2. If //model/@canonical_link is not specified, and the model
    has at least one link, choose the first link.
  3. Otherwise, chose the canonical link of the first nested
    model. This may be a recursive process if there are multiple
    levels of nested models without links.

This change is Reviewable

Add parsing stage for identifying the canonical
link of a model with 3 steps:

1. If //model/@canonical_link is specified, choose that link.
2. If //model/@canonical_link is not specified, and the model
   has at least one link, choose the first link.
3. Otherwise, chose the canonical link of the first nested
   model. This may be a recursive process if there are multiple
   levels of nested models without links.

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@scpeters
Copy link
Member Author

scpeters commented Sep 2, 2020

this should complete gazebosim/sdformat#342

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.

Reviewed 1 of 1 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @EricCousineau-TRI, @iche033, and @scpeters)


pose_frame_semantics/proposal.md, line 1651 at r1 (raw file):

    7.2 Identify the canonical link of the model:

    7.2.1 Return an error for 

Not sure the first 7.2.1 supposed to be there


pose_frame_semantics/proposal.md, line 1660 at r1 (raw file):

          contains links, choose the first link.

    7.2.3 Otherwise (i.e. if the `//model/@canonical_link` attribute

7.2.2 and 7.2.3 look the same to me. Should this be about nested models?

Copy link
Collaborator

@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: with some minor comments

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @azeey, @iche033, and @scpeters)


pose_frame_semantics/proposal.md, line 1660 at r1 (raw file):

Previously, azeey (Addisu Z. Taddese) wrote…

7.2.2 and 7.2.3 look the same to me. Should this be about nested models?

Perhaps they could be folded by saying something like
"... by choosing the first link. If there is no link directly in the current model, then proceed to use the next nested model's canonical link, recursing until a link is found, otherwise returning an error."

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
Copy link
Member Author

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

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


pose_frame_semantics/proposal.md, line 1651 at r1 (raw file):

Previously, azeey (Addisu Z. Taddese) wrote…

Not sure the first 7.2.1 supposed to be there

it was incomplete; I meant for that step to mean to quickly throw an error if the model has no links or nested models; updated in 2bc6370


pose_frame_semantics/proposal.md, line 1660 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Perhaps they could be folded by saying something like
"... by choosing the first link. If there is no link directly in the current model, then proceed to use the next nested model's canonical link, recursing until a link is found, otherwise returning an error."

I've updated it in 2bc6370 to fix the numbering and explain how to look into nested models if the current model has no links

@scpeters
Copy link
Member Author

@azeey can you take a look at my recent fixes to this PR?

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.

:lgtm:

Reviewed 1 of 1 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @iche033)

@scpeters scpeters merged commit 2011655 into master Sep 24, 2020
@scpeters scpeters deleted the nested_canonical_links branch September 24, 2020 18:04
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.

3 participants