-
Notifications
You must be signed in to change notification settings - Fork 277
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
Update all velocity and acceleration components of non-link entities #1868
Conversation
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.
Thanks for the contribution! I think these components weren't populated because we didn't have a need for them, but since users won't pay for the performance unless they create the corresponding components, I don't think there's any harm in adding them.
Would you be able to add tests that exercise these change and make sure physics is populating them correctly?
Sorry for the delay, I was off on holiday. Those unit test make sure that the components are updated and seem coherent. Thanks again for reviewing it. |
7169856
to
0881128
Compare
Sorry for the two force-push, I was fixing the "signed-off" message to pass the DCO |
@azeey any update on this PR? |
fixes gazebosim#1866 Signed-off-by: Alaa El Jawad <[email protected]>
Signed-off-by: Alaa El Jawad <[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.
Sorry for the delay in reviewing this. This great work! I only have one suggestion to improve the test.
This reminded me that we need to update the documentation of the LinearVelocity
and AngularVelocity
components since they are currently not very precise (see #87).
@azeey Thanks for review. I just update my PR with the changes. |
Signed-off-by: Alaa El Jawad <[email protected]>
Signed-off-by: Alaa El Jawad <[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.
I fixed some style issues in 214841e. LGTM! Thanks again for the contribution!
Signed-off-by: Addisu Z. Taddese <[email protected]>
Thanks for the review and the merge! |
🎉 New feature
Closes #1866
Summary
This PR implements the situation explained the related issue
Test it
Checklist
codecheck
passed (See contributing)Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining
Signed-off-by
messages.