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

Require components viewed in Schedule to implement Sync. #207

Merged
merged 1 commit into from
Apr 14, 2023
Merged

Conversation

Anders429
Copy link
Owner

Just requires Task::View to implement Send (which is equivalent to the components themselves implementing Sync, as T implements Send if and only if &T implements Sync). See the added trybuild test for an example that previously (incorrectly) compiled but now does not.

Not sure how many users would actually be affected by this. Most components stored in ECS patterns are fairly straight-forward, but I could see some user trying to store an Rc for some shared state between entities or something.

In the future, there could be a special Task type that specifies which components are non-Sync and can therefore not be borrowed simultaneously between two threads. This would allow all components to be used in schedules again. Not sure what the best API for such a feature is.

@Anders429 Anders429 added C - Bug Category: Something is not functioning as expected. C - Unsound Category: Undefined behavior. P - High Priority: Urgent, needing immediate attention. F - rayon Feature: Parallel processing through the rayon library. A - Scheduling Area: Parallel scheduling of systems. labels Apr 14, 2023
@Anders429
Copy link
Owner Author

Code coverage loss is, again, expected, because this PR touched src/system/schedule/mod.rs's tests. I ought to figure out how to fix that.

@codecov
Copy link

codecov bot commented Apr 14, 2023

Codecov Report

Merging #207 (ddcaebc) into dev (65e1d95) will decrease coverage by 0.05%.
The diff coverage is 75.75%.

@@            Coverage Diff             @@
##              dev     #207      +/-   ##
==========================================
- Coverage   95.45%   95.40%   -0.05%     
==========================================
  Files          89       89              
  Lines       14082    14115      +33     
==========================================
+ Hits        13442    13467      +25     
- Misses        640      648       +8     
Impacted Files Coverage Δ
src/system/schedule/task/sealed.rs 100.00% <ø> (ø)
src/system/schedule/mod.rs 74.00% <75.75%> (+0.07%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@Anders429 Anders429 merged commit cb3a526 into dev Apr 14, 2023
@Anders429 Anders429 deleted the sync branch April 14, 2023 23:59
@Anders429 Anders429 added this to the 0.9.0 milestone Apr 15, 2023
@Anders429 Anders429 added V - Major Breaking Change Versioning: Requires a major bump according to semver. and removed V - Major Breaking Change Versioning: Requires a major bump according to semver. labels Apr 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A - Scheduling Area: Parallel scheduling of systems. C - Bug Category: Something is not functioning as expected. C - Unsound Category: Undefined behavior. F - rayon Feature: Parallel processing through the rayon library. P - High Priority: Urgent, needing immediate attention.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant