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 From<String> for AssetPath<'a> #6337

Closed
wants to merge 3 commits into from
Closed

[Merged by Bors] - Add From<String> for AssetPath<'a> #6337

wants to merge 3 commits into from

Conversation

ong-yy
Copy link
Contributor

@ong-yy ong-yy commented Oct 22, 2022

Objective

Fixes #6291

Solution

Implement From<String> for AssetPath<'a>

harudagondi
harudagondi suggested changes Oct 22, 2022
View reviewed changes
crates/bevy_asset/src/path.rs Outdated Show resolved Hide resolved
harudagondi
harudagondi approved these changes Oct 22, 2022
View reviewed changes
Copy link
Member

@harudagondi harudagondi left a comment

Choose a reason for hiding this comment

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

It's sad that the solution is just copying pasting from &str with the only difference being that this returns owned values. I'm not blocking on this, but perhaps implementing ToOwned would be viable here?

Approving this =)

@djeedai djeedai added A-Assets Load files from disk to use for things like images, models, and sounds C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Oct 22, 2022
@mockersf mockersf 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 Oct 22, 2022
@ong-yy
Copy link
Contributor Author

ong-yy commented Oct 23, 2022

It's sad that the solution is just copying pasting from &str with the only difference being that this returns owned values. I'm not blocking on this, but perhaps implementing ToOwned would be viable here?

Approving this =)

Yea. I agree with you. But since we are passing in an owned String, any variable created would not live long enough to return.
https://stackoverflow.com/questions/32682876/is-there-any-way-to-return-a-reference-to-a-variable-created-in-a-function

Do you have some reference as to how ToOwned will help and its implementation?

@harudagondi
Copy link
Member

Do you have some reference as to how ToOwned will help and its implementation?

Essentially both &str and String implementations are basically the same except one returns Cow::Borrowed fields and the other returns Cow::Owned. Looking at the docs once more, AssetPath actually implements Clone.

Maybe asset_path.as_str().into().clone() would suffice?

@ong-yy
Copy link
Contributor Author

ong-yy commented Oct 23, 2022

I tried implementing using asset_path.as_str().into().clone() as below
image

And got the following error which I think is the same as asset_path.as_str().into()

cannot return value referencing function parameter asset_path
returns a value referencing data owned by the current function rustcE0515

@harudagondi
Copy link
Member

Hmm. What if we go through the PathBuf route? According to the impl, converting from String to PathBuf shouldn't allocate memory, which is nice.

@ong-yy
Copy link
Contributor Author

ong-yy commented Oct 23, 2022

That is possible but we will lose the label of AssetPath as it's assigned a None to it.

@alice-i-cecile
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Oct 24, 2022
@ong-yy
Add From<String> for AssetPath<'a> (#6337)
c226fee 
# Objective
Fixes #6291 

## Solution
Implement `From<String>` for `AssetPath<'a>`
@bors
Copy link
Contributor

bors bot commented Oct 24, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Add From<String> for AssetPath<'a> [Merged by Bors] - Add From<String> for AssetPath<'a> Oct 24, 2022
@bors bors bot closed this Oct 24, 2022
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
@ong-yy @james7132
Add From<String> for AssetPath<'a> (bevyengine#6337)
ab28ee5 
# Objective
Fixes bevyengine#6291 

## Solution
Implement `From<String>` for `AssetPath<'a>`
Pietrek14 pushed a commit to Pietrek14/bevy that referenced this pull request Dec 17, 2022
@ong-yy @Pietrek14
Add From<String> for AssetPath<'a> (bevyengine#6337)
5b431ec 
# Objective
Fixes bevyengine#6291 

## Solution
Implement `From<String>` for `AssetPath<'a>`
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
@ong-yy @ItsDoot
Add From<String> for AssetPath<'a> (bevyengine#6337)
500bc44 
# Objective
Fixes bevyengine#6291 

## Solution
Implement `From<String>` for `AssetPath<'a>`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@harudagondi harudagondi harudagondi approved these changes

@mockersf mockersf mockersf approved these changes

Assignees
No one assigned
Labels
A-Assets Load files from disk to use for things like images, models, and sounds 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
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement From<String> for AssetPath<'a>
5 participants
@ong-yy @harudagondi @alice-i-cecile @mockersf @djeedai