diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 2d314eb5a..3f70fdb85 100644 --- a/.github/workflows/build.yml +++ b/.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 diff --git a/src/Game.h b/src/Game.h index 1cfcc05be..61d4efb90 100644 --- a/src/Game.h +++ b/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- diff --git a/src/SConscript b/src/SConscript index 58aa1b369..38f1aba81 100644 --- a/src/SConscript +++ b/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') diff --git a/src/render/GameRenderUnits.cpp b/src/render/GameRenderUnits.cpp index 778ccb057..e55d477f7 100644 --- a/src/render/GameRenderUnits.cpp +++ b/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->actiontypeNum]; 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 (mapSize >> 1)) + off -= mapSize; + return slotTile - off; +} diff --git a/test/EnteringUnitDrawHarness.cpp b/test/EnteringUnitDrawHarness.cpp new file mode 100644 index 000000000..2dda15c57 --- /dev/null +++ b/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 +#ifdef __APPLE__ +#include +#else +#include +#endif +#include +#include +#include +#include +#include +#include +#include + +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 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 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 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; +} diff --git a/test/README.md b/test/README.md index 604312137..746ec192d 100644 --- a/test/README.md +++ b/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.png` and, +on failure, `walking-delta.png` for the unit-only comparison, plus +`scene-delta.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` diff --git a/test/SConstruct b/test/SConstruct index ba683ed34..5408607b0 100644 --- a/test/SConstruct +++ b/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 diff --git a/test/UnitDrawGeometryTest.cpp b/test/UnitDrawGeometryTest.cpp new file mode 100644 index 000000000..cb311e21d --- /dev/null +++ b/test/UnitDrawGeometryTest.cpp @@ -0,0 +1,69 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +#include +#include "UnitDrawGeometry.h" + +// Regression cover for the enter-the-building animation: a unit entering a +// building keeps its map slot on the tile it is leaving, so the draw loop +// visits it one square behind its own position. +class UnitDrawGeometryTest : public CppUnit::TestFixture +{ + CPPUNIT_TEST_SUITE(UnitDrawGeometryTest); + CPPUNIT_TEST(testSlotOnUnitPosition); + CPPUNIT_TEST(testSlotLagsWhileEnteringBuilding); + CPPUNIT_TEST(testSeamOccurrencesStayDistinct); + CPPUNIT_TEST(testSeamOccurrencesWhileEnteringBuilding); + CPPUNIT_TEST_SUITE_END(); + + static constexpr int MAP_SIZE = 64; + +public: + //! Every action but entering a building claims the destination map slot, + //! so the visited tile is already the unit's tile and must be kept as-is. + void testSlotOnUnitPosition() + { + for (int viewport = 0; viewport < MAP_SIZE; ++viewport) + for (int slotTile = -1; slotTile <= 20; ++slotTile) + { + const int unitPos = (slotTile + viewport) & (MAP_SIZE - 1); + CPPUNIT_ASSERT_EQUAL(slotTile, + unitDrawTile(slotTile, viewport, unitPos, MAP_SIZE)); + } + } + + //! Unit::handleActionEnteringBuilding advances posX/posY onto the building + //! tile but leaves the map slot behind, in any of the eight directions. + //! The anchor must follow the position, not the stale slot — otherwise the + //! arrival interpolation runs a square early and the unit walks backwards. + void testSlotLagsWhileEnteringBuilding() + { + for (int step = -1; step <= 1; ++step) + for (int viewport = 0; viewport < MAP_SIZE; ++viewport) + for (int slotTile = -1; slotTile <= 20; ++slotTile) + { + const int unitPos = (slotTile + viewport + step) & (MAP_SIZE - 1); + CPPUNIT_ASSERT_EQUAL(slotTile + step, + unitDrawTile(slotTile, viewport, unitPos, MAP_SIZE)); + } + } + + //! A wrapped tile visible at both screen edges is visited twice; each + //! occurrence must keep its own anchor so both copies of the unit are drawn. + void testSeamOccurrencesStayDistinct() + { + // Map tile 63 shows up as viewport-relative -1 and as 63. + CPPUNIT_ASSERT_EQUAL(-1, unitDrawTile(-1, 0, MAP_SIZE - 1, MAP_SIZE)); + CPPUNIT_ASSERT_EQUAL(MAP_SIZE - 1, unitDrawTile(MAP_SIZE - 1, 0, MAP_SIZE - 1, MAP_SIZE)); + } + + //! Both properties at once: the seam copies stay distinct while each still + //! tracks the one-square lag of a unit entering a building across the seam. + void testSeamOccurrencesWhileEnteringBuilding() + { + // Slot on tile 63, building tile 0: the left-edge occurrence walks into + // tile 0 and the right-edge one walks off past tile 63. + CPPUNIT_ASSERT_EQUAL(0, unitDrawTile(-1, 0, 0, MAP_SIZE)); + CPPUNIT_ASSERT_EQUAL(MAP_SIZE, unitDrawTile(MAP_SIZE - 1, 0, 0, MAP_SIZE)); + } +}; + +CPPUNIT_TEST_SUITE_REGISTRATION(UnitDrawGeometryTest);