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

Update all velocity and acceleration components of non-link entities #1868

Merged
merged 7 commits into from
Mar 7, 2023

Conversation

ejalaa12
Copy link
Contributor

🎉 New feature

Closes #1866

Summary

This PR implements the situation explained the related issue

Test it

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • 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 and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

@ejalaa12 ejalaa12 requested a review from azeey as a code owner January 24, 2023 17:18
@github-actions github-actions bot added the 🌱 garden Ignition Garden label Jan 24, 2023
@azeey azeey changed the title update all velocity and acceleration components of non-link entities Update all velocity and acceleration components of non-link entities Jan 26, 2023
Copy link
Contributor

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

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?

@ejalaa12 ejalaa12 requested a review from mjcarroll as a code owner February 14, 2023 15:57
@ejalaa12
Copy link
Contributor Author

Sorry for the delay, I was off on holiday.
I just updated the PR with added unit test.

Those unit test make sure that the components are updated and seem coherent.

Thanks again for reviewing it.

@ejalaa12 ejalaa12 force-pushed the issue-1866 branch 2 times, most recently from 7169856 to 0881128 Compare February 14, 2023 16:05
@ejalaa12
Copy link
Contributor Author

Sorry for the two force-push, I was fixing the "signed-off" message to pass the DCO

@ejalaa12
Copy link
Contributor Author

@azeey any update on this PR?

@azeey azeey self-assigned this Mar 2, 2023
Copy link
Contributor

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

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

test/integration/physics_system.cc Outdated Show resolved Hide resolved
test/integration/physics_system.cc Outdated Show resolved Hide resolved
@ejalaa12
Copy link
Contributor Author

ejalaa12 commented Mar 4, 2023

@azeey Thanks for review. I just update my PR with the changes.

Signed-off-by: Addisu Z. Taddese <[email protected]>
Copy link
Contributor

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

I fixed some style issues in 214841e. LGTM! Thanks again for the contribution!

@azeey
Copy link
Contributor

azeey commented Mar 7, 2023

The Ubuntu CI failure is due to a network issue with Fuel. The homebrew failure (INTEGRATION_breadcrumbs) is a flaky test (#630), and windows warning should be fixed once #1917 is merged. I'll go ahead and merge this.

@azeey azeey merged commit d2475ff into gazebosim:gz-sim7 Mar 7, 2023
@azeey azeey mentioned this pull request Mar 8, 2023
8 tasks
@ejalaa12 ejalaa12 deleted the issue-1866 branch March 8, 2023 23:41
@ejalaa12
Copy link
Contributor Author

ejalaa12 commented Mar 8, 2023

Thanks for the review and the merge!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌱 garden Ignition Garden
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Physics System should Populate/Update all velocity/acceleration components for non-link entities
2 participants