Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Component: Sync (might be) too strict. #2487

Closed
DJMcNab opened this issue Jul 15, 2021 · 6 comments
Closed

Component: Sync (might be) too strict. #2487

DJMcNab opened this issue Jul 15, 2021 · 6 comments
Labels
A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change C-Feature A new feature, making something new possible C-Performance A change motivated by improving speed, memory usage or compile times S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it!

Comments

@DJMcNab
Copy link
Member

DJMcNab commented Jul 15, 2021

What problem does this solve or what need does it fill?

Our requirements are too strict. This stops us from using types which are Send but not Sync in components, such as Cell<T>.

What solution would you like?

Remove the bound of Sync on component, and audit all the methods of World to ensure that any which go from &World->&T require T: Sync (primarily the SystemParam impl for &'_ T).

Note that the impl for &'_ mut T would not require Sync, since we guarantee that this reference is only on one thread (since the resultant Mut<T> can be used to get an &mut T directly).

What alternative(s) have you considered?

Keep things as they are - we haven't had a need for this yet, and it seems unlikely that we ever will.

However, philosophically there's no reason to have these bounds, and it strictly increases our usefulness.

Additional context

https://doc.rust-lang.org/nomicon/send-and-sync.html

https://doc.rust-lang.org/std/marker/trait.Sync.html

@DJMcNab DJMcNab added C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Jul 15, 2021
@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change and removed S-Needs-Triage This issue needs to be labelled labels Jul 15, 2021
@DJMcNab
Copy link
Member Author

DJMcNab commented Sep 29, 2021

So we've done a review of uses of NonSend, and failed to find a use of it where T: !Send - so this might be usable to 'parallelise' current uses of NonSend.

@alice-i-cecile alice-i-cecile added C-Performance A change motivated by improving speed, memory usage or compile times S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it! labels Dec 12, 2021
@alice-i-cecile
Copy link
Member

I would be happy to see a PR tackling this: I think that weakening trait bounds like this is generally good hygiene.

@btrepp
Copy link

btrepp commented Sep 29, 2022

I've run into this using a linear type pattern for a state machine. The statemachine API doesn't really support borrows. Everything is moved.

When integrating with bevy, I realized the state machine needs to implement sync (send is still logical) but doesn't. So I either need to change my data structure to make bevy happy, or put it in a mutex to make bevy happy :).

@harudagondi
Copy link
Member

When integrating with bevy, I realized the state machine needs to implement sync (send is still logical) but doesn't. So I either need to change my data structure to make bevy happy, or put it in a mutex to make bevy happy :).

In your case, bevy main has SyncCell which is Exclusive in nightly. Perhaps this may be useful to you.

@james7132
Copy link
Member

james7132 commented Dec 10, 2022

This is partially addressed via #6864, which makes it a user-facing problem instead of one solved by bevy_ecs itself. Though it forces mutable access which may not be conducive to UX, especially regarding change detection.

@DJMcNab
Copy link
Member Author

DJMcNab commented Dec 10, 2022

I agree that this is a capability we can get from composability.

@DJMcNab DJMcNab closed this as not planned Won't fix, can't repro, duplicate, stale Dec 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change C-Feature A new feature, making something new possible C-Performance A change motivated by improving speed, memory usage or compile times S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it!
Projects
None yet
5 participants