`LazyUpdate::exec` and `LazyUpdate::exec_mut` should not require `F: Sync`

kyren commented 4 years ago

F must already be FnOnce, and the Sync requirement is too strict, it should only require Send when the parallel feature is enabled.

Unfortunately I couldn't come up with a clean way of using or modifying the parallel_feature macro to do this, so I was forced to copy the doc comments and implementation of exec / exec_mut. If you would rather make an additional parallel_feature macro or modify that in some way let me know.

Checklist

[ ] I've added tests for all code changes and additions (where applicable)
[ ] I've added a demonstration of the new feature to one or more examples
[ ] I've updated the book to reflect my changes
[ ] Usage of new public items is shown in the API docs

API changes

I don't think this is a breaking API change since it's strictly reducing the bounds of two LazyUpdate methods. It also removes requirements on two pub traits but they are marked "internal" so hopefully they shouldn't matter?

WaDelma commented 4 years ago

You could add new trait MaybeSend (or some better name) that either does or does not have Send as super trait depending on feature

azriel91 commented 4 years ago

Yeap, removing the bounds doesn't break API, though I find the fn exec taking a &mut World to be a bug... ish, introduced probably by me -- see lazy.rs@a5459b2:L262-L271, though before that it already held a &mut World and passed it as &World to the function.

Fixing that would be a breaking change, but people can simply use exec_mut anyway.

I'm just wondering if we should relax Sync for when "parallel" is enabled.

amethyst / specs

`LazyUpdate::exec` and `LazyUpdate::exec_mut` should not require `F: Sync` #691

Checklist

API changes