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

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

Draft
wants to merge 4 commits into
base: gz-sim8
Choose a base branch
from

Conversation

scpeters
Copy link
Member

@scpeters scpeters commented May 2, 2024

🎉 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:

  • Move definition of priority type, default priority, and priority element name to public System.hh header
  • Apply changes to PreUpdate as well
  • Retarget to Ionic and test with sensor systems
  • Profile performance of sensors using PostUpdate / Update / PreUpdate

Test it

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

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.

Specifying an integer value in a <gz:system_priority> tag
for a system will control the order in which System Update
callbacks are executed, with lower values executing first.

Signed-off-by: Steve Peters <[email protected]>
Signed-off-by: Steve Peters <[email protected]>
Signed-off-by: Steve Peters <[email protected]>
Copy link

codecov bot commented May 2, 2024

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 ⚠️
Additional details and impacted files
@@           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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@arjo129
Copy link
Contributor

arjo129 commented May 3, 2024

@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: #1815

@scpeters
Copy link
Member Author

scpeters commented May 3, 2024

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
Copy link
Member Author

scpeters commented May 3, 2024

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
Copy link
Contributor

xela-95 commented May 3, 2024

CC @traversaro

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🎵 harmonic Gazebo Harmonic
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

New phase for handling user updates of world state
3 participants