Skip to content

change return type of World::resource_ref to Ref #15263

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

Merged
merged 5 commits into from
Sep 21, 2024

Conversation

atornity
Copy link
Contributor

@atornity atornity commented Sep 17, 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>.

Migration guide

Previously World::get_resource_ref::<T> and World::resource_ref::<T> would return a Res<T> which was inconsistent with the rest of the World API (notably resource_scope). This has been fixed and the methods now return Ref<T>.

This means it is no longer possible to get Res<T> from World. If you were relying on this, you should try using Ref<T> instead since it has the same functionality.

Before

let my_resource: Res<MyResource> = world.resource_ref();
function_taking_resource(my_resource);

fn function_taking_resource(resource: Res<MyResource>) { /* ... */ }

After

let my_resource: Ref<MyResource> = world.resource_ref();
function_taking_resource(my_resource);

fn function_taking_resource(resource: Ref<MyResource>) { /* ... */ }

@atornity atornity changed the title implement conversions between change detection types conversions between change detection types Sep 17, 2024
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should only allow conversions to Ref from Res, not the other way around. Ditto for ResMut and Mut. Otherwise, it's possible to get a Ref or Mut from a Query and then treat it as a resource, which is very confusing.

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Sep 17, 2024
@atornity
Copy link
Contributor Author

atornity commented Sep 17, 2024

We should only allow conversions to Ref from Res, not the other way around. Ditto for ResMut and Mut. Otherwise, it's possible to get a Ref or Mut from a Query and then treat it as a resource, which is very confusing.

You would only be able to convert Ref to Res if the inner value is a resource. Although I would like to replace From<Ref> to a new constructor from_ref.

I'm okay with removing it altogether though, but I do remember needing to get a Res from a world (this was my motivation for adding get_resource_ref) forever ago, and if there is no conversion then it will no longer be possible once get_resource_ref returns a Ref.

Anyways, I don't see a problem with potentially getting Res from a Ref in a query since the component need to also be a resource for that to be possible, and you cant really so anything weird/funky with it anyways.

What do you think? (Sorry for the ramble, I'm on my phone)

@atornity
Copy link
Contributor Author

atornity commented Sep 17, 2024

Alternatively, I could add a into_raw or similar to get the value and ticks from a Ref which can then be used to construct a Res with the new method

Nvm, Res doesn't have a new method

@alice-i-cecile
Copy link
Member

You would only be able to convert Ref to Res if the inner value is a resource

Ah, I see: the trait bound on the From impl will enforce that. Hmm. Okay, I don't hate exposing this then.

@atornity
Copy link
Contributor Author

Ah, I see: the trait bound on the From impl will enforce that. Hmm. Okay, I don't hate exposing this then.

Great, thoughts on From impl vs a new from_ref constructor?

A new constructor would mean you can't do let my_res = my_ref.into(), but you can still do let my_res = Res::from_ref(my_ref)

@alice-i-cecile
Copy link
Member

No strong feelings. IMO let's stick with the From impls for consistency.

@alice-i-cecile alice-i-cecile added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Sep 17, 2024
@maniwani
Copy link
Contributor

maniwani commented Sep 20, 2024

This seems like the opposite of what was requested in the linked issue?

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.

This is true1. Functions that aren't systems should only be taking or providing Ref<T> and Mut<T> as parameters, not Res<T> and ResMut<T>.

I don't think we should encourage using them interchangeably even if Res<T> and ResMut<T> transparently wrap Ref<T> and Mut<T>. I think it should be one-way conversions from Res<T> to Ref<T> and from ResMut<T> to Mut<T>.

Footnotes

  1. Technically, they exist to distinguish between send and non-send resources, but same principle, param types control system fetch/execution.

@atornity
Copy link
Contributor Author

I don't think we should encourage using them interchangeably even if Res<T> and ResMut<T> transparently wrap Ref<T> and Mut<T>. I think it should be one-way conversions from Res<T> to Ref<T> and from ResMut<T> to Mut<T>.

Yeah, I once needed to get a Res from a World and I wanted to make sure that's still possible once I change the resource_ref to return a Ref. But I'm realizing that my use case was way to niche and I abandoned that project forever ago anyways, so I don't need it anymore. Once I have time, I'll fix up the pr to remove some of the conversions as well as change the return type of the resource_ref methods.

@atornity atornity changed the title conversions between change detection types change return type of resource_ref to Ref Sep 20, 2024
@atornity atornity changed the title change return type of resource_ref to Ref change return type of World::resource_ref to Ref Sep 20, 2024
@alice-i-cecile alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Sep 20, 2024
@atornity
Copy link
Contributor Author

@alice-i-cecile are you sure this is controversial now? I removed the conversion from Ref/Mut -> Res/ResMut which is the only part that got pushback afaik.

@alice-i-cecile alice-i-cecile added X-Uncontroversial This work is generally agreed upon and removed X-Controversial There is active debate or serious implications around merging this PR labels Sep 21, 2024
Copy link
Contributor

@pablo-lua pablo-lua left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No reason why not expose this method, can reduce confusion over the types Res and Ref.

@pablo-lua pablo-lua added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Sep 21, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Sep 21, 2024
Merged via the queue into bevyengine:main with commit 66a474a Sep 21, 2024
29 checks passed
@james7132 james7132 added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Sep 21, 2024
Copy link
Contributor

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Could you add some context on what users should update when this change get released in a new version of Bevy?
It will be used to help writing the migration guide for the version. Putting it after a ## Migration Guide will help it get automatically picked up by our tooling.

@ethereumdegen
Copy link
Contributor

ethereumdegen commented Dec 15, 2024

This broke my code and there is nothing in the migration guide... what do i do. now im blocked on my project and nobody can help me

#16831

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-Usability A targeted quality-of-life change that makes Bevy easier to use M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Needs-Review Needs reviewer attention (from anyone!) to move forward S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Uncontroversial This work is generally agreed upon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

World::resource_ref should return Ref<T>
6 participants