bevy icon indicating copy to clipboard operation
bevy copied to clipboard

Allow Commands to register systems

Open pablo-lua opened this issue 2 years ago • 9 comments

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

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

pablo-lua avatar Dec 18 '23 23:12 pablo-lua

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

pablo-lua avatar Dec 19 '23 00:12 pablo-lua

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.

alice-i-cecile avatar Dec 19 '23 00:12 alice-i-cecile

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

pablo-lua avatar Dec 19 '23 14:12 pablo-lua

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

doonv avatar Dec 19 '23 22:12 doonv

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?

pablo-lua avatar Dec 21 '23 23:12 pablo-lua

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.

alice-i-cecile avatar Dec 22 '23 01:12 alice-i-cecile

I think that latter is more robust

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

pablo-lua avatar Dec 22 '23 10:12 pablo-lua

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

alice-i-cecile avatar Dec 23 '23 05:12 alice-i-cecile

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

pablo-lua avatar Dec 24 '23 13:12 pablo-lua

I'll solve conflicts today :)

pablo-lua avatar Mar 22 '24 10:03 pablo-lua