Remove Sync bound on Component and Resource (adopted) by JohnTheCoolingFan · Pull Request #5879 · bevyengine/bevy

JohnTheCoolingFan · 2022-09-04T18:52:41Z

Adoption of #4680

Objective

Fixes #2487. Remove the Sync bound on Component and Resource to allow Send-only components.

Solution

Remove Sync bound from Component and Resource.
Address the compilation errors.
Changed the PhantomData<T> in FetchState and SystemParamState to use PhantomData<fn() -> T> to ensure they're Sync.
Add a compile_fail_test.
Add Sync bound on the WorldQuery impl for &T.
Add Sync bound on Res<T> and Option<Res<T>>
Add Sync bound on Query::get_component, World::get, EntityRef::get, and EntityMut::get.

Changelog

Removed: Sync bound on Component and Resource.

Migration Guide

Component and Resource no longer requires Sync. Any generic code using Component as a bound, &T as Query parameter, or Res<T> might require an additional Sync bound.

JohnTheCoolingFan · 2022-09-04T19:11:09Z

Currently working on resolving merge conflicts

alice-i-cecile · 2022-09-04T20:47:26Z

Sounds good :) Let us know if you have any questions or need help.

bjorn3 · 2022-09-05T13:12:43Z

crates/bevy_ecs/src/schedule/stage.rs


 /// A type that can run as a step of a [`Schedule`](super::Schedule).
-pub trait Stage: Downcast + Send + Sync {
+pub trait Stage: Downcast + Send {


Doesn't schedule.get_stage() depend on stages being Sync?

Same for .iter_stages().

bjorn3 · 2022-09-05T13:16:56Z

crates/bevy_ecs/src/world/mod.rs

    pub fn get_resource_by_id(&self, component_id: ComponentId) -> Option<Ptr<'_>> {
        let info = self.components.get_info(component_id)?;
-        if !info.is_send_and_sync() {
+        if !info.is_send() {


This should check for sync, right?

JohnTheCoolingFan · 2022-09-05T16:43:07Z

Currently can't figure out how to make impl WorldQuery for &mut T for !Sync T

BoxyUwU · 2022-09-05T17:10:56Z

crates/bevy_ecs/src/query/fetch.rs

+/// Non-`Sync` components are supported, but cannot be accessed as `&T`,
+/// as the type cannot be safely read from multiple threads at the same
+/// time. These components must be accessed via a `&mut T` to guarentee
+/// that the system has exclusive access to the components.


this isnt going to work and imo is flawed anyway, why shouldn't you be able to use &T from multiple places in a system?
imo we should do one of:

Add NonSync versions of &T and &mut T, leaving the existing types requiring T: Sync

Remove T: Sync bound from &T and &mut T worldqueries (this is a bad idea since now we're going to be registering almost eveyr component as !Sync)

Add a NonSync<T> and use that as the readonly version of &mut T (this is a bad idea since its going to screw up the Query::to_readonly effort by making the readonly version of Query<&mut T> not be Query<&T>)

I am assuming here that the scheduler is smart enough to know not to schedule two systems with read only access of a !Sync component in parallel, if not that should be fixed

Why not just add NonSyncRead<T> for the 'use &T from multiple places in a system' case?

Fundamentally, &mut T requiring T: Sync is entirely superfluous - the requirement that T is Sync would never be used.

Oh I see, because we need to convert to readonly. That's annoying.
I don't suppose we can gate a readonly conversion existing on T being Sync? Is conversion to readonly really a fundamental part of the Query API?

conversion to readonly doesnt require it. we could after all set the readonly version of &mut T to be NonSync<T>. It's just kinda silly to have the readonly version of &mut T not be &T.

Is conversion to readonly really a fundamental part of the Query API?

I think so? It's not quite there yet but I hope that in a bit of time we'll have much more flexibility around changing the Q/F generics and being able to downgrade a query to readonly is a part of that. Personally I value to_readonly working on everything a lot more than some extreme edge case of non sync components needing to use NonSyncMut<T> instead of &mut T

BoxyUwU · 2022-10-17T19:06:41Z

@JohnTheCoolingFan are you still working on this? I'd like to get !Send !Sync components implemented to make World: !Send + !Sync a more usable option for solving some other bevy issues

JohnTheCoolingFan · 2022-10-17T19:37:05Z

No, I just resolved the merge conflicts in an attempt to bring some attention and possibly merge. I now realise that I do not have the knowledge and skills to further maintain this PR.
@BoxyUwU

BoxyUwU · 2022-10-18T01:51:13Z

I'll make a PR for this then 👍

james7132 added 17 commits May 6, 2022 13:19

Bare minimum: Get it to compile

87ec70a

Update bounds on Query/World/Entity{Ref/Mut}

16e86ac

Add compile_fail_test and update Component derive macro

998b4bf

Formatting

dd6fcd3

unsafe impl -> PhantomData<fn() -> T>

f101e84

Fix examples

61f4d7c

Fix examples

01669da

Update paths for compile tests

62d0c4b

Attempt removing Sync on Resource

4feca9d

Formatting k

3f7a0a3

Remaining unsafe impl

6deee18

Use TRYBUILD='overwrite'

7b40740

Convert compile_fail_tests to doctests

dd63b22

Add error codes for compile_fail tests

e6cb45c

Merge branch 'main' into relax-sync-bound

f5b2b4b

Remove unecessary bounds

f1e3983

is_send_and_sync -> is_send

6251294

alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Sep 4, 2022

alice-i-cecile requested review from BoxyUwU and DJMcNab September 4, 2022 20:46

alice-i-cecile mentioned this pull request Sep 4, 2022

Remove Sync bound on Component and Resource #4680

Closed

Merge branch 'relax-sync-bound'

799f197

bjorn3 reviewed Sep 5, 2022

View reviewed changes

JohnTheCoolingFan added 3 commits September 5, 2022 16:29

Add a Sync bound on impl Plugin for CameraProjectionPlugin

25f5b80

Add a Sync bound on ExtractResource::Source

02c35f9

Remove Sync bound in impl WorldQuery for &mut T

b8ca58a

BoxyUwU reviewed Sep 5, 2022

View reviewed changes

alice-i-cecile added the S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! label Oct 17, 2022

BoxyUwU removed the S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! label Oct 18, 2022

alice-i-cecile closed this Oct 18, 2022

Uh oh!

Conversation

JohnTheCoolingFan commented Sep 4, 2022

Objective

Solution

Changelog

Migration Guide

Uh oh!

JohnTheCoolingFan commented Sep 4, 2022

Uh oh!

alice-i-cecile commented Sep 4, 2022

Uh oh!

bjorn3 Sep 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bjorn3 Sep 5, 2022

Choose a reason for hiding this comment

Uh oh!

bjorn3 Sep 5, 2022

Choose a reason for hiding this comment

Uh oh!

JohnTheCoolingFan commented Sep 5, 2022

Uh oh!

BoxyUwU Sep 5, 2022

Choose a reason for hiding this comment

Uh oh!

DJMcNab Sep 5, 2022

Choose a reason for hiding this comment

Uh oh!

DJMcNab Sep 5, 2022

Choose a reason for hiding this comment

Uh oh!

BoxyUwU Sep 5, 2022

Choose a reason for hiding this comment

Uh oh!

BoxyUwU commented Oct 17, 2022

Uh oh!

JohnTheCoolingFan commented Oct 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BoxyUwU commented Oct 18, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

bjorn3 Sep 5, 2022 •

edited

Loading

JohnTheCoolingFan commented Oct 17, 2022 •

edited

Loading