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

Remove systems if their parent entity is removed #2232

Merged
merged 53 commits into from
Jul 17, 2024

Conversation

arjo129
Copy link
Contributor

@arjo129 arjo129 commented Nov 7, 2023

🦟 Bug fix

Fixes #2217

Summary

In particular if a user despawns an entity, the associated plugin gets removed. This should prevent issues like #2165. TBH I'm not sure if this is the right way forward as a system should technically be able to access any entity in a traditional ECS.

no_crash

The PR has now been reworked and greatly simplified. All we do is stop all worker threads if an entity is removed and then recreaye remaining threads.

Checklist

  • Signed all commits for DCO
  • Added tests
  • 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.

🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸

@github-actions github-actions bot added the 🌱 garden Ignition Garden label Nov 7, 2023
Copy link

codecov bot commented Nov 8, 2023

Codecov Report

Attention: Patch coverage is 92.55319% with 7 lines in your changes missing coverage. Please review.

Project coverage is 64.78%. Comparing base (0f27ce0) to head (89a0f7d).
Report is 119 commits behind head on main.

Current head 89a0f7d differs from pull request most recent head f3c57d0

Please upload reports for the commit f3c57d0 to get more accurate results.

Files Patch % Lines
src/SystemManager.cc 89.83% 6 Missing ⚠️
src/systems/pose_publisher/PosePublisher.cc 90.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2232      +/-   ##
==========================================
- Coverage   65.64%   64.78%   -0.87%     
==========================================
  Files         324      358      +34     
  Lines       30937    29213    -1724     
==========================================
- Hits        20308    18925    -1383     
+ Misses      10629    10288     -341     

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

@jrutgeer
Copy link
Contributor

jrutgeer commented Nov 9, 2023

@arjo129

I thought of this possible alternative implementation:

  • Only the system plugin itself knows which entities are necessary for its functioning,
    • This could e.g. be the model entity it is attached to, but it could as well be any other entity,
    • Hence it's the system plugin's responsibility to identify those entities,
  • To identify these entities, the system plugin adds a component used_by_plugin to each entity it acts upon,
    • A used_by_plugin component of an entity identifies a plugin which acts on that entity,
  • Upon removal of entities, the ECM checks whether they have used_by_plugin components,
    • If so, the SystemManager is triggered to remove the corresponding plugin.

A possible extension could be to also have a kind of severity flag in the component, to allow for different reactions from the SystemManager depending on which entities are removed.

E.g.: consider a system attached to a model and which acts on some subentities of that model:

  • If the model entity is removed, the system should also be removed,
  • But if a subentity is removed, maybe the system should not be removed yet, but rather perform some 'reconfigure' action instead.

@arjo129
Copy link
Contributor Author

arjo129 commented Nov 9, 2023

I like the thought process behind your suggested API addition. However, in my mind that would be an added feature. We need to be able to remove systems safely. That is something that will take a fair deal of work right now and what this PR hopes to (eventually achieve). Your proposed API could be bolted on top in Ionic once we are able to perform safe removal. The current state of this PR is we can safely remove some plugins, but it causes weird breakages in the PosePublisher plugin which I need to investigate prior to making any changes. It is also entirely possible that I'm doing something wrong with the barriers. Either ways I need to revisit this another day when my head is clearer and I have more time.

image

@arjo129
Copy link
Contributor Author

arjo129 commented Nov 24, 2023

I think this works now... Will have to write some unit tests to verify. But it does not go 💥 on the warehouse world.

@azeey
Copy link
Contributor

azeey commented Dec 1, 2023

I haven't had a chance to look into this deeply, but one thing to keep is how this interacts with the levels feature. If a model that contains a system is unloaded by the levels feature and gets loaded back again, what would be the behavior? Would the system get configured and start anew? If that works properly, that would cover most cases, but we might also want the ability for a system to stay loaded even when its parent entity is removed so that if the parent entity is loaded back in, the system might continue from where it left off.

@arjo129
Copy link
Contributor Author

arjo129 commented Dec 1, 2023

Thanks for pointing that out. I'll go check out levels and see how this PR interacts with it. A cursory reading of the levels code though makes it seem like we despawn entities and then respawn them which makes me wonder if holding a reference "entity" in a system is correct. I've not looked into this deeply enough to say anything. Will update this PR when I try it. I'm currently out on my anual firefighting reservist.

@Bogdanov-am
Copy link

