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

More convenient way to load embedded maps #266

Closed
raimundomartins opened this issue May 13, 2023 · 8 comments · Fixed by #272
Closed

More convenient way to load embedded maps #266

raimundomartins opened this issue May 13, 2023 · 8 comments · Fixed by #272

Comments

@raimundomartins
Copy link

Hi! I'm trying out Bevy and (similar Amethyst) it passes a &[u8] to load the file. As of version 0.11, I have no idea how can load maps as implementing ResourceReader for loading from byte arrays would have to be intricate to then be able to load the remaining files from disk.

Is there any chance to add back this super useful method? If not, how can I load the main tmx from bytes and the rest of the files using FilesystemResourceReader?

@bjorn
Copy link
Member

bjorn commented May 13, 2023

In the bevy_ecs_tilemap crate, somebody introduced a BytesResourceReader, see StarArawn/bevy_ecs_tilemap#425. Does that help or would you still suggest we should add back load_tmx_map_from?

@raimundomartins
Copy link
Author

Huh, I guess I should've gone to the main branch before posting this. That along with what's in #265 is a good enough solution to me. I still don't see why the helper method had to go, but at least there's a reasonable alternative. Thanks!

@aleokdev
Copy link
Contributor

I still don't see why the helper method had to go, but at least there's a reasonable alternative. Thanks!

load_..._from was a hacky method to only load completely embedded maps from some source. That is still possible in 0.11, it's just a bit more manual work. Perhaps we could add it back as the BytesResourceReader solution.

@raimundomartins
Copy link
Author

Personally, it wouldn't do much good unless it would support supplying another ResourceReader for the remainder of files. I know I asked how I could use it with the FilesystemResourceReader, but since I can easily use a more portable solution from the beginning, I prefer portability.

@aleokdev
Copy link
Contributor

Personally, it wouldn't do much good unless it would support supplying another ResourceReader for the remainder of files.

Yeah, that's something load_tmx_map_from could not do and it may be useful to have.

I know I asked how I could use it with the FilesystemResourceReader, but since I can easily use a more portable solution from the beginning, I prefer portability.

What do you mean by this? I don't understand.

Now that I think about it, we could implement ResourceReader for anything that implements for<'a> Fn(&'a Path) -> Result<R, E> where R: Read, E: Error + Send + Sync + 'static, which would allow easily defining a ResourceReader inline in the Loader declaration, removing a lot of boilerplate and simplifying the process altogether, something like this:

let mut loader = Loader::with_cache_and_reader(
    DefaultResourceCache,
    |path| { if path == "my-map.tmx" { Ok(that_map) } else { Err(MapLoadingError) }
);

What do you think about this?

@raimundomartins
Copy link
Author

raimundomartins commented May 15, 2023

What do you mean by this? I don't understand.

I was just explaining why I wouldn't use it. It's not important.

What's important is your last suggestion which is actually pretty cool! That I would definitely use!

@aleokdev
Copy link
Contributor

Thanks :) I'll try to PR it whenever possible. I'm currently really busy with exams so it probably won't be done until ~2weeks from now.

@aleokdev aleokdev changed the title Add back load_tmx_map_from() More convenient way to load embedded maps May 19, 2023
@aleokdev
Copy link
Contributor

aleokdev commented Oct 4, 2023

This has been merged for a while now, so we can safely close this issue.

@aleokdev aleokdev closed this as completed Oct 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants