amethyst / shred

Shared resource dispatcher
Apache License 2.0
233 stars 66 forks source link

Potential bug with DispatcherBuilder::add_batch() #221

Closed shimaowo closed 1 year ago

shimaowo commented 1 year ago

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.