In the past, many users had difficulties with panics just saying "No resource with the given id". This is neither beginner friendly, nor a good way to handle the error.

The default behavior has been changed to adding the missing resource automatically. For this, a Default implementation needs to be in place.

In case a resource does not have a Default implementation, it's recommended that one uses Option<Fetch> and checks whether the resource exists or not. If the system cannot work without the resource, it should simply return.

In case the resource is absolutely required and the user is sure it exists, they can use FetchExpect. It is pretty much like Fetch was before, but now the panic message has been improved and there's a nightly feature that also prints the type name of the missing resource.

The overall goal is to make shred more robust, easier to debug and beginner-friendly.

This change is 

Hello

As mentioned in #197, I've sketched some API how the batch dispatching could be improved so it's less awkward and doesn't require unsafe.

This is very much a PoC/WiP, I don't want to merge it as it is. I know there are missing docs, copy-pasted comments that are out of context, extra includes that are no longer needed, some things are public when in fact shouldn't be and in general a lot of garbage. I plan to clean it up before asking for it to get merged.

What I would like now is to have some feedback on the API. I have some specific questions, but any other suggestions are fine too:

• Naming. In particular, the simpler case where it is decided up-front how many times the sub-dispatcher should run is currently called multi-batch. I think this is a bad name, because it suggests it's somehow „more“ than the usual but more complex case.
• The multi-batch is a separate method of the builder. Alternatively, it could be exposed as the wrapper type and one could create it directly. I'm not sure on what's better.

In general, however, I feel like this makes the API more approachable (obviously, after it gets some docs and examples).

@azriel91 Do you think I should continue in this? (and, btw, there's a tiny doc update PR from me sitting there for some time as well).

This is the second iteration to implement the Batch dispatching following the feedback received in this PR: https://github.com/slide-rs/shred/pull/144 (Was easier for me redo everything rather change it)

This is the project to test this feature: shred_test.zip

A small update of the FxHashMap HashMap to use the more perfomante hashbrown HashMap.

hashbrown is a Rust port of Google's high-performance SwissTable hash map and is around 2 times faster than FxHashMap.

The current code for defining a new task looks like this (from the README)

struct PrintSystem;

impl<'a> System<'a> for PrintSystem {
    type SystemData = PrintData<'a>;

    fn work(&mut self, bundle: PrintData<'a>) {
        println!("{:?}", &*bundle.a);
        println!("{:?}", &*bundle.b);
        
        *bundle.b = ResB; // We can mutate ResB here
                          // because it's `FetchMut`.
    }
}

Every system needs an additional struct (often unit type from my own experience) which becomes quite verbose. Alternative might be replace the Task trait with a function:

fn system_print(_: &mut (), bundle: PrintData<'a>) {
    println!("{:?}", &*bundle.a);
    println!("{:?}", &*bundle.b);
    *bundle.b = ResB;
}

The dispatch builder would then take a F: FnMut(&mut T, U), U: SystemData (or something similar).

let mut dispatcher = DispatcherBuilder::new()
    .add((), system_print, "print", &[]) // Adds a system "print" without dependencies
    .finish();

I'm not 100% sure if this will be possible, tests some time ago looked positive..

Hey!

So to start out: this is an experiment. I don't expect it to be merged, but I also want to make sure to document that I took this approach for a spin.

I was reading the shred code to try to understand what the lifetime associated to systems was actually used for.

The lifetime appears to be required to have a named lifetime in the fetch function that is also associated with the return value (e.g. Self).

This guarantees that Resources cannot be modified, while the fetch function is running.

I then tried to figure out what the scope of this lifetime would be. And it appears like this doesn't actually come into effect that much. Primarily it's used in RunNow: https://github.com/slide-rs/shred/blob/master/src/system.rs#L124

But here is the kicker: resources is already borrowed here, so do we really need to track the lifetime of the values borrowed from it? These should already be valid for the duration of this function call, which calls into the System itself.

So this is a patch that removes that lifetime, in favor of making Read and Write keep track of a raw pointer into TrustCell.

Some major caveats

Resources wrapped inRead and Write must never, ever, ever be able to leave or be stored in the System. This is normally accomplished with the lifetime associated with the system itself. This could probably be patched by treating them as weak pointers to a specific generation of the collection of resources, and panic in case someone tried to use them if this generation has changed.

Some APIs have been completely butchered for their existing safety guarantees, and some safety holes are available as APIs. E.g. it's possible to discard and SystemFetch because that's needed to perform composition of fetches.

To wrap up: this is probably a terrible idea. It was fun to try out, but please don't merge :).

cc @nchashch

This is how I imagined to have those systems divided into stages:

let dispatcher = DispatcherBuilder::new()
    .add(InputHandler, "handle_input", &[])
    .add(NetworkSynchronizer, "netsync", &[])
    .new_stage()
    .add(ForceGenerator, "force", &[])
    .add(Integrator, "integrator", &["force"])
    .add(EnemySpawner)
    .new_stage()
    .add(Renderer, "render", &[])
    .add(Audio, "audio", &[])
    .build();

cc https://github.com/amethyst/amethyst/issues/238

Updates the requirements on cgmath to permit the latest version.

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting @dependabot rebase.

Note: This repo was added to Dependabot recently, so you'll receive a maximum of 5 PRs for your first few update runs. Once an update run creates fewer than 5 PRs we'll remove that limit.

You can always request more updates by clicking Bump now in your Dependabot dashboard.

Adds a contains method to DispatcherBuilder that allows the caller to determine if a system with the given name has already been added.

This supports https://github.com/amethyst/specs/issues/736.

Hello

I've been mulling a bit over the batch execution API. To be honest, I don't find it very comfortable to use. I see several problems:

• It's verbose. To use it, you have to implement two traits (System and BatchController), and for the System you have to implement even the methods that you don't really care about most of the time (eg. accessor and setup).
• Unsafe is sticking out of it even though there's nothing the user needs to uphold towards the library or anything unsafe the user wants to do.
• As the controller is passed as a type parameter, not as a value, there's no (reasonable) way to pass additional info into it.

If my guess is correct, the System is used mostly to make the implementation easier, because the Dispatcher just works with Systems all the time.

I have a proposal how to make the interface better while not introducing much more complexity.

Let's have a trait, something like this (modulo naming, lifetimes...):

trait BatchController {
    type SystemData: SystemData;
    type Tmp;
    fn run_pre(&mut self, data: Self::SystemData) -> Self::Tmp;
    fn run(&mut self, tmp: Self::Tmp, dispatcher: DispatcherProxy);
}

Then, internally, there would be some wrapper that would implement the System, hold the dispatcher, and the instance of this new BatchController.

The relevant part of the example would look something like this:

// No data needed here, the accessor and dispatcher are part of the wrapper below the surface.
struct CustomBatchControllerSystem;

impl BatchController for CustomBatchControllerSystem }
    type SystemData = Read<TomatoStore>;
    type Tmp = TomatoStore;
    fn run_pre(&mut self, data: Self::SystemData) -> Self::Tmp {
        *data
    }
    fn run(&mut self, _ts: TomatoStore, dispatcher: DispatcherProxy) {
      for _i in 0..3 {
         // The BatchUncheckedWorld is part of the proxy
         dispatcher.dispatch();
      }
}

