Skip to content

Allow Commands to register systems#11019

Merged
alice-i-cecile merged 27 commits intobevyengine:mainfrom
pablo-lua:register_system_with_commands
Mar 22, 2024
Merged

Allow Commands to register systems#11019
alice-i-cecile merged 27 commits intobevyengine:mainfrom
pablo-lua:register_system_with_commands

Conversation

@pablo-lua
Copy link
Contributor

@pablo-lua pablo-lua commented Dec 18, 2023

Objective

  • Allow registering of systems from Commands with Commands::register_one_shot_system
  • Make registering of one shot systems more easy

Solution

  • Add the Command RegisterSystem for Commands use.
  • Creation of SystemId based on lazy insertion of the System
  • Changed the privacy of the fields in SystemId so Commands can return the SystemId

Changelog

Added

  • Added command RegisterSystem
  • Added function Commands::register_one_shot_system
  • Added function App::register_one_shot_system

Changed

  • Changed the privacy and the type of struct tuple to regular struct of SystemId

Migration Guide

  • Changed SystemId fields from tuple struct to a normal struct
    If you want to access the entity field, you should use SystemId::entity instead of SystemId::0

Showcase

Before, if you wanted to register a system with Commands, you would need to do:

commands.add(|world: &mut World| {
    let id = world.register_system(your_system);
    // You would need to insert the SystemId inside an entity or similar
})

Now, you can:

let id = commands.register_one_shot_system(your_system);
// Do what you want with the Id

@ItsDoot ItsDoot added C-Feature A new feature, making something new possible A-ECS Entities, components, systems, and events labels Dec 18, 2023
@pablo-lua
Copy link
Contributor Author

While at it, is it a good Idea to add tests for testing Commands::run_system and Commands::register_system in system_registry.rs?

@alice-i-cecile
Copy link
Member

I will never say no to more tests :D I think in this case we should probably split them out into a seperate PR if we can: I suspect they'll be much faster to merge that way.

@pablo-lua
Copy link
Contributor Author

pablo-lua commented Dec 19, 2023

Regarding the matter about verifying if the System was already registred before, found out something

fn do_something() {}
fn do_another_something() {}

// If we cast the function into an pointer, we can found out the actual address of the function, which can be very useful

assert_ne!(do_something as usize, do_another_something as usize);

// That same thing is possible with closure, with a little problem though

let closure_one  = || {};
let closure_two = || {};

assert_ne!(closure_one as fn() as usize, closure_two as fn() as usize);

// Closures must be casted first as a function, then into an address for this to work

// Last, with the same problem, closures created from a function can't be casted into an Address
// But if you pass the function into a var first, it can later be casted into an address from inside the function

// This works
fn get_closure_address() -> usize {
   let closure = || {};
   closure as fn() as usize
} 

// This doesn't
fn get_closure_address() -> impl Fn() {
   || {}
}
let closure = get_closure_address();
closure as fn() as usize // Can't be casted

Maybe that can be useful

@doonv
Copy link
Contributor

doonv commented Dec 19, 2023

It would be cool if you added Commands::register_one_shot_system, but maybe that's better as an App method.

@pablo-lua
Copy link
Contributor Author

Made an doc-test, but not sure if that is the best doc-test In this situation. Must I assert that the function is indeed an System or maybe assert that the run will make the Counter value change?

@alice-i-cecile
Copy link
Member

Made an doc-test, but not sure if that is the best doc-test In this situation. Must I assert that the function is indeed an System or maybe assert that the run will make the Counter value change?

I think the latter is more robust.

@pablo-lua
Copy link
Contributor Author

pablo-lua commented Dec 22, 2023

I think that latter is more robust

You mean that's too much for this doc Test or more robust like "better"?

@alice-i-cecile
Copy link
Member

Better :) Less likely to pass while the functionality is broken.

@pablo-lua
Copy link
Contributor Author

pablo-lua commented Dec 24, 2023

Done it, now the doc verify both: If the function is a system and if the run affect the counter, asserting that the system is registered

Copy link
Contributor

@jdm jdm left a comment

Choose a reason for hiding this comment

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

Nice usability improvement, and I found the example very helpful!

@alice-i-cecile alice-i-cecile added X-Needs-SME This type of work requires an SME to approve it. and removed X-Needs-SME This type of work requires an SME to approve it. labels Jan 28, 2024
@pablo-lua pablo-lua added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Mar 22, 2024
@pablo-lua
Copy link
Contributor Author

I'll solve conflicts today :)

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Mar 22, 2024
Merged via the queue into bevyengine:main with commit 78335a5 Mar 22, 2024
@pablo-lua pablo-lua deleted the register_system_with_commands branch March 22, 2024 18:05
github-merge-queue bot pushed a commit that referenced this pull request Jul 27, 2024
# Objective

- `SystemId`'s `Debug` implementation includes its `entity` field twice.
- This was likely an oversight in #11019, since before that PR the
second field was the `PhantomData` one.

## Solution

- Only include it once

Alternatively, this could be changed to match the struct representation
of `SystemId`, thus instructing the formatter to print a named struct
and including the `PhantomData` field.
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 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.

6 participants