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

World::resource_ref should return Ref<T> #11825

Closed
joseph-gio opened this issue Feb 12, 2024 · 3 comments · Fixed by #15263
Closed

World::resource_ref should return Ref<T> #11825

joseph-gio opened this issue Feb 12, 2024 · 3 comments · Fixed by #15263
Labels
A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior C-Usability A targeted quality-of-life change that makes Bevy easier to use X-Controversial There is active debate or serious implications around merging this PR

Comments

@joseph-gio
Copy link
Member

joseph-gio commented Feb 12, 2024

What problem does this solve or what need does it fill?

World::resource_ref currently returns Res<T>, which does not make sense as Res<T> doesn't provide any functionality over Ref<T>. The only reason that Res<T> and ResMut<T> exist is that system params need to use the type to distinguish resources from other data. Otherwise, Res{Mut}<T> do not need to exist, and thus we have always treated Ref<T> and Mut<T> and as the change detection primitives. ResMut::map_unchanged and ResMut::reborrow have always returned Mut<T> to reflect this.

I disagree with returning Res<T> here, as it encourages more use of this type. We should use types like Res<T> and ResMut<T> and little as possible, so we won't need to duplicate all change detection APIs for these types.

What solution would you like?

World::resource_ref should return Ref<T>.

What alternative(s) have you considered?

None

Additional context

#9926
#11776
#11776
#9940

@joseph-gio joseph-gio added C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use and removed S-Needs-Triage This issue needs to be labelled C-Feature A new feature, making something new possible labels Feb 12, 2024
@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior C-Usability A targeted quality-of-life change that makes Bevy easier to use and removed C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Feb 12, 2024
@alice-i-cecile
Copy link
Member

I'm on board with that as a design principle.

However we should have a parallel method on Res to convert it into a Ref.

Can you close out all of the conflicting issues and PRs?

@Adamkob12
Copy link
Contributor

Adamkob12 commented Feb 13, 2024

I have no problem with going in this direction. But to play devil's advocate, I'll say that maybe if the only way to get a change-detection-enabled reference to a resource in a system is through Res, it should also be the only way to do so outside of the system.

We don't have a choice on whether to use Ref or Res inside systems, but we do have a choice on which to use outside of systems. Why not keep them consistent?

Res may not have functional value over Ref, but it has semantic value.

I know when I was learning bevy I was confused on why some things were different inside of systems as opposed to outside of them (when interacting directly with the World for example)

I don't think code duplication is a major concern because we can implement Into and/or Deref etc.

additional context:
#9926 (comment)

@tguichaoua
Copy link
Contributor

tguichaoua commented Feb 14, 2024

The only reason that Res<T> and ResMut<T> exist is that system params need to use the type to distinguish resources from other data. Otherwise, Res{Mut}<T> do not need to exist, and thus we have always treated Ref<T> and Mut<T> and as the change detection primitives.

To reflect this idea in the code, Res and ResMut should be refactorized into thin wrappers around Ref and Mut.

struct Res<'w, T>(Ref<'w, T>);
struct ResMut<'w, T>(Mut<'w, T>);

It will also help reduce code duplication by proxying the API calls.

github-merge-queue bot pushed a commit that referenced this issue Sep 21, 2024
# Objective

Closes #11825

## Solution

Change return type of `get_resource_ref` and `resource_ref` from `Res`
to `Ref` and implement `From Res<T> for Ref<T>`.
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-Bug An unexpected or incorrect behavior C-Usability A targeted quality-of-life change that makes Bevy easier to use X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants