Merge pull request #58 from grunt-lucas/meta/issue-0012/todos

Many small changes to address FIXMEs and TODOs marked 1.0.0

include/errors_warnings.h

@@ -253,6 +253,9 @@ void fatalerror_assignCacheInvalidValue(const ErrorsAndWarnings &err, const Comp
 void fatalerror_paletteAssignParamSearchMatrixFailed(const ErrorsAndWarnings &err, const CompilerSourcePaths &srcs,
                                                      const CompilerMode &mode);
 
+void fatalerror_noImpliedLayerType(const ErrorsAndWarnings &err, const DecompilerSourcePaths &srcs,
+                                   DecompilerMode mode);
+
 /*
  * Compilation warnings (due to possible mistakes in user input), compilation can continue
  */

include/types.h

@@ -26,6 +26,7 @@ constexpr std::size_t TILE_NUM_PIX = TILE_SIDE_LENGTH_PIX * TILE_SIDE_LENGTH_PIX
 constexpr std::size_t METATILE_TILE_SIDE_LENGTH_TILES = 2;
 constexpr std::size_t METATILE_SIDE_LENGTH = TILE_SIDE_LENGTH_PIX * METATILE_TILE_SIDE_LENGTH_TILES;
 constexpr std::size_t METATILES_IN_ROW = 8;
+constexpr std::size_t METATILE_SHEET_WIDTH = METATILE_SIDE_LENGTH * METATILES_IN_ROW;
 constexpr std::size_t PAL_SIZE = 16;
 constexpr std::size_t MAX_BG_PALETTES = 16;
 constexpr std::size_t TILES_PER_METATILE_DUAL = 8;
@@ -644,11 +645,11 @@ enum class AssignAlgorithm { DFS, BFS };
 enum class DecompilerMode { PRIMARY, SECONDARY };
 
 /*
- * TODO 1.0.0 : combine CompilerMode and DecompilerMode into a single type: CompilationMode ?
+ * TODO : combine CompilerMode and DecompilerMode into a single type: CompilationMode ?
  */
 
 /*
- * TODO 1.0.0 : Remove all checks against Subcommand type in the codebase, prefer explicit CompilationMode parameters
+ * TODO : Remove all checks against Subcommand type in the codebase, prefer explicit CompilationMode parameters
  */
 
 std::string subcommandString(Subcommand subcommand);

script/todo.sh

@@ -10,6 +10,7 @@ Usage: todo.sh all
        todo.sh {NOTE,note}
        todo.sh {FEATURE,feature}
        todo.sh 1.0.0
+       todo.sh tests
        todo.sh --help
 
 Display TODO related note comments.
@@ -55,10 +56,20 @@ fi
 parse_params "$@"
 
 if [[ ${args[0]} == "all" ]]; then
+    # regular
     rg 'TODO :'
     rg 'FIXME :'
     rg 'NOTE :'
     rg 'FEATURE :'
+
+    # 1.0.0
+    rg 'TODO 1.0.0 :'
+    rg 'FIXME 1.0.0 :'
+    rg 'NOTE 1.0.0 :'
+    rg 'FEATURE 1.0.0 :'
+
+    # tests
+    rg 'TODO tests :'
 elif [[ ${args[0]} == "TODO" || ${args[0]} == "todo" ]]; then
     rg 'TODO :'
 elif [[ ${args[0]} == "FIXME" || ${args[0]} == "fixme" ]]; then
@@ -72,6 +83,8 @@ elif [[ ${args[0]} == "1.0.0" ]] then
     rg 'FIXME 1.0.0 :'
     rg 'NOTE 1.0.0 :'
     rg 'FEATURE 1.0.0 :'
+elif [[ ${args[0]} == "tests" ]] then
+    rg 'TODO tests :'
 else
     echo "error: unknown command \`${args[0]}'"
     usage_exit_error

src/decompiler.cpp

@@ -14,11 +14,6 @@
 
 namespace porytiles {
 
-/*
- * FIXME 1.0.0 : decompiler will break if attribute format is wonky, see rusturf_tunnel example from Josh
- * To fix this, we should add out-of-range warnings like we did for the tile and palette indexes
- */
-
 static RGBATile setTilePixels(const PorytilesContext &ctx, const GBATile &gbaTile, const GBAPalette &palette,
                               bool hFlip, bool vFlip)
 {
@@ -88,8 +83,8 @@ std::unique_ptr<DecompiledTileset> decompile(PorytilesContext &ctx, DecompilerMo
    * attribute count (i.e. the true metatile count). If division by 8 matches, then we are dual layer. If 12 matches, we
    * are triple. Otherwise, we have corruption and should fail.
    */
-  std::size_t dualImpliedMetatileCount = compiledTileset.metatileEntries.size() / TILES_PER_METATILE_DUAL;
-  std::size_t tripleImpliedMetatileCount = compiledTileset.metatileEntries.size() / TILES_PER_METATILE_TRIPLE;
+  auto dualImpliedMetatileCount = compiledTileset.metatileEntries.size() / TILES_PER_METATILE_DUAL;
+  auto tripleImpliedMetatileCount = compiledTileset.metatileEntries.size() / TILES_PER_METATILE_TRIPLE;
 
   if (dualImpliedMetatileCount == attributesMap.size()) {
     decompiledTileset->tripleLayer = false;
@@ -98,12 +93,7 @@ std::unique_ptr<DecompiledTileset> decompile(PorytilesContext &ctx, DecompilerMo
     decompiledTileset->tripleLayer = true;
   }
   else {
-    // TODO 1.0.0 : we should create a custom warning here so users know what happened
-    // TODO 1.0.0 : handle this via CLI flag?
-    internalerror(
-        fmt::format("emitter::emitDecompiled compiledTileset.metatileEntries.size()={}, attributesMap.size()={} "
-                    "did not imply a layer type",
-                    compiledTileset.metatileEntries.size(), attributesMap.size()));
+    fatalerror_noImpliedLayerType(ctx.err, ctx.decompilerSrcPaths, mode);
   }
 
   std::size_t metatileIndex = 0;

src/driver.cpp

@@ -597,10 +597,9 @@ static void driveEmitDecompiledTileset(PorytilesContext &ctx, DecompilerMode mod
   std::ostringstream outAttributesContent{};
   std::size_t metatileCount = attributesMap.size();
   std::size_t imageHeight = std::ceil(metatileCount / 8.0) * 16;
-  // TODO 1.0.0 : replace the 128s strewn about the code with a named constant METATILE_SHEET_WIDTH or something
-  png::image<png::rgba_pixel> bottomPng{128, static_cast<png::uint_32>(imageHeight)};
-  png::image<png::rgba_pixel> middlePng{128, static_cast<png::uint_32>(imageHeight)};
-  png::image<png::rgba_pixel> topPng{128, static_cast<png::uint_32>(imageHeight)};
+  png::image<png::rgba_pixel> bottomPng{METATILE_SHEET_WIDTH, static_cast<png::uint_32>(imageHeight)};
+  png::image<png::rgba_pixel> middlePng{METATILE_SHEET_WIDTH, static_cast<png::uint_32>(imageHeight)};
+  png::image<png::rgba_pixel> topPng{METATILE_SHEET_WIDTH, static_cast<png::uint_32>(imageHeight)};
   porytiles::emitDecompiled(ctx, mode, bottomPng, middlePng, topPng, outAttributesContent, tileset, attributesMap,
                             behaviorReverseMap);
 
@@ -731,7 +730,6 @@ static void driveDecompilePrimary(PorytilesContext &ctx)
   /*
    * Import behavior header, if it was supplied
    */
-  // TODO 1.0.0 : better error message if file did not exist? see compile version
   auto [behaviorMap, behaviorReverseMap] =
       prepareBehaviorsHeaderForImport(ctx, DecompilerMode::PRIMARY, ctx.decompilerSrcPaths.metatileBehaviors);
 
@@ -755,7 +753,6 @@ static void driveDecompileSecondary(PorytilesContext &ctx)
   /*
    * Import behavior header, if it was supplied
    */
-  // TODO 1.0.0 : better error message if file did not exist? see compile version
   auto [behaviorMap, behaviorReverseMap] =
       prepareBehaviorsHeaderForImport(ctx, DecompilerMode::SECONDARY, ctx.decompilerSrcPaths.metatileBehaviors);
 
@@ -1003,7 +1000,7 @@ TEST_CASE("drive should emit all expected files for anim_metatiles_2 primary set
 
   porytiles::drive(ctx);
 
-  // TODO 1.0.0 : test impl check pal files
+  // TODO tests : (drive should emit all expected files...) test palette files are correct
 
   // Check tiles.png
 
@@ -1170,7 +1167,7 @@ TEST_CASE("drive should emit all expected files for anim_metatiles_2 secondary s
 
   porytiles::drive(ctx);
 
-  // TODO 1.0.0 : test impl check pal files
+  // TODO tests : (drive should emit all expected files...) test palette files are correct
 
   // Check tiles.png
 
@@ -1388,7 +1385,7 @@ TEST_CASE("drive should emit all expected files for compiled_emerald_general")
       std::filesystem::path{"res/tests/compiled_emerald_general/expected_decompiled/attributes.csv"},
       parentDir / std::filesystem::path{"attributes.csv"});
 
-  // TODO : test animations once we implement anim decomp
+  // TODO tests : (drive should emit all expected files) test animations once we implement anim decomp
 
   std::filesystem::remove_all(parentDir);
 }

src/emitter.cpp

@@ -325,7 +325,7 @@ void emitDecompiled(PorytilesContext &ctx, DecompilerMode mode, png::image<png::
   else {
     outCsv << "id,behavior" << std::endl;
   }
-  // TODO 1.0.0 : if all elements in a row correspond to the selected defaults, skip emitting this row?
+  // TODO : if all elements in a row correspond to the selected defaults, skip emitting this row?
   for (std::size_t metatileIndex = 0; metatileIndex < attributesMap.size(); metatileIndex++) {
     if (ctx.targetBaseGame == TargetBaseGame::FIRERED) {
       if (behaviorReverseMap.contains(attributesMap.at(metatileIndex).metatileBehavior)) {
@@ -559,7 +559,7 @@ TEST_CASE("emitMetatilesBin should emit metatiles.bin as expected based on setti
 
 TEST_CASE("emitAnim should correctly emit compiled animation PNG files")
 {
-  // TODO 1.0.0 : test impl emitAnim should correctly emit compiled animation PNG files
+  // TODO tests : (emitAnim should correctly emit compiled animation PNG files)
 }
 
 TEST_CASE("emitAttributes should correctly emit metatile attributes")
@@ -736,5 +736,5 @@ TEST_CASE("emitAttributes should correctly emit metatile attributes")
 
 TEST_CASE("emitDecompiled should correctly emit the decompiled tileset files")
 {
-  // TODO 1.0.0 : test impl emitDecompiled should correctly emit the decompiled tileset files
+  // TODO tests : (emitDecompiled should correctly emit the decompiled tileset files)
 }

src/errors_warnings.cpp

@@ -38,7 +38,7 @@ const char *const WARN_PALETTE_INDEX_OUT_OF_RANGE = "palette-index-out-of-range"
 
 static std::string getTilePrettyString(const RGBATile &tile)
 {
-  // TODO 1.0.0 : add CLI option to display indexes according to offsets? (so they match up with Porymap?)
+  // TODO : display indexes according to offsets? (so they match up with Porymap?)
   std::string tileString = "";
   if (tile.type == TileType::LAYERED) {
     tileString = fmt::format("metatile 0x{:x} ({}), {}, {}", tile.metatileIndex, tile.metatileIndex,
@@ -102,7 +102,7 @@ void error_layerWidthNeq128(ErrorsAndWarnings &err, TileLayer layer, png::uint_3
 {
   err.errCount++;
   if (err.printErrors) {
-    pt_err("{} layer source PNG width `{}' was not 128", layerString(layer), fmt::styled(width, fmt::emphasis::bold));
+    pt_err("{} layer source PNG width `{}' was not {}", layerString(layer), fmt::styled(width, fmt::emphasis::bold), METATILE_SHEET_WIDTH);
     pt_println(stderr, "");
   }
 }
@@ -542,6 +542,16 @@ void fatalerror_paletteAssignParamSearchMatrixFailed(const ErrorsAndWarnings &er
   die_compilationTerminated(err, srcs.modeBasedSrcPath(mode), fmt::format("palette assign param search matrix failed"));
 }
 
+void fatalerror_noImpliedLayerType(const ErrorsAndWarnings &err, const DecompilerSourcePaths &srcs, DecompilerMode mode)
+{
+  if (err.printErrors) {
+    pt_fatal_err("no layer type was implied by the supplied metatiles and attributes");
+    pt_note("either you forgot to supply the correct `-target-base-game' option, or a file is corrupted");
+    pt_println(stderr, "");
+  }
+  die_decompilationTerminated(err, srcs.modeBasedSrcPath(mode), fmt::format("no implied layer type"));
+}
+
 static void printWarning(ErrorsAndWarnings &err, WarningMode warningMode, const std::string_view &warningName,
                          const std::string &message)
 {

src/importer.cpp

@@ -836,14 +836,14 @@ static std::vector<GBAPalette> importCompiledPalettes(PorytilesContext &ctx, Dec
 
   for (const std::unique_ptr<std::ifstream> &stream : paletteFiles) {
     std::string line;
-
     /*
      * TODO : should fatal errors here have better messages? Users shouldn't ever really see these errors, since
      * compiled palettes will always presumably have correct formatting unless a user has manually messed with one
      */
-
-    // FIXME 1.0.0 : this function assumes the pal file is DOS format, need to fix this
-
+    /*
+    * FIXME : this function assumes the pal file is DOS format, need to fix this
+    * Pret PR here recently addressed the gbagfx DOS line ending issue: https://github.com/pret/pokeemerald/pull/2004
+    */
     std::getline(*stream, line);
     if (line.size() == 0) {
       fatalerror(ctx.err, ctx.decompilerSrcPaths, decompilerMode, "invalid blank line in pal file");
@@ -1142,7 +1142,10 @@ RGBATile importPalettePrimer(PorytilesContext &ctx, CompilerMode compilerMode, s
    * TODO : fatalerrors in this function need better messaging
    */
 
-  // FIXME 1.0.0 : this function assumes the pal file is DOS format, need to fix this
+  /*
+   * FIXME : this function assumes the pal file is DOS format, need to fix this
+   * Pret PR here recently addressed the gbagfx DOS line ending issue: https://github.com/pret/pokeemerald/pull/2004
+   */
 
   std::string line;
   std::getline(paletteFile, line);
@@ -1746,7 +1749,7 @@ TEST_CASE("importCompiledTileset should import a triple-layer pokeemerald tilese
     std::filesystem::path paletteFile = decompileCtx.decompilerSrcPaths.primaryPalettes() / filename.str();
     paletteFiles.push_back(std::make_unique<std::ifstream>(paletteFile));
   }
-  // TODO 1.0.0 : actually test anims import
+  // TODO tests : (importCompiledTileset should import a triple-layer...) actually test anims import
   auto [importedTileset, attributesMap] =
       porytiles::importCompiledTileset(decompileCtx, porytiles::DecompilerMode::PRIMARY, metatiles, attributes,
                                        std::unordered_map<std::uint8_t, std::string>{}, tilesheetPng, paletteFiles,
@@ -1780,12 +1783,12 @@ TEST_CASE("importCompiledTileset should import a triple-layer pokeemerald tilese
     }
   }
 
-  // TODO 1.0.0 : test impl check attributes map
+  // TODO tests : (importCompiledTileset should import a triple-layer...) check attributes map correctness
 
   std::filesystem::remove_all(parentDir);
 }
 
 TEST_CASE("importCompiledTileset should import a dual-layer pokefirered tileset correctly")
 {
-  // TODO 1.0.0 : test impl importCompiledTileset should import a dual-layer pokefirered tileset correctly
+  // TODO tests : (importCompiledTileset should import a dual-layer pokefirered tileset correctly)
 }