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

Add App and SubApp resource methods #13425

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

pietrosophya
Copy link
Contributor

Objective

Add helper methods to App and SubApp to ease plugin maintainers and less verbose code. It also adds examples to show how to use these methods.

It also deprecates the World::contains_non_send in favor of the more consistent World::contains_non_send_resource.

@pablo-lua pablo-lua added C-Usability A targeted quality-of-life change that makes Bevy easier to use A-App Bevy apps and plugins labels May 19, 2024
@alice-i-cecile
Copy link
Member

I can see how this makes the code nicer, but it's more to maintain and synchronize and makes the docs much worse. I don't think we should do this.

It also deprecates the World::contains_non_send in favor of the more consistent World::contains_non_send_resource.

I would happily approve a standalone PR for this though!

@alice-i-cecile alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label May 21, 2024
@mockersf
Copy link
Member

It also deprecates the World::contains_non_send in favor of the more consistent World::contains_non_send_resource.

The system param is NonSend, which is why the method is called contains_non_send

@pietrosophya
Copy link
Contributor Author

@alice-i-cecile ok, I'll create a PR only for the non_send_resource (and maybe 👇 ?).

Is there any other reasonable way to achieve the same result?

Or, does it make sense to have some of the world methods but not all of them? I mean only those related to resources only, since they are handled inside plugins.

@mockersf maybe the NonSend should be renamed to NonSendResource too? NonSend seems very generic (and also it's not strictly a noun 😊 )

@mockersf
Copy link
Member

@mockersf maybe the NonSend should be renamed to NonSendResource too?

Names should stay coherent, and if you update one you should update the others.

When adding NonSend, we decided to keep it short. changing the names to have the word resource would make them longer, without adding useful meaning in my opinion. I would prefer to keep them as they are currently.

@pietrosophya
Copy link
Contributor Author

pietrosophya commented May 22, 2024

I see, it makes sense to keep names short.

My 2c as a regular user: I found it confusing that regular resources are initialized as init_resource/insert_resource, derived as Resource and accessed through Res/ResMut, while main-thread-only are initialized as init_non_send_resource/insert_non_send_resource, and accessed through NonSend/NonSendMut.

I wouldn't care about the longer name, both since NonSend resources are way less used, and it wouldn't break consistency (so, I'd rename also NonSend as NonSendRes and NonSendMut as NonSendResMut).

I can always define my own internal type (or a macro) to shorten the name.

Anyway, I'll create the PR, so that we can move the discussion there where it should belong 😊.

@MrGVSV
Copy link
Member

MrGVSV commented May 22, 2024

I'd rename also NonSend as NonSendRes and NonSendMut as NonSendResMut

I think this is a good compromise. I also had no clue that a NonSend was a resource until I read the docs.

@alice-i-cecile
Copy link
Member

NonSend data is... not really a resource, and is definitely not going to remain a resource. See #5135 for more context.

@pietrosophya
Copy link
Contributor Author

NonSend data is... not really a resource, and is definitely not going to remain a resource. See #5135 for more context.

I created this to handle this specific topic.

I read the discussion there, and it seems that rather "not resource" they are treated as "thread-local" ones, right?

@alice-i-cecile alice-i-cecile added the S-Needs-SME Decision or review from an SME is required label May 26, 2024
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 C-Usability A targeted quality-of-life change that makes Bevy easier to use 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.

5 participants