Is any work ongoing in this PR? I'm currently working on a simulation that requires frequent loading and unloading of models. The problem of unloaded plugins really worries me.

@arjo129
Copy link
Contributor Author

arjo129 commented Mar 6, 2024

I just need to write some tests for this.

@arjo129 arjo129 marked this pull request as ready for review March 18, 2024 08:25
@arjo129 arjo129 requested a review from mjcarroll as a code owner March 18, 2024 08:25
@arjo129 arjo129 changed the base branch from gz-sim7 to main March 18, 2024 08:26
@arjo129 arjo129 added 🏛️ ionic Gazebo Ionic and removed 🌱 garden Ignition Garden labels Mar 18, 2024
@arjo129
Copy link
Contributor Author

arjo129 commented Mar 18, 2024

Tests have been written. I think this is ready for feedback.

@landersson
Copy link

Also hoping to see this merged soon.

@arjo129
Copy link
Contributor Author

arjo129 commented May 2, 2024

If you'd like to get this checked in faster you could help with reviewing it. One thing to do is to checkout the relevant branch and run it on example worlds to see that things work as expected.

@arjo129 arjo129 requested a review from azeey May 2, 2024 02:10
@landersson
Copy link

If you'd like to get this checked in faster you could help with reviewing it. One thing to do is to checkout the relevant branch and run it on example worlds to see that things work as expected.

Unfortunately, I don't have any experience with the gz-sim codebase and can't really comment on whether this is a suitable solution or not.

Just need a way to remove models without plugins like JointStatePublisher crashing.

@arjo129
Copy link
Contributor Author

arjo129 commented May 2, 2024

Unfortunately, I don't have any experience with the gz-sim codebase and can't really comment on whether this is a suitable solution or not.

You can try it out and tell me if it crashes! There may be plugins I've not tested on!

src/SystemManager.hh Outdated Show resolved Hide resolved
Copy link
Member

@scpeters scpeters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: fix spelling in header file name

src/SystemCointainer.hpp Outdated Show resolved Hide resolved
src/SystemContainer_TEST.cc Outdated Show resolved Hide resolved
src/SystemManager.cc Outdated Show resolved Hide resolved
src/SystemManager.hh Outdated Show resolved Hide resolved
src/SystemContainer.hpp Outdated Show resolved Hide resolved
src/SimulationRunner.cc Outdated Show resolved Hide resolved
@azeey azeey requested a review from scpeters June 10, 2024 18:38
Copy link
Member

@scpeters scpeters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks good to me! I just noted two places where comments could be improved, and then I will be ready to approve

src/SimulationRunner.hh Outdated Show resolved Hide resolved
src/SystemManager.cc Outdated Show resolved Hide resolved
@azeey
Copy link
Contributor

azeey commented Jul 11, 2024

See #2473