...
DispatcherBuilder::new()
    .with(SayHelloSystem, "say_hello", &[])
    .with_batch(
         CustomBatchControllerSystem, // Passing value, not a type
         DispatcherBuilder::new().with(..)
         "BatchSystemTest",
         &[],
     )
     .build();

If you like the idea, I can try making a PR with the changes.

Alternatively, it would also be possible to create just the wrapper that would implement the current traits. That would preserve the current API, but the cost is that it would still not be possible to pass values in there, only types.

tynm 0.1.2 used the todo!() macro which is only stable in Rust 1.40. tynm 0.1.3 switches that to unimplemented!() to be compatible with Rust 1.38.

This change ensures the fix is used (though consumers may already receive it simply with cargo update).

I don't know enough about the inner workings of shred to know if this is actually a bug, but it looks like one.

DispatcherBuilder::add_batch() has what looks like a copy/paste error:

let mut reads = dispatcher_builder.stages_builder.fetch_all_reads();
reads.extend(<T::BatchSystemData as SystemData>::reads());
reads.sort();
reads.dedup();

let mut writes = dispatcher_builder.stages_builder.fetch_all_writes();
writes.extend(<T::BatchSystemData as SystemData>::reads());                 <<<<< here
writes.sort();
writes.dedup();

let accessor = BatchAccessor::new(reads, writes);

When determining the write accesses for the batch being added, it calls reads() instead of writes(). It seems like this could pretty negatively affect the ability to parallelize things around the batch.

I would make a PR, but I wasn't sure if there was actually a reason for doing things this way or not.

Hi,

After experimenting a bith with Amethyst, I started wondering why the System trait is responsible for both setup and run (+dispose). I programmed my setup to fetch a resource, then store it as a struct member, but I am forced to wrap it inside an Option and unwrap it upon each call of run. Ideally I would have the System trait not having the run or dispose method, but move those to another trait. The setup would then consume the system and return an instance of this new trait. In this way there will be no more need for me to wrap the resource during setup.

I tried to tinker with shred's code a bit myself, and it becomes clear that this is not a minor code change. Obviously it would break the API as well. So my question is : will this concept be in a future release ?

There seems to be no need to have a lock around an option wrapping an Arc - since we have an (allocated) Arc, we can just use its raw pointer for AtomicPtr, and swap it out whenever the pool should be changed.

cc @AndreaCatania

From what I've seen, there doesn't seem to be the ability to suspend Systems? Basically, after exploring through amethyst and specs, I ended up concluding that shred would presumably be where this functionality would go. Feel free to direct me in the right place if that's not the case.

Right now, I know that amethyst supports a Pausable wrapper which simply checks a condition every time the system is run; this is okay, but it would be nice to be able to suspend a system (making it no longer dispatched) and be able to later signal it to be added back to the queue. Presumably, this would help make it easier to interface with Generators and the like later on.

Alternatively, you could argue that the best case for this is to have multiple Dispatchers and use the right one depending on the state of the system. This might be better for cases where the set of currently running systems is swapped, e.g. a pause menu in a game, rather than the generic case which might be better for I/O. If this is the route that seems best, then I can move this issue to specs and discuss that there.

Anyway, I didn't see a discussion on this, so, I figured I'd start one.

It would be useful to be able to inspect the plans shred comes up with, e.g. what systems execute in parallel when, and so on. It would also be useful to be able to get information about the execution times of various systems after a dispatch. I could imagine writing a tool that hooks into shred and generates a graphviz .dot file that lets you debug systems causing bottlenecks, or an amethyst editor plugin that lets you watch the relative execution times of your various systems in real time.