Anchor an entering unit's sprite on its own tile, not its map slot

.github/workflows/build.yml

@@ -88,6 +88,11 @@ jobs:
           ./build/src/EnteringUnitSaveHarness
           ./build/src/EnteringUnitSaveHarness --load test/fixtures/entering-explorer/reproducer.game
 
+      - name: Build and run the entering unit draw regression
+        run: |
+          scons -j$(nproc) release=1 server=0 entering-unit-draw-test
+          timeout 300s xvfb-run -a -s '-screen 0 1024x768x24' ./build/src/EnteringUnitDrawHarness
+
       - name: Build and run the immobile unit gradient regression
         run: |
           scons -j$(nproc) release=1 server=0 immobile-unit-gradient-test

src/Game.h

@@ -121,6 +121,7 @@ class Game
 {
 	bool hasSavedRandomState = false;
 	friend class HighResolutionIntegrationHarness;
+	friend class EnteringUnitDrawHarness;
 	static const bool verbose = false;
 public:
 	/// Per-client viewer state (selection + mouse). Defined below; forward-

src/SConscript

@@ -587,6 +587,11 @@ if not env['server']:
     regression_sources += local.Object('ResourceFetchTargetHarness.o', '#test/ResourceFetchTargetHarness.cpp')
     regression_test = local.Program('ResourceFetchTargetHarness', regression_sources)
     local.Alias('resource-fetch-target-test', regression_test)
+if not env['server'] and 'entering-unit-draw-test' in COMMAND_LINE_TARGETS:
+    entering_draw_sources = [source for source in source_files if source != 'Glob2.cpp']
+    entering_draw_sources += local.Object('EnteringUnitDrawHarness.o', '#test/EnteringUnitDrawHarness.cpp')
+    entering_draw_test = local.Program('EnteringUnitDrawHarness', entering_draw_sources)
+    local.Alias('entering-unit-draw-test', entering_draw_test)
 if not env['server']:
     highres_sources = [source for source in source_files if source != 'Glob2.cpp']
     highres_sources += local.Object('HighResolutionIntegrationHarness.o', '#test/HighResolutionIntegrationHarness.cpp')

src/render/GameRenderUnits.cpp

@@ -21,6 +21,7 @@
 #include "GlobalContainer.h"
 #include "Order.h"
 #include "Unit.h"
+#include "UnitDrawGeometry.h"
 #include "UnitSkin.h"
 #include "Utilities.h"
 #include "GameGUI.h"
@@ -61,9 +62,12 @@ void Game::drawUnit(int x, int y, Uint16 gid, int viewportX, int viewportY, int
 	assert(unit->action<NB_MOVE);
 	const UnitSkin &skin = g_unitSkins[unit->typeNum];
 	imgid=skin.startImage[unit->action];
-	// Draw the map copy being visited, including repeated copies in wide views.
-	int px = x * 32;
-	int py = y * 32;
+	// Anchor on the visible occurrence x/y rather than on unit->posX/posY, so a
+	// unit on a map seam keeps its opposite-edge copy, and recover the unit's
+	// own tile from it: while entering a building the map slot lags one square
+	// behind the position (see UnitDrawGeometry.h).
+	int px = unitDrawTile(x, viewportX, unit->posX, map.getW()) * Map::TILE_PX;
+	int py = unitDrawTile(y, viewportY, unit->posY, map.getH()) * Map::TILE_PX;
 	int deltaLeft=255-unit->delta;
 	if (unit->action<BUILD)
 	{

src/render/UnitDrawGeometry.h

@@ -0,0 +1,29 @@
+// SPDX-License-Identifier: GPL-3.0-or-later
+#pragma once
+
+//! Viewport-relative tile a unit's sprite is anchored on, along one wrapped axis.
+//!
+//! \param slotTile viewport-relative tile the draw loop found the unit's map slot on
+//! \param viewportStart map coordinate of the viewport's first tile on this axis
+//! \param unitPos the unit's own map coordinate on this axis (Unit::posX / posY)
+//! \param mapSize the map's extent on this axis, always a power of two
+//!
+//! The two tiles normally coincide: every unit action clears the old map slot
+//! and claims the destination one, so the visited tile already is the unit's
+//! position. Entering a building is the deliberate exception — the map slot
+//! stays on the tile the unit is leaving so it remains drawable, while
+//! posX/posY already name the building tile (Unit::handleActionEnteringBuilding).
+//! Anchoring on the visited tile alone would then run the arrival interpolation
+//! one square too early, so the signed wrapped offset between the two is folded
+//! back in.
+//!
+//! Working from the visited occurrence, rather than converting unitPos afresh,
+//! is what keeps both copies of a unit standing on a map seam: a wrapped tile
+//! visible at each screen edge is visited twice and must be drawn twice.
+inline int unitDrawTile(int slotTile, int viewportStart, int unitPos, int mapSize)
+{
+	int off = (slotTile + viewportStart - unitPos) & (mapSize - 1);
+	if (off > (mapSize >> 1))
+		off -= mapSize;
+	return slotTile - off;
+}

test/EnteringUnitDrawHarness.cpp

@@ -0,0 +1,218 @@
+// SPDX-License-Identifier: GPL-3.0-or-later
+// Renders a worker on its final step into a building and requires the sprite to
+// land exactly where a worker walking onto the same tile lands.
+//
+// A unit entering a building keeps its map slot on the tile it is leaving
+// (Unit::handleActionEnteringBuilding) while posX/posY already name the
+// building tile, so the draw loop visits it one square behind itself. Nothing
+// in Game::drawUnit reads `displacement`: at equal delta the two states are the
+// same picture, and any difference means the sprite is anchored on the wrong
+// tile — the "entering a building jumps back one square" regression.
+#include "GlobalContainer.h"
+#include "Game.h"
+#include "Unit.h"
+#include "Building.h"
+#include "IntBuildingType.h"
+#include "GraphicContext.h"
+#include <SDL_image.h>
+#ifdef __APPLE__
+#include <OpenGL/gl.h>
+#else
+#include <epoxy/gl.h>
+#endif
+#include <algorithm>
+#include <cstdio>
+#include <cstdlib>
+#include <filesystem>
+#include <string>
+#include <set>
+#include <vector>
+
+GlobalContainer* globalContainer = nullptr;
+
+namespace
+{
+	// Screen and map geometry. The map is deliberately larger than the drawn
+	// region so nothing wraps into it; the seam cases are covered by the pure
+	// UnitDrawGeometryTest.
+	const int SCREEN_W = 1024;
+	const int SCREEN_H = 768;
+	const int DRAW_W = 512;
+	const int DRAW_H = 512;
+	//! Tile the unit is walking from, and the building tile it walks into.
+	const int FROM_X = 8, FROM_Y = 8;
+	const int INTO_X = 9, INTO_Y = 8;
+
+	const char* outputDir = ".cache/entering-unit-draw-check";
+	//! Sampled points of one step: arrival, three intermediates, departure.
+	const int DELTAS[] = {0, 63, 128, 191, 255};
+
+	void require(bool ok, const char* message)
+	{
+		if (!ok) { std::fprintf(stderr, "FAIL: %s\n", message); std::exit(1); }
+	}
+
+	struct Frame
+	{
+		std::vector<unsigned char> data;
+		int w = 0, h = 0;
+	};
+
+	Frame grab()
+	{
+		Sprite::flushBatches(globalContainer->gfx);
+		glFinish();
+		GLint viewport[4];
+		glGetIntegerv(GL_VIEWPORT, viewport);
+		Frame frame;
+		frame.w = viewport[2];
+		frame.h = viewport[3];
+		frame.data.resize(frame.w * frame.h * 4);
+		glReadPixels(viewport[0], viewport[1], frame.w, frame.h, GL_RGBA, GL_UNSIGNED_BYTE, frame.data.data());
+		require(glGetError() == GL_NO_ERROR, "read back the framebuffer");
+		return frame;
+	}
+
+	void save(const Frame& frame, const std::string& name)
+	{
+		std::vector<unsigned char> flipped(frame.data.size());
+		for (int y = 0; y < frame.h; ++y)
+			std::copy_n(frame.data.data() + y * frame.w * 4, frame.w * 4,
+				flipped.data() + (frame.h - 1 - y) * frame.w * 4);
+		auto* surface = SDL_CreateRGBSurfaceWithFormatFrom(flipped.data(), frame.w, frame.h, 32,
+			frame.w * 4, SDL_PIXELFORMAT_RGBA32);
+		require(surface != nullptr, "wrap the framebuffer for PNG output");
+		require(IMG_SavePNG(surface, (std::string(outputDir) + "/" + name + ".png").c_str()) == 0,
+			"write the PNG");
+		SDL_FreeSurface(surface);
+	}
+
+	//! Horizontal extent of everything drawn on the cleared background, in gfx
+	//! coordinates. With only one unit on screen this is the glob's sprite.
+	void spriteSpan(const Frame& frame, int* left, int* right)
+	{
+		*left = SCREEN_W;
+		*right = -1;
+		for (int y = 0; y < frame.h; ++y)
+			for (int x = 0; x < frame.w; ++x)
+			{
+				const unsigned char* pixel = &frame.data[(y * frame.w + x) * 4];
+				if (!pixel[0] && !pixel[1] && !pixel[2])
+					continue;
+				const int gfxX = x * SCREEN_W / frame.w;
+				*left = std::min(*left, gfxX);
+				*right = std::max(*right, gfxX);
+			}
+	}
+}
+
+class EnteringUnitDrawHarness
+{
+	//! Game::drawMapGroundUnits is private; this class is a friend of Game.
+	static Frame render(Game& game, Game::ViewState& view)
+	{
+		auto* gfx = globalContainer->gfx;
+		gfx->drawFilledRect(0, 0, gfx->getW(), gfx->getH(), 0, 0, 0);
+		game.drawMapGroundUnits(0, 0, DRAW_W >> 5, DRAW_H >> 5, DRAW_W, DRAW_H,
+			0, 0, 0, Game::DRAW_WHOLE_MAP, view);
+		return grab();
+	}
+
+	//! Terrain, unit and building together, purely so a human can look at the
+	//! animation. The comparison below stays on the unit-only render, which has
+	//! no animated water or clouds to make frames differ by themselves.
+	static void capturePresentation(Game& game, Unit* unit, Game::ViewState& view)
+	{
+		auto* gfx = globalContainer->gfx;
+		std::set<Building*> visible;
+		for (int delta : DELTAS)
+		{
+			unit->delta = delta;
+			gfx->drawFilledRect(0, 0, gfx->getW(), gfx->getH(), 0, 0, 0);
+			game.drawMapTerrain(0, 0, DRAW_W >> 5, DRAW_H >> 5, 0, 0, 0, Game::DRAW_WHOLE_MAP);
+			game.drawMapGroundUnits(0, 0, DRAW_W >> 5, DRAW_H >> 5, DRAW_W, DRAW_H,
+				0, 0, 0, Game::DRAW_WHOLE_MAP, view);
+			game.drawMapGroundBuildings(0, 0, DRAW_W >> 5, DRAW_H >> 5, DRAW_W, DRAW_H,
+				0, 0, 0, Game::DRAW_WHOLE_MAP, &visible, nullptr);
+			save(grab(), "scene-delta" + std::to_string(delta));
+		}
+	}
+
+public:
+	static void run()
+	{
+		Game game(nullptr);
+		game.map.setSize(5, 5, GRASS);
+		game.map.setGame(&game);
+		game.addTeam(0);
+		require(game.addBuilding(INTO_X, INTO_Y,
+			globalContainer->buildingsTypes.getFinishedTypeNum("inn"), 0) != nullptr,
+			"place the inn the worker walks into");
+
+		Game::ViewState view;
+		Unit* unit = game.addUnit(FROM_X, FROM_Y, 0, WORKER, 0, 0, 1, 0);
+		require(unit != nullptr, "create the worker");
+		unit->action = WALK;
+		unit->directionFromDxDy();
+		require(game.map.getGroundUnit(FROM_X, FROM_Y) == unit->gid,
+			"the worker starts registered on the tile it is leaving");
+		// Entering a building: posX/posY moved onto the building tile, the map slot
+		// deliberately left behind so the unit stays drawable.
+		unit->posX = INTO_X;
+		unit->posY = INTO_Y;
+
+		capturePresentation(game, unit, view);
+
+		int checked = 0;
+		for (int delta : DELTAS)
+		{
+			unit->delta = delta;
+			const Frame entering = render(game, view);
+
+			// The same instant of the same step, expressed the way every other
+			// action leaves it: slot and position both on the destination tile.
+			game.map.setGroundUnit(FROM_X, FROM_Y, NOGUID);
+			game.map.setGroundUnit(INTO_X, INTO_Y, unit->gid);
+			const Frame walking = render(game, view);
+			game.map.setGroundUnit(INTO_X, INTO_Y, NOGUID);
+			game.map.setGroundUnit(FROM_X, FROM_Y, unit->gid);
+
+			int enteringLeft, enteringRight, walkingLeft, walkingRight;
+			spriteSpan(entering, &enteringLeft, &enteringRight);
+			spriteSpan(walking, &walkingLeft, &walkingRight);
+			// A drawn glob is the whole point; two empty frames would compare equal.
+			require(enteringRight >= enteringLeft, "the entering worker is drawn at all");
+			std::printf("delta %3d: entering sprite x %d..%d, walking x %d..%d\n",
+				delta, enteringLeft, enteringRight, walkingLeft, walkingRight);
+			save(entering, "entering-delta" + std::to_string(delta));
+			if (entering.data != walking.data)
+			{
+				save(walking, "walking-delta" + std::to_string(delta));
+				std::fprintf(stderr,
+					"FAIL: at delta %d the entering worker is drawn %d px from where the same "
+					"step drawn as a walk puts it (one tile is 32 px); see %s/\n",
+					delta, enteringLeft - walkingLeft, outputDir);
+				std::exit(1);
+			}
+			++checked;
+		}
+		std::printf("Entering-unit draw regression passed: %d deltas render identically to the "
+			"equivalent walk\n", checked);
+	}
+};
+
+int main()
+{
+	std::filesystem::create_directories(outputDir);
+	GlobalContainer globals("glob2-entering-unit-draw-test");
+	globalContainer = &globals;
+	globals.settings.screenWidth = SCREEN_W;
+	globals.settings.screenHeight = SCREEN_H;
+	globals.settings.screenFlags = GraphicContext::USEGPU;
+	globals.settings.rememberUnit = false;
+	globals.settings.mute = 1;
+	globals.load();
+	IntBuildingType::init();
+	EnteringUnitDrawHarness::run();
+	return 0;
+}

test/README.md

@@ -171,6 +171,38 @@ it on both supported Ubuntu versions.
 
 Saved state and step-by-step before/after reproduction: [PR #166 fixture](fixtures/entering-explorer/README.md).
 
+## Entering unit draw regression
+
+From the repository root, run `scons -j8 release=1 server=0 entering-unit-draw-test`
+and `xvfb-run -a -s '-screen 0 1024x768x24' ./build/src/EnteringUnitDrawHarness`.
+
+A unit on its final step into a building keeps its map slot on the tile it is
+leaving (`Unit::handleActionEnteringBuilding`) while `posX`/`posY` already name
+the building tile, so `Game::drawMapGroundUnits` visits it one square behind
+itself. Nothing in `Game::drawUnit` reads `displacement`, so at equal `delta`
+that state must render pixel-for-pixel like the same step expressed as an
+ordinary walk onto the destination tile. The harness renders both and compares
+framebuffers over five points of one step; a mismatch is reported in pixels
+against the 32 px tile size. It protects the fix in `src/render/UnitDrawGeometry.h`
+for issue #230, where the sprite was anchored on the stale map slot and the glob
+walked backwards into the square it came from.
+
+Frames land in `.cache/entering-unit-draw-check/`: `entering-delta<N>.png` and,
+on failure, `walking-delta<N>.png` for the unit-only comparison, plus
+`scene-delta<N>.png` with terrain and the inn for looking at by eye. The
+comparison itself stays on the unit-only render, which has no animated water or
+clouds to make two frames differ by themselves.
+
+It needs a display and a GL context. CI already installs `xvfb` and mesa for the
+fullscreen aspect harness, so a job step is a two-liner:
+
+```yaml
+      - name: Build and run the entering unit draw regression
+        run: |
+          scons -j$(nproc) release=1 server=0 entering-unit-draw-test
+          timeout 300s xvfb-run -a -s '-screen 0 1024x768x24' ./build/src/EnteringUnitDrawHarness
+```
+
 ## Immobile unit gradient regression
 
 From the repository root, run `scons -j8 release=1 server=0 immobile-unit-gradient-test`

test/SConstruct

@@ -22,6 +22,7 @@ common_cpppath = ['..', '../src', '../src/ai',
                   '../src/yog', '../src/sgsl',
                   '../src/map', '../src/map/edit', '../src/map/generator',
                   '../src/map/gradient', '../src/map/io', '../src/map/pathfind',
+                  '../src/render',
                   '../libgag/include', '../libusl/src']
 common_defines = ['HAVE_CONFIG_H', '_THREAD_SAFE']
 
@@ -102,6 +103,8 @@ ParticleCrossfadeTest.cpp
 
 UnitTimingTest.cpp
 
+UnitDrawGeometryTest.cpp
+
 CortexUpgradeTest.cpp
 
 FilenameStripTest.cpp