azeey and others added 2 commits July 12, 2024 16:51
…h interface (#2473)


---------

Signed-off-by: Addisu Z. Taddese <[email protected]>
Signed-off-by: Steve Peters <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
Co-authored-by: Steve Peters <[email protected]>
Co-authored-by: Arjo Chakravarty <[email protected]>
@azeey
Copy link
Contributor

azeey commented Jul 15, 2024

I haven't had a chance to look into this deeply, but one thing to keep is how this interacts with the levels feature. If a model that contains a system is unloaded by the levels feature and gets loaded back again, what would be the behavior? Would the system get configured and start anew? If that works properly, that would cover most cases, but we might also want the ability for a system to stay loaded even when its parent entity is removed so that if the parent entity is loaded back in, the system might continue from where it left off.

I've confirmed that when a level that contains a system get unloaded and loaded back again, the associated systems are also unloaded and loaded properly 🎉 . Regarding the ability for a system to stay loaded and continue where it left off, I propose we open a feature request and not address it right now since it's probably not going to be widely used.

Since there are a number of bug reports around this issue and since levels will work fine with this, I'm in favor of backporting this to Harmonic and earlier.

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 this much needed feature/bug fix and for your patience in iterating!

@landersson
Copy link

Thanks everyone for your efforts, I will be making use of this ASAP.

@arjo129 arjo129 enabled auto-merge (squash) July 15, 2024 23:14
@arjo129
Copy link
Contributor Author

arjo129 commented Jul 15, 2024

Thanks everyone for your guidance! Seems like our homebrew build is blocking the merge.

@scpeters
Copy link
Member

Thanks everyone for your guidance! Seems like our homebrew build is blocking the merge.

I can reproduce the test failures on my macOS machine. I'm investigating, but it's a bit strange.

The test failures may have been introduced by #2473 or #2475, since macOS CI did not run for either of those pull requests.

@scpeters
Copy link
Member

Thanks everyone for your guidance! Seems like our homebrew build is blocking the merge.

I can reproduce the test failures on my macOS machine. I'm investigating, but it's a bit strange.

The test failures may have been introduced by #2473 or #2475, since macOS CI did not run for either of those pull requests.

  • I can reproduce the seg-faults when building normally and running tests with ctest
  • I tried reverting 7026b57, and it has no effect
  • The tests pass when run with lldb
  • The tests pass when recompiled with -DCMAKE_BUILD_TYPE=Debug

@scpeters
Copy link
Member

actually the tests have been failing on main since https://build.osrfoundation.org/job/gz_sim-ci-main-homebrew-amd64/68/

@scpeters
Copy link
Member

rerunning homebrew CI now that gazebosim/gz-math#606 and gazebosim/sdformat#1458 have been reverted after seeing that the reverts fixed tests in #2482

@arjo129 arjo129 merged commit 2994087 into main Jul 17, 2024
9 checks passed
@arjo129 arjo129 deleted the arjo/feat/remove_systems_with_entities branch July 17, 2024 17:39
arjo129 added a commit that referenced this pull request Jul 19, 2024
n particular if a user despawns an entity, the associated plugin gets removed. This should prevent issues like #2165. TBH I'm not sure if this is the right way forward as a system should technically be able to access any entity in a traditional ECS.

The PR has now been reworked and greatly simplified. All we do is stop all worker threads if an entity is removed and then recreate remaining threads.
arjo129 added a commit that referenced this pull request Jul 22, 2024
n particular if a user despawns an entity, the associated plugin gets removed. This should prevent issues like #2165. TBH I'm not sure if this is the right way forward as a system should technically be able to access any entity in a traditional ECS.

The PR has now been reworked and greatly simplified. All we do is stop all worker threads if an entity is removed and then recreate remaining threads.
arjo129 added a commit that referenced this pull request Jul 26, 2024
n particular if a user despawns an entity, the associated plugin gets removed. This should prevent issues like #2165. TBH I'm not sure if this is the right way forward as a system should technically be able to access any entity in a traditional ECS.

The PR has now been reworked and greatly simplified. All we do is stop all worker threads if an entity is removed and then recreate remaining threads.
arjo129 added a commit that referenced this pull request Jul 26, 2024
n particular if a user despawns an entity, the associated plugin gets removed. This should prevent issues like #2165. TBH I'm not sure if this is the right way forward as a system should technically be able to access any entity in a traditional ECS.

The PR has now been reworked and greatly simplified. All we do is stop all worker threads if an entity is removed and then recreate remaining threads.

Signed-off-by: Arjo Chakravarty <[email protected]>
arjo129 added a commit that referenced this pull request Jul 26, 2024
n particular if a user despawns an entity, the associated plugin gets removed. This should prevent issues like #2165. TBH I'm not sure if this is the right way forward as a system should technically be able to access any entity in a traditional ECS.

The PR has now been reworked and greatly simplified. All we do is stop all worker threads if an entity is removed and then recreate remaining threads.

Signed-off-by: Arjo Chakravarty <[email protected]>
iche033 pushed a commit that referenced this pull request Aug 1, 2024
n particular if a user despawns an entity, the associated plugin gets removed. This should prevent issues like #2165. TBH I'm not sure if this is the right way forward as a system should technically be able to access any entity in a traditional ECS.

The PR has now been reworked and greatly simplified. All we do is stop all worker threads if an entity is removed and then recreate remaining threads.

Signed-off-by: Arjo Chakravarty <[email protected]>
iche033 pushed a commit that referenced this pull request Aug 2, 2024
n particular if a user despawns an entity, the associated plugin gets removed. This should prevent issues like #2165. TBH I'm not sure if this is the right way forward as a system should technically be able to access any entity in a traditional ECS.

The PR has now been reworked and greatly simplified. All we do is stop all worker threads if an entity is removed and then recreate remaining threads.

Signed-off-by: Arjo Chakravarty <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏛️ ionic Gazebo Ionic
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

System plugins keep executing after their model is removed.
6 participants