bevyengine / bevy

A refreshingly simple data-driven game engine built in Rust
https://bevyengine.org
Apache License 2.0
32.97k stars 3.2k forks source link

Add App and SubApp resource methods #13425

Open pietrosophya opened 2 weeks ago

pietrosophya commented 2 weeks ago

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.

alice-i-cecile commented 1 week ago

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!

mockersf commented 1 week ago

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 commented 1 week ago

@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 commented 1 week ago

@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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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

pietrosophya commented 1 week ago

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?