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

Add "ChangedWorldPose" to ForwardStep::Output's expected data #238

Merged
merged 1 commit into from
Mar 24, 2021

Conversation

adlarkin
Copy link
Contributor

@adlarkin adlarkin commented Mar 23, 2021

Signed-off-by: Ashton Larkin ashton@openrobotics.org

🎉 New feature

Related to #223

Possible solution for #219

Summary

#223 added functionality to DART and TPE to write changed link data from an iteration to ForwardStep::Output so that downstream libraries like ign-gazebo can use this data for more efficient post-physics processing/updates (gazebosim/gz-sim#678). In order for downstream libraries to make this functionality optional, the changed link data that is written to by DART and TPE must be of type ExpectedData instead of RequiredData (that way, downstream libraries can see if this data was actually written to or not). This PR makes this change.

Test it

Using this PR with gazebosim/gz-sim#678, users can run the ign-gazebo physics system integration test. This will use the output link data that ign-physics writes to in order to update the simulation state in ign-gazebo. All checks in the integration test should pass.

Users can then check out DART and TPE before these engines were modified to write output data (commit 109de85):

cd ign-physics
git checkout 109de85 -- dartsim/ tpe/

Re-run the physics integration test. This will use data internal to ign-gazebo (a local link pose cache in combination with the EntityComponentManager) to update the simulation state. All checks in the integration test should pass.

If you'd like to verify that the steps I described above use both data written to from ign-physics and data internal to ign-gazebo, you can add some print statements in each if/else block here to see what is being called.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • 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

@github-actions github-actions bot added the 🏢 edifice Ignition Edifice label Mar 23, 2021
@adlarkin adlarkin force-pushed the adlarkin/use_expected_output branch from 1890cb0 to 9ad0446 Compare March 23, 2021 19:40
Copy link
Contributor Author

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

@azeey, just a few questions for you about the implementation.

@adlarkin adlarkin marked this pull request as ready for review March 23, 2021 19:47
@adlarkin adlarkin requested a review from mxgrey as a code owner March 23, 2021 19:47
@codecov
Copy link

codecov bot commented Mar 23, 2021

Codecov Report

Merging #238 (78b8ec0) into main (246909b) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #238   +/-   ##
=======================================
  Coverage   83.15%   83.15%           
=======================================
  Files         107      107           
  Lines        4221     4221           
=======================================
  Hits         3510     3510           
  Misses        711      711           
Impacted Files Coverage Δ
include/ignition/physics/ForwardStep.hh 100.00% <ø> (ø)
dartsim/src/SimulationFeatures.cc 91.22% <100.00%> (ø)
tpe/plugin/src/SimulationFeatures.cc 78.33% <100.00%> (ø)

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 246909b...78b8ec0. Read the comment docs.

@adlarkin adlarkin requested a review from azeey March 23, 2021 21:23
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.

LGTM!

Signed-off-by: Ashton Larkin <ashton@openrobotics.org>
@adlarkin adlarkin force-pushed the adlarkin/use_expected_output branch from 89fcada to 78b8ec0 Compare March 24, 2021 03:00
@adlarkin adlarkin merged commit 78b8ec0 into main Mar 24, 2021
@adlarkin adlarkin deleted the adlarkin/use_expected_output branch March 24, 2021 03:41
@nkoenig nkoenig mentioned this pull request Apr 19, 2021
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.

2 participants