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

Rename CanonicalLink to RootLink #234

Merged
merged 6 commits into from
Mar 23, 2021
Merged

Conversation

azeey
Copy link
Contributor

@azeey azeey commented Mar 18, 2021

🦟 Bug fix

Fixes #233

Summary

See #233 for context.
This deprecates the end-user facing function CanonicalLink adds a new one called UnconstrainedLink.

Note: While the changes in this PR won't break downstream users of ign-physics, it will break any physics plugins the implement the FreeGroup feature.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
@azeey azeey requested a review from scpeters March 18, 2021 02:33
@azeey azeey requested a review from mxgrey as a code owner March 18, 2021 02:33
@github-actions github-actions bot added the 🏢 edifice Ignition Edifice label Mar 18, 2021
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
@chapulina chapulina added the beta Targeting beta release of upcoming collection label Mar 18, 2021
@adlarkin adlarkin self-requested a review March 18, 2021 20:18
Copy link
Contributor

@adlarkin adlarkin left a comment

Choose a reason for hiding this comment

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

Changes look OK to me. I'll let @scpeters do one last final pass since I'm still pretty new to ign-physics.

Should we add a note to the migration guide about this change so that other users who have a plugin that implements the FreeGroup feature know what updates to make to reflect this change?

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, just need a migration guide entry

@mxgrey
Copy link
Contributor

mxgrey commented Mar 23, 2021

I might make a suggestion about the naming:

UnconstrainedLink sounds like it refers to a link that does not have any constraints attached to it. I.e. it has no parent nor child joints.

If Canonical is not considered a suitable name anymore, then instead of UnconstrainedLink I might recommend calling this the RootLink. If it's called RootLink then that explicitly conveys that it doesn't have parent joints but may have child joints.

azeey added 3 commits March 23, 2021 11:04
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
@azeey
Copy link
Contributor Author

azeey commented Mar 23, 2021

I might make a suggestion about the naming:

UnconstrainedLink sounds like it refers to a link that does not have any constraints attached to it. I.e. it has no parent nor child joints.

If Canonical is not considered a suitable name anymore, then instead of UnconstrainedLink I might recommend calling this the RootLink. If it's called RootLink then that explicitly conveys that it doesn't have parent joints but may have child joints.

RootLink make more sense. Renamed in 14a6b21

@azeey
Copy link
Contributor Author

azeey commented Mar 23, 2021

Ubuntu CI failed due to infra issues.
@osrf-jenkins run tests please

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
@azeey azeey changed the title Rename CanonicalLink to UnconstrainedLink Rename CanonicalLink to RootLink Mar 23, 2021
@azeey azeey merged commit 4d14501 into gazebosim:main Mar 23, 2021
@azeey azeey deleted the rename_canonical branch March 23, 2021 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta Targeting beta release of upcoming collection 🏢 edifice Ignition Edifice
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants