Closed B-Reif closed 3 years ago
Looking good (well, except for the build failure ;) ). Would be important to have a few test files, though. I haven't yet figured out how to install Aseprite 1.3 without disabling my existing 1.2 version.
Still a bit more to do to support External File and Tileset chunks. I'll be sure to include some test files.
Tilesets can contain the actual image data (in the spec as PIXEL[]
). This means some actual pixel-parsing logic needs to be refactored out of the Cel module and into some shared Pixel module.
Added a basic Tileset test, which currently fails because it doesn't parse pixel data correctly.
I don't exactly understand how to parse a Tilemap cel. It has a list of 'tiles', and a 'tile' is a value. I'm not really sure how to interpret the Tile value to get pixel data. do you have any insight on this? @alpine-alpaca
I don't exactly understand how to parse a Tilemap cel. It has a list of 'tiles', and a 'tile' is a value. I'm not really sure how to interpret the Tile value to get pixel data. do you have any insight on this? @alpine-alpaca
My guess is that the tile size divides the cel into regions of tile width x tile height. And then the image data to be used is indicated by the tile id (ie. TILE
). And the actual data is stored in the Tileset chunk, which stores its data as an image of width tile width and height (tiles x tile height). So the cel data is just an array of ids. and it should be be of size (ceil(image width / tile width) * ceil(image height / tile height)
.
Okay, I've marked this as Ready and updated the OP with a quick tour of the PR. Tests are passing, including the new Tilemap tests.
Ok, I've changed all of the panics in functions that already return Result
. We can either refactor/extend the validation logic upstream, or we can change the file
module functions to return a Result<()>
.
Pushed a commit to return Result<()>
from .image
methods. We can revert this for something else if we want to pursue a different strategy.
EDIT: I see now that the idea is that validation happens during initial parsing so that the Image part can be guaranteed. I'll investigate moving these checks to validate then.
Yes, in earlier versions I had image
return a result, but then changed it because I felt it's difficult to deal with errors at that stage and it's much nicer to check the assumptions/invariants during parsing where errors must be expected anyway.
We can either refactor/extend the validation logic upstream, or we can change the
file
module functions to return aResult<()>
.
I'm not sure what you mean about upstream
. Do you mean Aseprite? That wouldn't be enough. This library shouldn't make any assumptions without explicitly validating them. The file could be corrupted in arbitrary ways (intentionally or not) and this library should not crash (just loading the file should return an error). Before a version 1.0 I would like to extend the CI to do some fuzz testing to ensure that it is the case.
Sorry I was talking about refactoring in either validate()
or in image()
, that's all.
A few new changes:
Result<ValidatedParseInfo>
.image()
now specifies which validation functions it's expecting to have been calledoops didn't mean to press Close
Ok, getting there. Some things I noticed:
TilesetsById
needs to be re-exported from lib.rs
and documented. It's part of the public interface. Similarly, TilesetId
probably needs to be exported. Not clear to me ATM how users are meant to construct them. There may be more. Just run cargo doc
and click through the new APIs and see if they make sense.Similarly,
TilesetId
probably needs to be exported. Not clear to me ATM how users are meant to construct them.
My thinking was that the underlying u32
would be abstracted away and users would only use these ids for lookup/comparison. I added a public constructor in case there's a need for it.
We might be able to further reduce the number of expect
calls with some changes to the types, but they would require substantial refactoring and probably need a separate PR. The validate functions should work for now. Let me know if there's anything else you'd like changed.
Yes, I think we can make things more robust by encapsulating the preconditions/invariants into types some more. A.k.a. "Parse, don't validate" or "Make illegal states unrepresentable".
v1.3 File Spec Changes
This PR adds new features to support the upcoming changes to the Aseprite file spec.. This involves both additional features and refactors of existing code. I'll document changes in the order provided by the spec, and then append changes specific to the crate.
What's new
Tile data type
From the spec:
The new
Tile
struct intile.rs
represents this type. Currently, Aseprite itself only supports 32-bitTILE
values, so this PR only targets 32-bit values as well.Tilemap layer type
The new
Tilemap
LayerType enum case stores a Tileset index (DWORD
) value.Tilemap cel
From the spec:
The new
Tilemap
struct stores this data. TheCel
struct has been refactored to support a newTilemap
enum case.External files chunk
From the spec:
The new
external_file
module supports parsing these chunks and theAsepriteFile
struct now exposes them. Aseprite itself currently has no way of creating .aseprite files with these chunks, so for now we have no test cases against this.User data chunk changes
From the spec:
Asefile currently just parses UserData chunks and discards them. To support these chunks properly, we'll need to also contextualize them (e.g. Sprite UserData, Tag UserData, etc). This PR does not include any changes around this.
Tileset chunk
Link to the spec
The new
tileset
module supports creating these chunks and exposing them as part of the exported AsepriteFile struct.What's changed
AseReader
The new AseReader struct wraps over some common reading behaviors. This change touches most lines of 'input' code to use AseReader methods.
Pixel
The new pixel module supports mapping the various pixel formats of Aseprite to structs. This supports image logic for both Tilesets and Cels, which each contain pixel data.
Tests
Two new test cases test against RGBA and Indexed Tilemap files respectively. We can add more tests (and functionality) once Aseprite itself catches up to its file spec.