gazebosim / gz-sim

Open source robotics simulator. The latest version of Gazebo.
https://gazebosim.org
Apache License 2.0
685 stars 262 forks source link

Prototype: specify order of execution for System::Update callbacks #2394

Closed scpeters closed 2 months ago

scpeters commented 5 months ago

🎉 New feature

Part of #2268

Summary

This is a prototype for specifying an integer priority value for a System using a <gz:system_priority/> tag in its XML content. The SystemManager parses this tag and stores vectors of system callbacks in a std::map indexed by this priority value. The prototype only implements controllable execution order for the System::Update callbacks, but should be extended to support PreUpdate and PostUpdate as well (I'm not sure it's relevant for other callbacks like Configure, ConfigureParameters, or Reset but it could be implemented for those as well).

TODO:

Test it

Follow the instructions for the priority_printer example to run an example world with a plugin that illustrates this feature.

Checklist

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.

codecov[bot] commented 5 months ago

Codecov Report

Attention: Patch coverage is 90.00000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 65.91%. Comparing base (cffd297) to head (0fc9e66). Report is 2 commits behind head on gz-sim8.

Files Patch % Lines
src/SystemManager.cc 85.71% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## gz-sim8 #2394 +/- ## ======================================== Coverage 65.91% 65.91% ======================================== Files 327 327 Lines 31314 31320 +6 ======================================== + Hits 20641 20646 +5 - Misses 10673 10674 +1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

arjo129 commented 5 months ago

@scpeters I think this PR might have some minor conflicts with #2232. It might be worthwhile discussing the simulation_managers and simulation_runners internals.

On another note, is it really needed to extend this to postUpdate, given that postUpdate systems should not modify the world?

Also related: https://github.com/gazebosim/gz-sim/issues/1815

scpeters commented 5 months ago

I think this PR might have some minor conflicts with #2232. It might be worthwhile discussing the simulation_managers and simulation_runners internals.

yes, these will conflict; we should figure out an integrated design for both controlling system execution order and removing systems

On another note, is it really needed to extend this to postUpdate, given that postUpdate systems should not modify the world?

PostUpdate calls can have effects outside of the ECM through publishing messages, so it may be useful to group them in a specific order. It is more complicated though, so we can give it a second thought before implementing it.

Also related: #1815

Yes, I think the changes in this PR could address the concerns of #1815 without adding a new phase to the World step

scpeters commented 5 months ago

I think this PR might have some minor conflicts with #2232. It might be worthwhile discussing the simulation_managers and simulation_runners internals.

yes, these will conflict; we should figure out an integrated design for both controlling system execution order and removing systems

now that I've thought about it some, I think the conflicts should be resolvable. The std::map added in this pull request can store a vector of SystemHolder instead of Iface raw pointers.

xela-95 commented 5 months ago

CC @traversaro

scpeters commented 2 months ago

this is targeting harmonic; I'm going to open a separate PR for ionic

scpeters commented 2 months ago

we can consider back porting to harmonic eventually, but I've closed this for now