From 4c6342cb73155a14b83142e80a2b54aff3ef73c5 Mon Sep 17 00:00:00 2001 From: scawful Date: Thu, 25 Sep 2025 11:16:13 -0400 Subject: [PATCH] Refactor UnpackBppTile and PackBppTile functions for clarity and correctness - Initialized the snes_tile8 structure to zero in UnpackBppTile for safety. - Reordered loops in UnpackBppTile to process rows before columns, improving readability. - Corrected bit manipulation logic in both UnpackBppTile and PackBppTile to ensure accurate color value handling. - Enhanced test cases for 2bpp and 4bpp tile unpacking to validate expected results, improving test coverage. --- src/app/gfx/snes_tile.cc | 78 +++++++++++++++++++------------------- test/gfx/snes_tile_test.cc | 64 +++++++++++++++++++++---------- 2 files changed, 84 insertions(+), 58 deletions(-) diff --git a/src/app/gfx/snes_tile.cc b/src/app/gfx/snes_tile.cc index 9fc011f1..a6a5a3aa 100644 --- a/src/app/gfx/snes_tile.cc +++ b/src/app/gfx/snes_tile.cc @@ -22,47 +22,47 @@ constexpr uint16_t TileNameMask = 0x03FF; snes_tile8 UnpackBppTile(std::span data, const uint32_t offset, const uint32_t bpp) { - snes_tile8 tile; + snes_tile8 tile = {}; // Initialize to zero assert(bpp >= 1 && bpp <= 8); unsigned int bpp_pos[8]; // More for conveniance and readibility - for (int col = 0; col < 8; col++) { - for (int row = 0; row < 8; row++) { + for (int row = 0; row < 8; row++) { // Process rows first (Y coordinate) + for (int col = 0; col < 8; col++) { // Then columns (X coordinate) if (bpp == 1) { - tile.data[col * 8 + row] = (data[offset + col] >> (7 - row)) & 0x01; + tile.data[row * 8 + col] = (data[offset + row] >> (7 - col)) & 0x01; continue; } /* SNES bpp format interlace each byte of the first 2 bitplanes. * | byte 1 of first bitplane | byte 1 of second bitplane | * | byte 2 of first bitplane | byte 2 of second bitplane | .. */ - bpp_pos[0] = offset + col * 2; - bpp_pos[1] = offset + col * 2 + 1; - char mask = 1 << (7 - row); - tile.data[col * 8 + row] = (data[bpp_pos[0]] & mask) == mask; - tile.data[col * 8 + row] |= ((data[bpp_pos[1]] & mask) == mask) << 1; + bpp_pos[0] = offset + row * 2; + bpp_pos[1] = offset + row * 2 + 1; + char mask = 1 << (7 - col); + tile.data[row * 8 + col] = (data[bpp_pos[0]] & mask) ? 1 : 0; + tile.data[row * 8 + col] |= ((data[bpp_pos[1]] & mask) ? 1 : 0) << 1; if (bpp == 3) { // When we have 3 bitplanes, the bytes for the third bitplane are after // the 16 bytes of the 2 bitplanes. - bpp_pos[2] = offset + 16 + col; - tile.data[col * 8 + row] |= ((data[bpp_pos[2]] & mask) == mask) << 2; + bpp_pos[2] = offset + 16 + row; + tile.data[row * 8 + col] |= ((data[bpp_pos[2]] & mask) ? 1 : 0) << 2; } if (bpp >= 4) { // For 4 bitplanes, the 2 added bitplanes are interlaced like the first // two. - bpp_pos[2] = offset + 16 + col * 2; - bpp_pos[3] = offset + 16 + col * 2 + 1; - tile.data[col * 8 + row] |= ((data[bpp_pos[2]] & mask) == mask) << 2; - tile.data[col * 8 + row] |= ((data[bpp_pos[3]] & mask) == mask) << 3; + bpp_pos[2] = offset + 16 + row * 2; + bpp_pos[3] = offset + 16 + row * 2 + 1; + tile.data[row * 8 + col] |= ((data[bpp_pos[2]] & mask) ? 1 : 0) << 2; + tile.data[row * 8 + col] |= ((data[bpp_pos[3]] & mask) ? 1 : 0) << 3; } if (bpp == 8) { - bpp_pos[4] = offset + 32 + col * 2; - bpp_pos[5] = offset + 32 + col * 2 + 1; - bpp_pos[6] = offset + 48 + col * 2; - bpp_pos[7] = offset + 48 + col * 2 + 1; - tile.data[col * 8 + row] |= ((data[bpp_pos[4]] & mask) == mask) << 4; - tile.data[col * 8 + row] |= ((data[bpp_pos[5]] & mask) == mask) << 5; - tile.data[col * 8 + row] |= ((data[bpp_pos[6]] & mask) == mask) << 6; - tile.data[col * 8 + row] |= ((data[bpp_pos[7]] & mask) == mask) << 7; + bpp_pos[4] = offset + 32 + row * 2; + bpp_pos[5] = offset + 32 + row * 2 + 1; + bpp_pos[6] = offset + 48 + row * 2; + bpp_pos[7] = offset + 48 + row * 2 + 1; + tile.data[row * 8 + col] |= ((data[bpp_pos[4]] & mask) ? 1 : 0) << 4; + tile.data[row * 8 + col] |= ((data[bpp_pos[5]] & mask) ? 1 : 0) << 5; + tile.data[row * 8 + col] |= ((data[bpp_pos[6]] & mask) ? 1 : 0) << 6; + tile.data[row * 8 + col] |= ((data[bpp_pos[7]] & mask) ? 1 : 0) << 7; } } } @@ -72,40 +72,40 @@ snes_tile8 UnpackBppTile(std::span data, const uint32_t offset, std::vector PackBppTile(const snes_tile8& tile, const uint32_t bpp) { // Allocate memory for output data std::vector output(bpp * 8, 0); // initialized with 0 - unsigned maxcolor = 2 << bpp; + unsigned maxcolor = 1 << bpp; // Fix: should be 1 << bpp, not 2 << bpp - // Iterate over all columns and rows of the tile - for (unsigned int col = 0; col < 8; col++) { - for (unsigned int row = 0; row < 8; row++) { - uint8_t color = tile.data[col * 8 + row]; - if (color > maxcolor) { + // Iterate over all rows and columns of the tile + for (unsigned int row = 0; row < 8; row++) { + for (unsigned int col = 0; col < 8; col++) { + uint8_t color = tile.data[row * 8 + col]; + if (color >= maxcolor) { throw std::invalid_argument("Invalid color value."); } // 1bpp format - if (bpp == 1) output[col] += (uint8_t)((color & 1) << (7 - row)); + if (bpp == 1) output[row] += (uint8_t)((color & 1) << (7 - col)); // 2bpp format if (bpp >= 2) { - output[col * 2] += ((color & 1) << (7 - row)); - output[col * 2 + 1] += (((color & 2) == 2) << (7 - row)); + output[row * 2] += ((color & 1) << (7 - col)); + output[row * 2 + 1] += (((color & 2) == 2) << (7 - col)); } // 3bpp format - if (bpp == 3) output[16 + col] += (((color & 4) == 4) << (7 - row)); + if (bpp == 3) output[16 + row] += (((color & 4) == 4) << (7 - col)); // 4bpp format if (bpp >= 4) { - output[16 + col * 2] += (((color & 4) == 4) << (7 - row)); - output[16 + col * 2 + 1] += (((color & 8) == 8) << (7 - row)); + output[16 + row * 2] += (((color & 4) == 4) << (7 - col)); + output[16 + row * 2 + 1] += (((color & 8) == 8) << (7 - col)); } // 8bpp format if (bpp == 8) { - output[32 + col * 2] += (((color & 16) == 16) << (7 - row)); - output[32 + col * 2 + 1] += (((color & 32) == 32) << (7 - row)); - output[48 + col * 2] += (((color & 64) == 64) << (7 - row)); - output[48 + col * 2 + 1] += (((color & 128) == 128) << (7 - row)); + output[32 + row * 2] += (((color & 16) == 16) << (7 - col)); + output[32 + row * 2 + 1] += (((color & 32) == 32) << (7 - col)); + output[48 + row * 2] += (((color & 64) == 64) << (7 - col)); + output[48 + row * 2 + 1] += (((color & 128) == 128) << (7 - col)); } } } diff --git a/test/gfx/snes_tile_test.cc b/test/gfx/snes_tile_test.cc index 5f6cdebd..9f5eddec 100644 --- a/test/gfx/snes_tile_test.cc +++ b/test/gfx/snes_tile_test.cc @@ -22,29 +22,55 @@ TEST(SnesTileTest, UnpackBppTile) { EXPECT_EQ(tile1bpp.data[63], 1); // Last pixel // Test 2bpp tile unpacking - std::vector data2bpp = {0x80, 0x40, 0x20, 0x10, 0x08, 0x04, - 0x02, 0x01, 0x01, 0x02, 0x04, 0x08, - 0x10, 0x20, 0x40, 0x80}; + // Create test data where we know the expected results + // For 2bpp: 16 bytes total (8 rows × 2 bytes per row) + // Each row has 2 bytes: plane 0 byte, plane 1 byte + // First pixel should be 3 (both bits set): plane0 bit7=1, plane1 bit7=1 + // Last pixel of first row should be 1: plane0 bit0=1, plane1 bit0=0 + std::vector data2bpp = { + 0x81, 0x80, // Row 0: plane0=10000001, plane1=10000000 + 0x00, 0x00, // Row 1: plane0=00000000, plane1=00000000 + 0x00, 0x00, // Row 2: plane0=00000000, plane1=00000000 + 0x00, 0x00, // Row 3: plane0=00000000, plane1=00000000 + 0x00, 0x00, // Row 4: plane0=00000000, plane1=00000000 + 0x00, 0x00, // Row 5: plane0=00000000, plane1=00000000 + 0x00, 0x00, // Row 6: plane0=00000000, plane1=00000000 + 0x01, 0x81 // Row 7: plane0=00000001, plane1=10000001 + }; auto tile2bpp = gfx::UnpackBppTile(data2bpp, 0, 2); - EXPECT_EQ(tile2bpp.data[0], 3); // First pixel (both bits set) - EXPECT_EQ(tile2bpp.data[7], - 1); // Last pixel of first row (only first bit set) - EXPECT_EQ(tile2bpp.data[56], - 2); // First pixel of last row (only second bit set) - EXPECT_EQ(tile2bpp.data[63], 3); // Last pixel (both bits set) + EXPECT_EQ(tile2bpp.data[0], 3); // First pixel: 1|1<<1 = 3 + EXPECT_EQ(tile2bpp.data[7], 1); // Last pixel of first row: 1|0<<1 = 1 + EXPECT_EQ(tile2bpp.data[56], 2); // First pixel of last row: 0|1<<1 = 2 + EXPECT_EQ(tile2bpp.data[63], 3); // Last pixel: 1|1<<1 = 3 - // Test 4bpp tile unpacking + // Test 4bpp tile unpacking + // According to SnesLab: First planes 1&2 intertwined, then planes 3&4 intertwined + // 32 bytes total: 16 bytes for planes 1&2, then 16 bytes for planes 3&4 std::vector data4bpp = { - 0x80, 0x40, 0x20, 0x10, 0x08, 0x04, 0x02, 0x01, 0x01, 0x02, 0x04, - 0x08, 0x10, 0x20, 0x40, 0x80, 0x80, 0x40, 0x20, 0x10, 0x08, 0x04, - 0x02, 0x01, 0x01, 0x02, 0x04, 0x08, 0x10, 0x20, 0x40, 0x80}; + // Planes 1&2 intertwined (rows 0-7) + 0x81, 0x80, // Row 0: bp1=10000001, bp2=10000000 + 0x00, 0x00, // Row 1: bp1=00000000, bp2=00000000 + 0x00, 0x00, // Row 2: bp1=00000000, bp2=00000000 + 0x00, 0x00, // Row 3: bp1=00000000, bp2=00000000 + 0x00, 0x00, // Row 4: bp1=00000000, bp2=00000000 + 0x00, 0x00, // Row 5: bp1=00000000, bp2=00000000 + 0x00, 0x00, // Row 6: bp1=00000000, bp2=00000000 + 0x01, 0x81, // Row 7: bp1=00000001, bp2=10000001 + // Planes 3&4 intertwined (rows 0-7) + 0x81, 0x80, // Row 0: bp3=10000001, bp4=10000000 + 0x00, 0x00, // Row 1: bp3=00000000, bp4=00000000 + 0x00, 0x00, // Row 2: bp3=00000000, bp4=00000000 + 0x00, 0x00, // Row 3: bp3=00000000, bp4=00000000 + 0x00, 0x00, // Row 4: bp3=00000000, bp4=00000000 + 0x00, 0x00, // Row 5: bp3=00000000, bp4=00000000 + 0x00, 0x00, // Row 6: bp3=00000000, bp4=00000000 + 0x01, 0x81 // Row 7: bp3=00000001, bp4=10000001 + }; auto tile4bpp = gfx::UnpackBppTile(data4bpp, 0, 4); - EXPECT_EQ(tile4bpp.data[0], 0xF); // First pixel (all bits set) - EXPECT_EQ(tile4bpp.data[7], - 0x5); // Last pixel of first row (bits 0 and 2 set) - EXPECT_EQ(tile4bpp.data[56], - 0xA); // First pixel of last row (bits 1 and 3 set) - EXPECT_EQ(tile4bpp.data[63], 0xF); // Last pixel (all bits set) + EXPECT_EQ(tile4bpp.data[0], 0xF); // First pixel: 1|1<<1|1<<2|1<<3 = 15 + EXPECT_EQ(tile4bpp.data[7], 0x5); // Last pixel of first row: 1|0<<1|1<<2|0<<3 = 5 + EXPECT_EQ(tile4bpp.data[56], 0xA); // First pixel of last row: 0|1<<1|0<<2|1<<3 = 10 + EXPECT_EQ(tile4bpp.data[63], 0xF); // Last pixel: 1|1<<1|1<<2|1<<3 = 15 } TEST(SnesTileTest, PackBppTile) {