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

fix: rename non send resources #13481

Closed

Conversation

pietrosophya
Copy link
Contributor

Objective

Rename non-send resources more consistently inside the crate.
Rewrite some docs to use "[NonSendRes] resource" instead of [!Send]
Rewrite some docs to use "[NonSendRes] resource" instead of "non-send resource"

Migration Guide

  • Rename NonSend to NonSendRes
  • Rename NonSendMut to NonSendResMut
  • Use World::contains_non_send_resource in place of World::contains_non_send

@mockersf
Copy link
Member

I would prefer to remove all mention of resource from NonSend methods and docs.

The fact that NonSend is kinda using resources in the implementation is an implementation detail, will probably soon change, and is not helpful to the user.

@pietrosophya
Copy link
Contributor Author

I still think they could be considered resources (like a filesystem, a network, a printer, or even corn or pasta), because they are "things" needed to make the world run.

If not Resource, what could be another good noun? NonSend is not a noun, and although clear to expert rust users, it's confusing (I'm pretty sure I can speak for many Bevy users here).

@alice-i-cecile
Copy link
Member

I think we need a better noun for them: they aren't resources in the sense of Resource, and are likely to drift even further away. Data maybe?

@MrGVSV
Copy link
Member

MrGVSV commented May 23, 2024

I think we need a better noun for them: they aren't resources in the sense of Resource, and are likely to drift even further away.

I think the problem is they're just too similar to what resources are even if given a different label. A "resource" is really just a convenient way to store a "single global instance of some data" (to quote the Cheatbook— not because it speaks for Bevy's views but because it sorta shows how the community sees these types). NonSend does this as well but just changes how a system is run.

They aren't a "resource" in that T has to derive Resource, but to me that's the implementation detail. NonSend could just as easily require a NonSendResource derive or something.

Granted, I’m not familiar with the discussion around the ultimate goals for handling non-send data so I could be waaayyy off haha. But imo if it still has the purpose of being a "thread-local/non-send global singleton" then it's still kind of a "resource" (especially if you access it via system parameter like you would a regular Resource).

So I personally prefer the rename presented in this PR.

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events X-Controversial There is active debate or serious implications around merging this PR C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples labels May 23, 2024
@BD103 BD103 added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label May 24, 2024
@alice-i-cecile alice-i-cecile added the S-Needs-SME Decision or review from an SME is required label May 29, 2024
@pietrosophya
Copy link
Contributor Author

@alice-i-cecile what should be the next steps for this PR (to approve or discard it)?

@joseph-gio joseph-gio self-requested a review July 12, 2024 22:52
@BenjaminBrienen BenjaminBrienen added the S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged label Jan 23, 2025
@BenjaminBrienen
Copy link
Contributor

@BenjaminBrienen BenjaminBrienen added S-Blocked This cannot move forward until something else changes and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Jan 23, 2025
@alice-i-cecile
Copy link
Member

Closing this out: I agree that this isn't a good name, but NonSend data needs a bigger refactor and it's not worth the churn for now.

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-Usability A targeted quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Blocked This cannot move forward until something else changes S-Needs-SME Decision or review from an SME is required X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants