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

[Merged by Bors] - add Res::clone #4109

Closed
wants to merge 1 commit into from
Closed

[Merged by Bors] - add Res::clone #4109

wants to merge 1 commit into from

Conversation

BoxyUwU
Copy link
Member

@BoxyUwU BoxyUwU commented Mar 5, 2022 •
edited
Loading

Objective

Make Res cloneable

Solution

Add an associated fn clone(self: &Self) -. Self instead of Copy + Clone trait impls to avoid res.clone() failing to clone out the underlying T

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Mar 5, 2022
@BoxyUwU BoxyUwU added the A-ECS Entities, components, systems, and events label Mar 5, 2022
@MrGVSV
Copy link
Member

MrGVSV commented Mar 5, 2022 •
edited
Loading

Would this prevent the contained data from being cloned by deref?

let my_cloned_data = res.clone();

Like, would we need to do the alternate form:

let my_cloned_data = (*res).clone();

@BoxyUwU
Copy link
Member Author

BoxyUwU commented Mar 5, 2022

the alternative form i think is actually (&*res).clone() but yes

@james7132
Copy link
Member

james7132 commented Mar 5, 2022 •
edited
Loading

Not a fan of this change. The expected behavior of a resource that is clonable should give you a cloned version of the inner value. If we explicitly want to create a new Res<T> from another one, I would much rather be explicit about it and use Res::copy(res) or something similar (akin to Arc::downgrade and other similar smart pointer APIs that avoid using self to ensure that it's not in conflict with an deref'ed function).

Does calling res.clone() on a Res<T: !Clone> give you &T due to Deref?

@alice-i-cecile alice-i-cecile added 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 labels Mar 5, 2022
@mcobzarenco
Copy link
Contributor

Not a fan of this change.

Same here

The expected behavior of a resource that is clonable should give you a cloned version of the inner value. If we explicitly want to create a new Res from another one, I would much rather be explicit about it and use Res::copy(res) or something similar (akin to Arc::downgrade and other similar smart pointer APIs that avoid using self to ensure that it's not in conflict with an deref'ed function).

Indeed, this is what std does, the Clone impls for smart pointers (Rc, Arc etc.) clones the underlying resource via deref and you're expected to use e.g. Arc::clone to clone the pointer.

@james7132
Copy link
Member

james7132 commented Mar 5, 2022 •
edited
Loading

Indeed, this is what std does, the Clone impls for smart pointers (Rc, Arc etc.) clones the underlying resource via deref and you're expected to use e.g. Arc::clone to clone the pointer.

Actually both Rc and Arc clone the pointer, not the value. Only Box clones both the pointer and value. Still, I expect the base Res<T> pointer to act like &T, and ultimately that's what enables the ergonomics around it.

@BoxyUwU
Copy link
Member Author

BoxyUwU commented Mar 6, 2022

&&T also has the issue of .clone() not cloning the inner T so this is not actually limited to Res<T>: Copy + Clone but regardless I removed the trait impls and just added Res::clone since we ought to provide some way of doing this even if the obvious way of Copy is disliked

@BoxyUwU BoxyUwU changed the title make Res<T>: Copy + Clone add Res::clone Mar 6, 2022
@alice-i-cecile alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Apr 22, 2022
alice-i-cecile
alice-i-cecile reviewed May 30, 2022
View reviewed changes
crates/bevy_ecs/src/system/system_param.rs
@@ -220,6 +220,17 @@ where
}

impl<'w, T: Resource> Res<'w, T> {
// no it shouldn't clippy
#[allow(clippy::should_implement_trait)]
pub fn clone(this: &Self) -> Self {
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be #[inline] as a tiny method.

@alice-i-cecile alice-i-cecile 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 May 30, 2022
@alice-i-cecile
Copy link
Member

After reading the discussion, I think I prefer the current design over implementing the trait.

cart
cart reviewed Aug 5, 2022
View reviewed changes
crates/bevy_ecs/src/system/system_param.rs
@@ -252,6 +252,17 @@ where
}

impl<'w, T: Resource> Res<'w, T> {
Copy link
Member

Choose a reason for hiding this comment

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

Just throwing an idea out there: maybe we should call this something else to avoid the ambiguity entirely?
(ex: duplicate())

Copy link
Member

Choose a reason for hiding this comment

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

Yep, that's my preference here too.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm actually within the context of requiring Res::clone to resolve the ambiguity, I think thats nice and clear, given that we're cloning the Res. I think we should merge this as-is.

@cart
Copy link
Member

cart commented Sep 27, 2022

bors r+

bors bot pushed a commit that referenced this pull request Sep 27, 2022
@BoxyUwU
add Res::clone (#4109)
92c90a9 
# Objective
Make `Res` cloneable
## Solution
Add an associated fn `clone(self: &Self) -. Self` instead of `Copy + Clone` trait impls to avoid `res.clone()` failing to clone out the underlying `T`
@bors
Copy link
Contributor

bors bot commented Sep 27, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title add Res::clone [Merged by Bors] - add Res::clone Sep 27, 2022
@bors bors bot closed this Sep 27, 2022
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 19, 2022
@BoxyUwU @james7132
add Res::clone (bevyengine#4109)
6dd5ced 
# Objective
Make `Res` cloneable
## Solution
Add an associated fn `clone(self: &Self) -. Self` instead of `Copy + Clone` trait impls to avoid `res.clone()` failing to clone out the underlying `T`
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
@BoxyUwU @james7132
add Res::clone (bevyengine#4109)
7a07a8f 
# Objective
Make `Res` cloneable
## Solution
Add an associated fn `clone(self: &Self) -. Self` instead of `Copy + Clone` trait impls to avoid `res.clone()` failing to clone out the underlying `T`
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
@BoxyUwU @ItsDoot
add Res::clone (bevyengine#4109)
ad7ffeb 
# Objective
Make `Res` cloneable
## Solution
Add an associated fn `clone(self: &Self) -. Self` instead of `Copy + Clone` trait impls to avoid `res.clone()` failing to clone out the underlying `T`
@mockersf mockersf mentioned this pull request Jun 4, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cart cart cart left review comments

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@bjorn3 bjorn3 bjorn3 left review comments

@james7132 james7132 james7132 approved these changes

Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it 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.
7 participants
@BoxyUwU @MrGVSV @james7132 @mcobzarenco @alice-i-cecile @cart @bjorn3