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

Refactoring types and invariants #4

Open
B-Reif opened this issue Jul 1, 2021 · 1 comment
Open

Refactoring types and invariants #4

B-Reif opened this issue Jul 1, 2021 · 1 comment

Comments

@B-Reif
Copy link
Contributor

B-Reif commented Jul 1, 2021

I'm opening this issue to explore how we can encode some existing panic/assert invariants in static typing. This is one design I'm considering:

pub enum ImageContent {
    Rgba {
        palette: Option<ColorPalette>,
        cels: CelsData<pixel::Rgba>,
        tilesets: TilesetsById<pixel::Rgba>,
    },
    Grayscale {
        palette: Option<ColorPalette>,
        cels: CelsData<pixel::Grayscale>,
        tilesets: TilesetsById<pixel::Grayscale>,
    },
    Indexed {
        palette: ColorPalette,
        transparent_color_index: u8,
        cels: CelsData<pixel::Indexed>,
        tilesets: TilesetsById<pixel::Indexed>,
    },
}

Any invariant behavior around the pixel format (a palette must be present, etc) would be encoded in this single ImageContent enum. This would replace the PixelFormat and Pixels enums, and additionally contain the dependent data. Cels and Tilesets would become generic over a pixel type.

Cels and Tilesets also contain a lot of non-pixel metadata. Making these types generic would make it harder to work with all of the non-image content. I also considered a design where the pixel data would live in a separate map from the non-pixel metadata:

// Only these pixel tables are generic.
// The rest of the data remains non-generic, in the existing top-level maps.
// This would prevent the generic types from "leaking" into other code that touches cels or tilesets.
pub struct CelPixelsById<T>(HashMap<CelId, Vec<T>>);
pub struct TilesetPixelsById<T>(HashMap<TilesetId, Vec<T>>);

pub enum ImagePixelContent {
    Rgba {
        palette: Option<ColorPalette>,
        cels: CelPixelsById<pixel::Rgba>,
        tilesets: TilesetPixelsById<pixel::Rgba>,
    },
    // etc
}

This indirection would allow users to use Tilesets and Cels from multiple files without regard for their pixel format.

@alpine-alpaca
Copy link
Owner

Hm, I haven't found a good time to fully wrap my head around how the tileset stuff fits in.

But I have a few quick thoughts on the handling of indexed data.

  • All indexed Pixel data uses the same palette, so they can all get an Rc<Palette> so we don't need to deal with lifetime parameters.
  • The transparent pixel index would be part of the palette.
  • The palette would always be of size 256, and the unused slots would all be transparent. That way we don't have to worry about out of bounds accesses.
  • We'd still validate that all indices are in range when we convert the data read from the file into our pixel data types. That conversion/validation must obviously happen after we have read the palette from the file.

The validator should still check for out of bounds indices. But if it has a bug there won't be a crash, just some transparent pixels.

The one ugly spot would be the handling of background-layer or not. I think that info shouldn't be moved into the cel, so it must be handled somewhere else.

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

No branches or pull requests

2 participants