Added parse_with_file_loader: overrides direct access to the filesystem while loading external tilesets. #100
Conversation
…em while loading external tilesets.
|
@koalefant This looks very useful! Would you be able to rebase the change to resolve the conflict in Also, @PieKing1215, I see you've already merged this change. Would you be available for providing a code review? I'm looking for additional people to help maintain this crate now that it has moved to the @mapeditor organization. |
| @@ -236,7 +236,7 @@ impl Map { | |||
| fn new<R: Read>( | |||
| parser: &mut EventReader<R>, | |||
| attrs: Vec<OwnedAttribute>, | |||
| map_path: Option<&Path>, | |||
| mut external_file_loader: impl FnMut(&str)->Result<Vec<u8>, TiledError>, | |||
There was a problem hiding this comment.
I'm not sure if this function should return a TiledError as Err, since it's performing an external data fetch, not something actually related to the Tiled object.
There was a problem hiding this comment.
Also, maybe instead of Vec<u8> it should be an object implementing Read, that way implementing async later would be easier.
There was a problem hiding this comment.
Agree on what @alexdevteam said. In my use case, I needed to wrap the actual errors with TiledError::Other and needed to manually read_to_end on a perfectly good File. These two changes would move that bit of bloat from user code into the library.
Sure, I can review this as well. |
| @@ -1082,7 +1076,7 @@ fn parse_animation<R: Read>(parser: &mut EventReader<R>) -> Result<Vec<Frame>, T | |||
| fn parse_infinite_data<R: Read>( | |||
| parser: &mut EventReader<R>, | |||
| attrs: Vec<OwnedAttribute>, | |||
| width: u32, | |||
| _width: u32, | |||
There was a problem hiding this comment.
Nitpick: should this be in this PR? (not sure how you want to handle tiny extra fixes like this)
| _width: u32, | |
| width: u32, |
| @@ -236,7 +236,7 @@ impl Map { | |||
| fn new<R: Read>( | |||
| parser: &mut EventReader<R>, | |||
| attrs: Vec<OwnedAttribute>, | |||
| map_path: Option<&Path>, | |||
| mut external_file_loader: impl FnMut(&str)->Result<Vec<u8>, TiledError>, | |||
There was a problem hiding this comment.
Agree on what @alexdevteam said. In my use case, I needed to wrap the actual errors with TiledError::Other and needed to manually read_to_end on a perfectly good File. These two changes would move that bit of bloat from user code into the library.
| @@ -1290,12 +1287,33 @@ fn parse_impl<R: Read>(reader: R, map_path: Option<&Path>) -> Result<Map, TiledE | |||
| } | |||
| } | |||
|
|
|||
| fn default_file_loader(map_path: Option<PathBuf>)->impl FnMut(&str)->Result<Vec<u8>, TiledError> { | |||
There was a problem hiding this comment.
| fn default_file_loader(map_path: Option<PathBuf>)->impl FnMut(&str)->Result<Vec<u8>, TiledError> { | |
| fn default_file_loader(map_path: Option<PathBuf>) -> impl FnMut(&str)->Result<Vec<u8>, TiledError> { |
Might also want to do the same in all of the FnMut(&str)->Results.
| fn default_file_loader(map_path: Option<PathBuf>)->impl FnMut(&str)->Result<Vec<u8>, TiledError> { | ||
| move |source: &str| { | ||
| let tileset_path = map_path.as_ref() |
There was a problem hiding this comment.
You can do this to avoid needing to_owned in the default_file_loader(Some(path.to_owned()))s below.
| fn default_file_loader(map_path: Option<PathBuf>)->impl FnMut(&str)->Result<Vec<u8>, TiledError> { | |
| move |source: &str| { | |
| let tileset_path = map_path.as_ref() | |
| fn default_file_loader(map_path: Option<&Path>)->impl FnMut(&str)->Result<Vec<u8>, TiledError> + '_ { | |
| move |source: &str| { | |
| let tileset_path = map_path |
| @@ -236,7 +236,7 @@ impl Map { | |||
| fn new<R: Read>( | |||
| parser: &mut EventReader<R>, | |||
| attrs: Vec<OwnedAttribute>, | |||
| map_path: Option<&Path>, | |||
| mut external_file_loader: impl FnMut(&str)->Result<Vec<u8>, TiledError>, | |||
There was a problem hiding this comment.
Would be nice to extract this into a type alias but I think that would be blocked until either rust-lang/rust#41517 or rust-lang/rust#63063 is stable.
|
Closing due to inactivity. Feel free to reopen. |
Hi, I would like to use external tileset in my game, yet my resources are not in the filesystem, they are packed in an archive. So I am adding a function
parse_with_file_loader(reader, file_loader)that allows me to supply a closure that is invoked when external tileset is loaded. Do you think this can be upstreamed in any form?