Skip to content

Remove remaining internal use of !Send resources #18386

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

Merged
merged 23 commits into from
May 6, 2025

Conversation

joshua-holmes
Copy link
Contributor

@joshua-holmes joshua-holmes commented Mar 18, 2025

Objective

Remaining work for and closes #17682. First half of work for that issue was completed in PR 17730. However, the rest of the work ended up getting blocked because we needed a way of forcing systems to run on the main thread without the use of !Send resources. That was unblocked by PR 18301.

This work should finish unblocking the resources-as-components effort.

Testing

Ran several examples using my Linux machine, just to make sure things are working as expected and no surprises pop up.

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events A-App Bevy apps and plugins X-Controversial There is active debate or serious implications around merging this PR S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Mar 18, 2025
@alice-i-cecile
Copy link
Member

I'm going to want @maniwani's eyes on this, hence the X-Controversial label.

@hymm hymm self-requested a review March 18, 2025 18:17
@joshua-holmes joshua-holmes changed the title Remove remaining internal use of !Send Remove remaining internal use of !Send resources Mar 19, 2025
@joshua-holmes
Copy link
Contributor Author

The work on this PR is complete, but we are waiting for NonSendMarker PR to get merged because this PR depends on that one. Once it's merged, I will rebase and fix up the git history so the commits related to creating NonSendMarker do not appear in this PR.

@joshua-holmes joshua-holmes force-pushed the no-more-non-send branch 3 times, most recently from 8b6d428 to ec82c5f Compare March 20, 2025 01:21
@alice-i-cecile
Copy link
Member

Merging the linked PR for you :)

@alice-i-cecile alice-i-cecile added this to the 0.17 milestone Mar 23, 2025
@joshua-holmes joshua-holmes marked this pull request as ready for review March 24, 2025 21:24
@joshua-holmes joshua-holmes force-pushed the no-more-non-send branch 2 times, most recently from 25caa7c to 158f3f6 Compare May 1, 2025 22:49
@alice-i-cecile alice-i-cecile added X-Blessed Has a large architectural impact or tradeoffs, but the design has been endorsed by decision makers and removed X-Controversial There is active debate or serious implications around merging this PR labels May 5, 2025
@alice-i-cecile alice-i-cecile added this pull request to the merge queue May 5, 2025
@alice-i-cecile
Copy link
Member

Thank you so much for tackling this! I'm really happy that this is finally done, now we just need to remove the old NonSend API surface and revive the resources-as-components work.

@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 5, 2025
@alice-i-cecile alice-i-cecile added this pull request to the merge queue May 6, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 6, 2025
@alice-i-cecile alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels May 6, 2025
@alice-i-cecile
Copy link
Member

Back to Waiting-on-Author until CI issues are resolved :(

@alice-i-cecile alice-i-cecile added this pull request to the merge queue May 6, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 6, 2025
@alice-i-cecile alice-i-cecile added this pull request to the merge queue May 6, 2025
Merged via the queue into bevyengine:main with commit 770f10b May 6, 2025
32 checks passed
andrewzhurov pushed a commit to andrewzhurov/bevy that referenced this pull request May 17, 2025
# Objective

Remaining work for and closes bevyengine#17682. First half of work for that issue
was completed in [PR
17730](bevyengine#17730). However, the rest
of the work ended up getting blocked because we needed a way of forcing
systems to run on the main thread without the use of `!Send` resources.
That was unblocked by [PR
18301](bevyengine#18301).

This work should finish unblocking the resources-as-components effort.

# Testing

Ran several examples using my Linux machine, just to make sure things
are working as expected and no surprises pop up.

---------

Co-authored-by: Chris Russell <8494645+chescock@users.noreply.github.com>
Co-authored-by: François Mockers <francois.mockers@vleue.com>
@IceSentry
Copy link
Contributor

Where's the migration guide for this PR? Also, why is WINIT_WINDOWS private? That's a huge breaking change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-App Bevy apps and plugins A-ECS Entities, components, systems, and events S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged X-Blessed Has a large architectural impact or tradeoffs, but the design has been endorsed by decision makers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐢 Make !Send resources thread_local!
7 participants