Fix reverse-graphics check for whether the tilemap references too-high tile IDs in bank 1 (#2086)

src/gfx/reverse.cpp

@@ -279,7 +279,7 @@ void reverse() {
 	}
 
 	std::optional<std::vector<uint8_t>> attrmap;
-	uint16_t nbTilesInBank[2] = {0, 0}; // Only used if there is an attrmap.
+	uint16_t nbTilesMappedInBank[2] = {0, 0}; // Only used if there is an attrmap.
 	if (!options.attrmap.empty()) {
 		attrmap = readInto(options.attrmap);
 		if (attrmap->size() != mapSize) {
@@ -329,8 +329,8 @@ void reverse() {
 				// The unsigned underflow for `tileOfs` is intentional, since a nonzero
 				// base tile ID may overflow and continue with IDs from 0.
 				if (uint8_t tileOfs = (*tilemap)[index] - options.baseTileIDs[bank];
-				    tileOfs >= nbTilesInBank[bank]) {
-					nbTilesInBank[bank] = tileOfs + 1;
+				    tileOfs >= nbTilesMappedInBank[bank]) {
+					nbTilesMappedInBank[bank] = tileOfs + 1;
 				}
 			}
 		}
@@ -338,29 +338,43 @@ void reverse() {
 		verbosePrint(
 		    VERB_INFO,
 		    "Number of tiles in bank {0: %" PRIu16 ", 1: %" PRIu16 "}\n",
-		    nbTilesInBank[0],
-		    nbTilesInBank[1]
+		    nbTilesMappedInBank[0],
+		    nbTilesMappedInBank[1]
 		);
 
 		for (int bank = 0; bank < 2; ++bank) {
-			if (nbTilesInBank[bank] > options.maxNbTiles[bank]) {
+			if (nbTilesMappedInBank[bank] > options.maxNbTiles[bank]) {
 				error(
 				    "Bank %d contains %" PRIu16 " tiles, but the specified limit is %" PRIu16,
 				    bank,
-				    nbTilesInBank[bank],
+				    nbTilesMappedInBank[bank],
 				    options.maxNbTiles[bank]
 				);
 			}
 		}
 
-		if (nbTilesInBank[0] + nbTilesInBank[1] > nbTiles + options.trim) {
-			fatal(
-			    "The tilemap references %" PRIu16 " tiles in bank 0 and %" PRIu16
-			    " in bank 1, but only %zu have been read in total",
-			    nbTilesInBank[0],
-			    nbTilesInBank[1],
-			    nbTiles
-			);
+		if (uint16_t const maxTotalNbTiles =
+		        nbTilesMappedInBank[1] > 0
+		            ? std::max<uint16_t>(
+		                  nbTilesMappedInBank[0], options.maxNbTiles[0] + nbTilesMappedInBank[1]
+		              )
+		            : nbTilesMappedInBank[0];
+		    maxTotalNbTiles > nbTiles + options.trim) {
+			std::string message =
+			    "The tilemap references " + std::to_string(nbTilesMappedInBank[0]) + " tiles";
+			if (nbTilesMappedInBank[1] > 0) {
+				if (nbTilesMappedInBank[0] != options.maxNbTiles[0]) {
+					message += " out of a maximum " + std::to_string(options.maxNbTiles[0]);
+				}
+				message += " in bank 0, and " + std::to_string(nbTilesMappedInBank[1])
+				           + " in bank 1 (total: " + std::to_string(maxTotalNbTiles) + ")";
+			}
+			message += ", but only " + std::to_string(nbTiles) + " have been read";
+			if (options.trim > 0) {
+				message += " plus " + std::to_string(options.trim)
+				           + " trimmed (total: " + std::to_string(nbTiles + options.trim) + ")";
+			}
+			fatal("%s", message.c_str());
 		}
 
 		requireZeroErrors();

test/gfx/reverse_tilemap_oob.err

@@ -0,0 +1,2 @@
+FATAL: The tilemap references 2 tiles out of a maximum 8 in bank 0, and 2 in bank 1 (total: 10), but only 8 have been read plus 1 trimmed (total: 9)
+Conversion aborted after 1 error

test/gfx/reverse_tilemap_oob.flags

@@ -0,0 +1,4 @@
+-N 8,8
+-x 1
+-t reverse_tilemap_oob.tilemap
+-a reverse_tilemap_oob.attrmap

test/gfx/test.sh

@@ -104,14 +104,20 @@ done
 
 for f in *.[12]bpp; do
 	# Do not process outputs or sample outputs of other tests as test inputs themselves
-	if [[ "$f" = result.[12]bpp ]] || [[ "$f" = *.out.[12]bpp ]]; then
+	if [[ "$f" = result.[12]bpp ]] || [[ "$f" = *.in.[12]bpp ]] || [[ "$f" = *.out.[12]bpp ]]; then
 		continue
 	fi
 
 	flags="$([[ -e "${f%.[12]bpp}.flags" ]] && echo "@${f%.[12]bpp}.flags") $([[ -e "${f%.1bpp}.flags" ]] && echo "-d 1")"
 
-	newTest "$RGBGFX $flags -o $f -r 1 result.png && $RGBGFX $flags -o result.2bpp result.png"
-	runTest && tryCmp "$f" result.2bpp || failTest $?
+	if [[ -e "${f%.[12]bpp}.err" ]]; then
+		newTest "$RGBGFX $flags -o $f -r 1 result.png"
+		runTest 2>"$errtmp"
+		diff -au --strip-trailing-cr <(sed "s/$f/<stdin>/g" "${f%.[12]bpp}.err") "$errtmp" || failTest
+	else
+		newTest "$RGBGFX $flags -o $f -r 1 result.png && $RGBGFX $flags -o result.2bpp result.png"
+		runTest && tryCmp "$f" result.2bpp || failTest $?
+	fi
 done
 
 # Test writing to stdout