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.

Sign up for GitHub

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

Jump to bottom

Refactor System functionality into SystemManager #1340

Merged
merged 6 commits into from
Feb 25, 2022
Merged

Refactor System functionality into SystemManager #1340

merged 6 commits into from
Feb 25, 2022

Conversation

mjcarroll
Copy link
Contributor

@mjcarroll mjcarroll commented Feb 15, 2022 •
edited
Loading

SimulationRunner is getting long/crowded, so I'm refactoring out a bit of the System-oriented functionality into it's own Manager class as a peer to things like EventsManager and LevelManager. This should make it a bit easier to test, as well as reduce the clutter in SimulationRunner

Note that this moves the functionality into internal headers, so it should not have an impact on our public API/ABI.

@mjcarroll mjcarroll requested review from azeey and nkoenig February 15, 2022 19:29
@mjcarroll mjcarroll requested a review from chapulina as a code owner February 15, 2022 19:29
@mjcarroll mjcarroll self-assigned this Feb 15, 2022
@github-actions github-actions bot added the 🌱 garden Ignition Garden label Feb 15, 2022
@nkoenig
Copy link
Contributor

nkoenig commented Feb 16, 2022

Can this be targeted to Fortress? I'm being a bit selfish, but I have some upcoming PR's that will conflict with these changes, and I'd rather deal with it in Fortress than in a forward port.

@mjcarroll
Copy link
Contributor Author

Can this be targeted to Fortress? I'm being a bit selfish, but I have some upcoming PR's that will conflict with these changes, and I'd rather deal with it in Fortress than in a forward port.

I'll give it a shot, I don't see any reason why it can't be done.

@codecov
Copy link

codecov bot commented Feb 16, 2022 •
edited
Loading

Codecov Report

Merging #1340 (4d05ad5) into ign-gazebo6 (3b94bd2) will increase coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@               Coverage Diff               @@
##           ign-gazebo6    #1340      +/-   ##
===============================================
+ Coverage        62.93%   62.98%   +0.04%     
===============================================
  Files              299      301       +2     
  Lines            24167    24199      +32     
===============================================
+ Hits             15210    15242      +32     
  Misses            8957     8957
Impacted Files Coverage Δ
src/SimulationRunner.hh 100.00% <ø> (ø)
src/SimulationRunner.cc 92.07% <100.00%> (-0.31%) ⬇️
src/SystemInternal.hh 100.00% <100.00%> (ø)
src/SystemManager.cc 100.00% <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 3b94bd2...4d05ad5. Read the comment docs.

@mjcarroll mjcarroll force-pushed the reduce_simulationrunner branch from af17afe to bfb1491 Compare February 16, 2022 18:17
@mjcarroll mjcarroll changed the base branch from main to ign-gazebo6 February 16, 2022 18:17
@mjcarroll
Copy link
Contributor Author

Can this be targeted to Fortress? I'm being a bit selfish, but I have some upcoming PR's that will conflict with these changes, and I'd rather deal with it in Fortress than in a forward port.

Retargeted 🤞

@mjcarroll mjcarroll force-pushed the reduce_simulationrunner branch from bfb1491 to bb0f013 Compare February 17, 2022 13:34
azeey
azeey approved these changes Feb 17, 2022
View reviewed changes
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.

Nice refactor! I mostly have nits.

src/WorldControl.hh Show resolved Hide resolved
src/WorldControl.hh Show resolved Hide resolved
src/WorldControl.hh Show resolved Hide resolved
src/SimulationRunner.cc Outdated Show resolved Hide resolved
src/SimulationRunner.cc Outdated Show resolved Hide resolved
src/SystemManager.hh Outdated Show resolved Hide resolved
src/SystemManager.cc Outdated Show resolved Hide resolved
src/SystemManager_TEST.cc Outdated Show resolved Hide resolved
src/SystemManager_TEST.cc Outdated Show resolved Hide resolved
@mjcarroll mjcarroll force-pushed the reduce_simulationrunner branch from 87bf128 to cd5552e Compare February 21, 2022 19:51
@mjcarroll
Copy link
Contributor Author

@osrf-jenkins retest this please

@mjcarroll mjcarroll force-pushed the reduce_simulationrunner branch from cd5552e to 9a577cb Compare February 24, 2022 21:40
@mjcarroll
SystemInternal and WorldControl into own headers
5b974b3 
Reduces the size of simulationrunner header a bit

Signed-off-by: Michael Carroll <michael@openrobotics.org>
@mjcarroll
Move System interaction to SystemManager
f885cf5 
Signed-off-by: Michael Carroll <michael@openrobotics.org>
@mjcarroll
Adjustments for Fortress
af90891 
Signed-off-by: Michael Carroll <michael@openrobotics.org>
@mjcarroll
Address reviewer feedback
4cb543e 
Signed-off-by: Michael Carroll <michael@openrobotics.org>
@mjcarroll mjcarroll force-pushed the reduce_simulationrunner branch from 9a577cb to 4cb543e Compare February 25, 2022 16:32
@mjcarroll mjcarroll mentioned this pull request Feb 25, 2022
Merged
8 tasks
@mjcarroll
Fix another sequencing issue
74ce41b 
Signed-off-by: Michael Carroll <michael@openrobotics.org>
@mjcarroll
Hide the system manager on Windows
4d05ad5 
Signed-off-by: Michael Carroll <michael@openrobotics.org>
@mjcarroll mjcarroll force-pushed the reduce_simulationrunner branch from 714836a to 4d05ad5 Compare February 25, 2022 18:40
@mjcarroll mjcarroll merged commit d03a77e into ign-gazebo6 Feb 25, 2022
@mjcarroll mjcarroll deleted the reduce_simulationrunner branch February 25, 2022 22:23
mjcarroll added a commit that referenced this pull request Feb 25, 2022
@mjcarroll
Refactor System functionality into SystemManager (#1340)
75085eb 
SimulationRunner is getting long/crowded, so I'm refactoring out a bit of the System-oriented functionality into it's own Manager class as a peer to things like EventsManager and LevelManager. This should make it a bit easier to test, as well as reduce the clutter in SimulationRunner

Signed-off-by: Michael Carroll <michael@openrobotics.org>
@chapulina chapulina added 🏯 fortress Ignition Fortress and removed 🌱 garden Ignition Garden labels Mar 10, 2022
@chapulina chapulina mentioned this pull request Mar 10, 2022
Merged
7 tasks
@osrf-triage
Copy link

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-ignition-releases-2022-03-25-fortress-edifice-citadel/1343/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nkoenig nkoenig nkoenig approved these changes

@azeey azeey azeey approved these changes

@chapulina chapulina Awaiting requested review from chapulina

@mabelzhang mabelzhang Awaiting requested review from mabelzhang

@iche033 iche033 Awaiting requested review from iche033

Assignees

@mjcarroll mjcarroll

Labels
🏯 fortress Ignition Fortress
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mjcarroll @nkoenig @osrf-triage @azeey @chapulina