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

World::resource and allies do not return Res / ResMut smart pointers #9926

Closed
alice-i-cecile opened this issue Sep 26, 2023 · 6 comments
Closed

World::resource and allies do not return Res / ResMut smart pointers #9926

alice-i-cecile opened this issue Sep 26, 2023 · 6 comments
Labels
A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior

Comments

@alice-i-cecile
Copy link
Member

Bevy version

0.11

What you did

Called World::resource

What went wrong

I expected a Res or ResMut smart pointer, so I could call change detection methods and reuse code more easily.

Instead I got a & or a Mut.
@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior D-Trivial Nice and easy! A great choice to get started with Bevy A-ECS Entities, components, systems, and events labels Sep 26, 2023
@A-Walrus
Copy link
Contributor

A-Walrus commented Sep 26, 2023 •
edited
Loading

Should UnsafeWorldCell change as well, or just World?

@alice-i-cecile
Copy link
Member Author

Yes, I think it should.

@mockersf
Copy link
Member

Maybe not a good first issue, as getting a Res or a ResMut requires system metadata (for last run tick), and you don't have those from just a World.

Resource changed/added ticks can be retrieved from get_resource_with_ticks

@alice-i-cecile alice-i-cecile removed the D-Trivial Nice and easy! A great choice to get started with Bevy label Sep 26, 2023
@bushrat011899 bushrat011899 mentioned this issue Sep 27, 2023
Closed
@cart
Copy link
Member

cart commented Sep 27, 2023

Fully on board for making APIs consistent. But as an alternative, maybe we should add get_res and get_res_mut variants? Currently not in love with the fact that we're doing unnecessary change detection state retrievals for Res everywhere. And doubling down on that by making resource-only-access impossible in our World APIs feels wrong to me. Looking at Bevy internals, the number of times we access Res without something like is_changed vs the number of times we do access change state is minuscule. That is work that we eat per-resource per-system per-frame.

@alice-i-cecile
Copy link
Member Author

An additional argument for consistency here: you can get the change ticks for a resource from the World, but only when extracting it mutably.

@atornity atornity mentioned this issue Jan 27, 2024
Merged
github-merge-queue bot pushed a commit that referenced this issue Jan 28, 2024
@atornity @alice-i-cecile
@hymm @MinerSebas
added ability to get Res<T> from World with `World::get_resource_…
2391e44 
…ref` (#11561)

# Objective

It's sometimes desirable to get a `Res<T>` rather than `&T` from
`World::get_resource`.
Alternative to #9940, partly adresses #9926

## Solution

added additional methods to `World` and `UnsafeWorldCell` to retrieve a
resource wrapped in a `Res`.
- `UnsafeWorldCell::get_resource_ref`
- `World::get_resource_ref`
- `World::resource_ref`

I can change it so `World::resource_mut` returns `ResMut` instead of
`Mut` as well if that's desired, but that could also be added later in a
seperate pr.

---------

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Co-authored-by: Mike <mike.hsu@gmail.com>
Co-authored-by: MinerSebas <66798382+MinerSebas@users.noreply.github.com>
tjamaan pushed a commit to tjamaan/bevy that referenced this issue Feb 6, 2024
@atornity @alice-i-cecile
@hymm @MinerSebas @tjamaan
added ability to get Res<T> from World with `World::get_resource_…
d7e3b1a 
…ref` (bevyengine#11561)

# Objective

It's sometimes desirable to get a `Res<T>` rather than `&T` from
`World::get_resource`.
Alternative to bevyengine#9940, partly adresses bevyengine#9926

## Solution

added additional methods to `World` and `UnsafeWorldCell` to retrieve a
resource wrapped in a `Res`.
- `UnsafeWorldCell::get_resource_ref`
- `World::get_resource_ref`
- `World::resource_ref`

I can change it so `World::resource_mut` returns `ResMut` instead of
`Mut` as well if that's desired, but that could also be added later in a
seperate pr.

---------

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Co-authored-by: Mike <mike.hsu@gmail.com>
Co-authored-by: MinerSebas <66798382+MinerSebas@users.noreply.github.com>
@JoJoJet JoJoJet mentioned this issue Feb 12, 2024
Closed
@JoJoJet
Copy link
Member

JoJoJet commented Feb 12, 2024

Closing in favor of #11825

@JoJoJet JoJoJet closed this as not planned Won't fix, can't repro, duplicate, stale Feb 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Updated World, UnsafeWorldCell Resource methods to use Res, ResMut bushrat011899/bevy
5 participants
@cart @alice-i-cecile @mockersf @JoJoJet @A-Walrus