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] - Allow String and &String as Id for AssetServer.get_handle(id) #3280

Closed
wants to merge 1 commit into from
Closed

[Merged by Bors] - Allow String and &String as Id for AssetServer.get_handle(id) #3280

wants to merge 1 commit into from

Conversation

Weasy666
Copy link
Contributor

@Weasy666 Weasy666 commented Dec 8, 2021

Objective

Make it easier to build and use an asset path with format!(). This can be useful for accessing assets in a loop.

Enabled by this PR:

let monkey_handle = asset_server.get_handle(&format!("models/monkey/Monkey.gltf#Mesh0/Primitive0"));
let monkey_handle = asset_server.get_handle(format!("models/monkey/Monkey.gltf#Mesh0/Primitive0"));

Before this PR:

let monkey_handle = asset_server.get_handle(format!("models/monkey/Monkey.gltf#Mesh0/Primitive0").as_str());

It's just a tiny improvement in ergonomics, but i ran into it and was wondering why the function does not accept a String and Bevy is all about simplicity/ergonomics, right? 😄😉

Solution

Implement Into<HandleId> for String and &String.

@Weasy666
Allow String and &String as Id for
81b3b01 
`AssetServer.get_handle(id)`
@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Dec 8, 2021
@alice-i-cecile alice-i-cecile 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 and removed S-Needs-Triage This issue needs to be labelled labels Dec 8, 2021
alice-i-cecile
alice-i-cecile approved these changes Dec 8, 2021
View reviewed changes
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.

Looks great, thanks!

@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 Dec 8, 2021
@aloucks
Copy link
Contributor

aloucks commented Dec 8, 2021

Before this PR:

let monkey_handle = asset_server.get_handle(format!("models/monkey/Monkey.gltf#Mesh0/Primitive0").as_str());

Can't you already do this?

let handle = asset_server.get_handle(&*format!("models/{}", name);

@Weasy666
Copy link
Contributor Author

Weasy666 commented Dec 9, 2021

Looks like you can do that, but why not make it easier? Especially for beginners.

@DJMcNab
Copy link
Member

DJMcNab commented Dec 9, 2021

We could possibly do it for T: AsRef<str> instead?

@Weasy666
Copy link
Contributor Author

Weasy666 commented Dec 9, 2021

That's an even better idea.

@Weasy666
Copy link
Contributor Author

Weasy666 commented Dec 9, 2021

I can condense this
https://github.com/bevyengine/bevy/pull/3280/files#diff-cffa1459c07909ff31710f4d23192ae34a30938951b42076f162702f04c3f601R175-R192)
into a AsRef<str>.
But this does not work for AssetPath, because it is conflicting with the impls on From<&'a Path> and From<PathBuf>.
The impl of AsRef<str> for HandleId would be enough to cover the case in my initial comment, even without adding From<&'a String> for AssetPath.

@cart
Copy link
Member

cart commented Dec 9, 2021

I think avoiding blanket impls here is a good idea because of their potential to conflict with future impls we need. I'm happy with the solution implemented in this pr.

@cart
Copy link
Member

cart commented Dec 9, 2021

bors r+

bors bot pushed a commit that referenced this pull request Dec 9, 2021
@Weasy666
Allow String and &String as Id for AssetServer.get_handle(id) (…
a2f0fe2 
…#3280)

# Objective

Make it easier to build and use an asset path with `format!()`. This can be useful for accessing assets in a loop.

Enabled by this PR:
```rust
let monkey_handle = asset_server.get_handle(&format!("models/monkey/Monkey.gltf#Mesh0/Primitive0"));
let monkey_handle = asset_server.get_handle(format!("models/monkey/Monkey.gltf#Mesh0/Primitive0"));
```

Before this PR:
```rust
let monkey_handle = asset_server.get_handle(format!("models/monkey/Monkey.gltf#Mesh0/Primitive0").as_str());
```

It's just a tiny improvement in ergonomics, but i ran into it and was wondering why the function does not accept a `String` and Bevy is all about simplicity/ergonomics, right? 😄😉

## Solution

Implement `Into<HandleId>` for `String` and `&String`.
@bors
Copy link
Contributor

bors bot commented Dec 9, 2021

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Allow String and &String as Id for AssetServer.get_handle(id) [Merged by Bors] - Allow String and &String as Id for AssetServer.get_handle(id) Dec 9, 2021
@bors bors bot closed this Dec 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile 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.
5 participants
@Weasy666 @aloucks @DJMcNab @cart @alice-i-cecile