Skip to content

Conversation

@grind086
Copy link
Contributor

@grind086 grind086 commented Nov 11, 2024

Objective

Fixes #16266

Solution

Added an UnregisterSystem command struct and Commands::unregister_system. Also renamed World::remove_system and World::remove_system_cached to World::unregister_*

Testing

It's a fairly simple change, but I tested locally to ensure it actually works.

@github-actions
Copy link
Contributor

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@BenjaminBrienen BenjaminBrienen added C-Feature A new feature, making something new possible A-ECS Entities, components, systems, and events D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Nov 11, 2024
@BenjaminBrienen
Copy link
Contributor

Can you add a unit test?

@grind086
Copy link
Contributor Author

Are there any existing tests to use as an example? I couldn't find any for the related commands other than one doctest example for Commands::run_system, but I could just be missing them if they're somewhere other than system::commands::tests or system::system_registry::tests. If not I can come up with something.

@BenjaminBrienen
Copy link
Contributor

If there aren't any tests for register_system, don't worry about it. We can open a separate issue for adding tests there. (Unless you feel like doing it here)

@hymm
Copy link
Contributor

hymm commented Nov 11, 2024

I feel like this shouldn't be called remove_system. That name feels like the opposite of add_systems, so I would expect it to work with Schedules.

@grind086
Copy link
Contributor Author

I could change it to unregister_system like the original issue suggested if that's preferable. I went with the current name because it's consistent with World::remove_system, which is the non-command equivalent.

@alice-i-cecile
Copy link
Member

Both this and the method it calls should be named unregister_system. Once that's done, I'm happy to approve and merge.

Copy link
Contributor

@BenjaminBrienen BenjaminBrienen left a comment

Choose a reason for hiding this comment

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

I just wanted to try out the suggestion feature lol

grind086 and others added 2 commits November 12, 2024 15:41
Apply suggestions from code review

Co-authored-by: Benjamin Brienen <benjamin.brienen@outlook.com>
@grind086 grind086 changed the title Add remove_system command Add unregister_system command Nov 12, 2024
@grind086
Copy link
Contributor Author

I think that's everything, assuming checks pass.

@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Nov 12, 2024
@alice-i-cecile alice-i-cecile added this to the 0.15 milestone Nov 12, 2024
@alice-i-cecile
Copy link
Member

alice-i-cecile commented Nov 12, 2024

Squeezing into 0.15 because it avoids us shipping the bad remove_system method name. Thanks for tackling this and responding to feedback :)

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Nov 12, 2024
Merged via the queue into bevyengine:main with commit a8c610a Nov 12, 2024
30 checks passed
mockersf pushed a commit that referenced this pull request Nov 16, 2024
# Objective

Fixes #16266 

## Solution

Added an `UnregisterSystem` command struct and
`Commands::unregister_system`. Also renamed `World::remove_system` and
`World::remove_system_cached` to `World::unregister_*`

## Testing

It's a fairly simple change, but I tested locally to ensure it actually
works.

---------

Co-authored-by: Benjamin Brienen <benjamin.brienen@outlook.com>
ecoskey pushed a commit to ecoskey/bevy that referenced this pull request Dec 2, 2024
# Objective

Fixes bevyengine#16266 

## Solution

Added an `UnregisterSystem` command struct and
`Commands::unregister_system`. Also renamed `World::remove_system` and
`World::remove_system_cached` to `World::unregister_*`

## Testing

It's a fairly simple change, but I tested locally to ensure it actually
works.

---------

Co-authored-by: Benjamin Brienen <benjamin.brienen@outlook.com>
ecoskey pushed a commit to ecoskey/bevy that referenced this pull request Jan 6, 2025
# Objective

Fixes bevyengine#16266 

## Solution

Added an `UnregisterSystem` command struct and
`Commands::unregister_system`. Also renamed `World::remove_system` and
`World::remove_system_cached` to `World::unregister_*`

## Testing

It's a fairly simple change, but I tested locally to ensure it actually
works.

---------

Co-authored-by: Benjamin Brienen <benjamin.brienen@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add unregister_system for Commands

4 participants