amethyst / shred

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

Implemented batch dispatching #147

Closed AndreaCatania closed 5 years ago

AndreaCatania commented 5 years ago

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

WaDelma commented 5 years ago

Could the project be an example? As in add it to the examples folder.

AndreaCatania commented 5 years ago

Done!

AndreaCatania commented 5 years ago

@torkleyy I did all the changes that you advised.

ThreadPool

About the ThreadPool assignment problem that I mentioned you before, I found a solution that seems nice and elegant.

I've created a struct, called ThreadPoolWrapper, which is shared across all batches. This wrapper contains the actual ThreadPool that now can be modified at any point.

The result of this addition is that when in amethyst the Application struct create the thread pool and assign it to the main DispatcherBuilder it change the internal part of the wrapper and everyone is automatically updated.

To me this seems clear enough but do you think that exist a better way to handle it?

Tests

I written three tests that demonstrate what you suggested, please let me know if I have to add more.

Commit squash

Do you want that I squash all this changes to a single commit?

AndreaCatania commented 5 years ago

@WaDelma I just did a commit to resolve the things that you pointed out

AndreaCatania commented 5 years ago

@WaDelma another commit done to fix the last pointed things.

WaDelma commented 5 years ago

I also realise that the suggestion thing is not really suited for multiline suggestions and it makes it bit hard to read what I changed. Sorry about that. (Gitlab allows multiline suggestions)

AndreaCatania commented 5 years ago

Any news for this?

bors[bot] commented 5 years ago

:v: jojolepro can now approve this pull request

torkleyy commented 5 years ago

bors r+

bors[bot] commented 5 years ago

Build succeeded

AndreaCatania commented 5 years ago

Guys, thank you a lot!

AndreaCatania commented 5 years ago

I would like to update shred in Amethyst and so use this new feature. @torkleyy can I help you to add a new version in crates.io?

AnneKitsune commented 5 years ago

I'd just like to precise that I reviewed only half of the PR yesterday (lack of time), which is why I didn't add an "accept" review thingy. However, it as been reviewed by 2 people, so I would tend to think that I would not have found more than 1-2 nitpicks in the second half.

AndreaCatania commented 5 years ago

@torkleyy, @jojolepro any idea about how to create a new version in crates.io? otherwise I can't use it

AnneKitsune commented 5 years ago

It depends when torkleyy actually wants to release. I'll ask him.

In the meantime, you can target the git repo using cargo using:

shred = { git = "https://github.com/slide-rs/shred" }
specs = { git = "https://github.com/your_specs_fork" }

For that, you'll need to fork specs and update that Cargo.toml file too so that it targets this repo.

AndreaCatania commented 5 years ago

@jojolepro Thanks for the advice! I wanted to avoid it but that's ok, I can continue to work on phythyst now.