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.

Sign up for GitHub

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

Jump to bottom

Inserting Resource Tuples #16458

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

Inserting Resource Tuples #16458

wants to merge 9 commits into from
+121 −27

Conversation

Wuketuke
Copy link
Contributor

Objective

Some structs like Systems or Plugins can be inserted via tuple, but not Resources, which greatly annoyed me.
Fixed #16398

Solution

I very closely copied what plugins do in bevy_app. I also modified every function signature relating to inserting Resources.
However, I still have some open questions about my implementation:

  • I dont exaclty know what #[track_caller] does. I tried faithfully keeping that functionality, but its possible that I broke something.
  • Somehow, World::insert_resource_with_caller is now unused, and I dont what went wrong
  • Some other functions like App::init_resource, which dont currently support tuples. Should I also rework these functions?
  • should i rename insert_resource to insert_resource to reflect this new feature?
  • I removed the generic function signature in a file on line 118 and 131, which hopefully didnt break anything

Testing

I added an example in the documentation, and two new unit tests

@IQuick143 IQuick143 added C-Feature A new feature, making something new possible 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 D-Macros Code that generates Rust code labels Nov 23, 2024
@alice-i-cecile alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Nov 23, 2024
@alice-i-cecile
Copy link
Member

We've chatted about this before, and the SME-ECS (and Cart) aren't fully sold that this is worth the added compile time. To move forward here, you'll need to measure the impact.

@alice-i-cecile alice-i-cecile added the S-Needs-Benchmarking This set of changes needs performance benchmarking to double-check that they help label Nov 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible D-Macros Code that generates Rust code S-Needs-Benchmarking This set of changes needs performance benchmarking to double-check that they help S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow resources to be inserted via tuple
3 participants
@Wuketuke @alice-i-cecile @IQuick143