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

Add PrototypesMut::load_folder #36

Merged
merged 4 commits into from
May 1, 2023
Merged

Add PrototypesMut::load_folder #36

merged 4 commits into from
May 1, 2023

Conversation

wusticality
Copy link
Contributor

@wusticality wusticality commented Apr 30, 2023

Add PrototypesMut::load_folder so that it mirrors AssetServer::load_folder, with the exception that the returned handles are typed.

let handles: Vec<_> = handles.iter().map(|handle| handle.clone().typed::<T>()).collect();

handles.iter().for_each(|handle| {
let path = self.asset_server.get_handle_path(handle).unwrap().path().to_str().unwrap().to_string();
Copy link
Contributor Author

@wusticality wusticality Apr 30, 2023

Choose a reason for hiding this comment

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

This is a bit gross, we should probably handle the error cases instead of calling unwrap.

Perhaps I should add PrototypesError?

Copy link
Owner

Choose a reason for hiding this comment

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

Theoretically this should always return a path, right? If so, I think we can go ahead and just unwrap it (maybe with an .expect() instead).

Same for the call to to_str (which we could replace with to_str_lossy but that could break the mapping).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added some errors for this, let me know what you think

bevy_proto_backend/src/proto/prototypes.rs Outdated Show resolved Hide resolved
bevy_proto_backend/src/proto/prototypes.rs Outdated Show resolved Hide resolved
bevy_proto_backend/src/proto/prototypes.rs Outdated Show resolved Hide resolved
let handles: Vec<_> = handles.iter().map(|handle| handle.clone().typed::<T>()).collect();

handles.iter().for_each(|handle| {
let path = self.asset_server.get_handle_path(handle).unwrap().path().to_str().unwrap().to_string();
Copy link
Owner

Choose a reason for hiding this comment

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

Theoretically this should always return a path, right? If so, I think we can go ahead and just unwrap it (maybe with an .expect() instead).

Same for the call to to_str (which we could replace with to_str_lossy but that could break the mapping).

Comment on lines 30 to 32
/// The path cannot be converted to a string.
#[error("cannot convert path to a string")]
ConversionError
Copy link
Owner

Choose a reason for hiding this comment

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

Nit: I wonder if this should give more info regarding why it failed. Like maybe it should be called NotUtf8 or maybe the error message should indicate that the path was not valid UTF-8.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think NotUtf8 works

Comment on lines 76 to 79
.path()
.to_str()
.ok_or(PathError::ConversionError)?
.to_string();
Copy link
Owner

Choose a reason for hiding this comment

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

Actually, what if we took in P: Into<PathBuf> and then stored the PathBuf in ProtoStorage instead of Cow<'static, str>?

This way we don't need to worry about conversions (we can remove the ConversionError) and it's a little more clear we're looking for paths to files and not just their ID or something.

bevy_proto_backend/src/proto/prototypes.rs Outdated Show resolved Hide resolved
Copy link
Owner

@MrGVSV MrGVSV left a comment

Choose a reason for hiding this comment

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

Thanks for adding this! I think it really helps bridge the gap between these system parameters and the AssetServer for new users when we have similar methods between the two.

@MrGVSV MrGVSV merged commit 97a4695 into MrGVSV:main May 1, 2023
@wusticality wusticality deleted the load-folder-support branch May 1, 2023 07:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants