Skip to content

Conversation

joshua-holmes
Copy link
Contributor

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

Objective

Create new NonSendMarker that does not depend on NonSend.

Required, in order to accomplish #17682. In that issue, we are trying to replace !Send resources with thread_local! in order to unblock the resources-as-components effort. However, when we remove all the !Send resources from a system, that allows the system to run on a thread other than the main thread, which is against the design of the system. So this marker gives us the control to require a system to run on the main thread without depending on !Send resources.

Solution

Create a new NonSendMarker to replace the existing one that does not depend on NonSend.

Testing

Other than running tests, I ran a few examples:

  • window_resizing
  • wireframe
  • volumetric_fog (looks so cool)
  • rotation
  • button

There is a Mac/iOS-specific change and I do not have a Mac or iOS device to test it. I am doubtful that it would cause any problems for 2 reasons:

  1. The change is the same as the non-wasm change which I did test
  2. The Pixel Eagle tests run Mac tests

But it wouldn't hurt if someone wanted to spin up an example that utilizes the bevy_render crate, which is where the Mac/iSO change was.

Migration Guide

If NonSendMarker is being used from bevy_app::prelude::*, replace it with bevy_ecs::system::NonSendMarker or use it from bevy_ecs::prelude::*. In addition to that, NonSendMarker does not need to be wrapped like so:

fn my_system(_non_send_marker: Option<NonSend<NonSendMarker>>) {
    ...
}

Instead, it can be used without any wrappers:

fn my_system(_non_send_marker: NonSendMarker) {
    ...
}

@joshua-holmes joshua-holmes changed the title Use new NonSendMarker Create new NonSendMarker Mar 13, 2025
Copy link
Contributor

Your PR caused a change in the graphical output of an example or rendering test. This might be intentional, but it could also mean that something broke!
You can review it at https://pixel-eagle.com/project/B04F67C0-C054-4A6F-92EC-F599FEC2FD1D?filter=PR-18301

If it's expected, please add the M-Deliberate-Rendering-Change label.

If this change seems unrelated to your PR, you can consider updating your PR to target the latest main branch, either by rebasing or merging main into it.

Copy link
Contributor

Your PR caused a change in the graphical output of an example or rendering test. This might be intentional, but it could also mean that something broke!
You can review it at https://pixel-eagle.com/project/B04F67C0-C054-4A6F-92EC-F599FEC2FD1D?filter=PR-18301

If it's expected, please add the M-Deliberate-Rendering-Change label.

If this change seems unrelated to your PR, you can consider updating your PR to target the latest main branch, either by rebasing or merging main into it.

1 similar comment
Copy link
Contributor

Your PR caused a change in the graphical output of an example or rendering test. This might be intentional, but it could also mean that something broke!
You can review it at https://pixel-eagle.com/project/B04F67C0-C054-4A6F-92EC-F599FEC2FD1D?filter=PR-18301

If it's expected, please add the M-Deliberate-Rendering-Change label.

If this change seems unrelated to your PR, you can consider updating your PR to target the latest main branch, either by rebasing or merging main into it.

@joshua-holmes joshua-holmes marked this pull request as ready for review March 15, 2025 18:35
Copy link
Contributor

@chescock chescock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-ECS Entities, components, systems, and events X-Controversial There is active debate or serious implications around merging this PR S-Needs-Review Needs reviewer attention (from anyone!) to move forward D-Straightforward Simple bug fixes and API improvements, docs, test and examples X-Contentious There are nontrivial implications that should be thought through and removed X-Controversial There is active debate or serious implications around merging this PR labels Mar 20, 2025
@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Mar 23, 2025
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Mar 23, 2025
Merged via the queue into bevyengine:main with commit 8d9f948 Mar 23, 2025
39 checks passed
github-merge-queue bot pushed a commit that referenced this pull request May 6, 2025
# Objective

Remaining work for and closes #17682. First half of work for that issue
was completed in [PR
17730](#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](#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 <[email protected]>
Co-authored-by: François Mockers <[email protected]>
github-merge-queue bot pushed a commit that referenced this pull request May 7, 2025
# Objective

In #18301, `NonSendMarker` was defined in such a way that it actually
implements `Send`. This isn't strictly a soundness issue, as its goal is
to be used as a `SystemParam`, and it _does_ appropriately mark system
access as `!Send`. It just seems odd that `NonSendMarker: Send`.

## Solution

- Made `NonSendMarker` wrap `PhantomData<*mut ()>`, which forces it to
be `!Send`.

## Testing

- CI

---

## Notes

This does mean constructing a `NonSendMarker` _value_ will require using
the `SystemParam` trait, but I think that's acceptable as the marker as
a value should be rarely required if at all.
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 <[email protected]>
Co-authored-by: François Mockers <[email protected]>
andrewzhurov pushed a commit to andrewzhurov/bevy that referenced this pull request May 17, 2025
# Objective

In bevyengine#18301, `NonSendMarker` was defined in such a way that it actually
implements `Send`. This isn't strictly a soundness issue, as its goal is
to be used as a `SystemParam`, and it _does_ appropriately mark system
access as `!Send`. It just seems odd that `NonSendMarker: Send`.

## Solution

- Made `NonSendMarker` wrap `PhantomData<*mut ()>`, which forces it to
be `!Send`.

## Testing

- CI

---

## Notes

This does mean constructing a `NonSendMarker` _value_ will require using
the `SystemParam` trait, but I think that's acceptable as the marker as
a value should be rarely required if at all.
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-Feature A new feature, making something new possible D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants