Anchor an entering unit's sprite on its own tile, not its map slot - #231
Merged
Conversation
PR #218 changed Game::drawUnit to derive the sprite origin from the tile the draw loop found the unit on (`px = x * 32`) instead of converting unit->posX/posY, so that a unit sitting on a map seam keeps the copy at the opposite screen edge instead of collapsing to one occurrence. Those two tiles agree for every action that clears the old map slot and claims the destination one. Entering a building is the deliberate exception: Unit::handleActionEnteringBuilding leaves the map slot on the tile the unit is leaving so the unit stays drawable, while posX/posY already name the building tile. Game::integrity() enforces exactly this (test/EnteringUnitSaveHarness.cpp: "entering unit registered at destination is invalid"). Anchoring on the stale slot ran the arrival interpolation `px -= dx*(255-delta)>>3` one square early, so a glob snapped back a square and walked into the tile it came from. Keep the per-occurrence anchor and recover the unit's own tile from it by folding in the signed wrapped offset to posX/posY — the same anchor-recovery idiom PR #218 uses for building footprints in drawMapGroundBuildings, but signed, because a unit's slot can sit one square behind its position as well as ahead of it. The arithmetic lives in src/render/UnitDrawGeometry.h so it can be tested without a GraphicContext; UnitDrawGeometryTest covers the coinciding case, all eight entry directions, and the seam occurrences on their own and combined with an entry across the seam. Fixes #230. Opus 5 helped authoring this commit.
UnitDrawGeometryTest pins the arithmetic, but nothing checked that the real render path puts the glob where the arithmetic says. This adds a GL harness that draws the state issue #230 is about and compares framebuffers. A unit on its final step into a building keeps its map slot on the tile it is leaving while posX/posY already name the building tile, so the draw loop 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 over five points of one step and reports any difference in pixels against the 32 px tile size. Against the pre-fix anchor it reports exactly -32 px at delta 0 — one tile backwards, which is the reported symptom. With the fix all five deltas match. The comparison uses the unit-only render, which has no animated water or clouds to make two frames differ by themselves; a second pass captures terrain, glob and inn together as scene-delta<N>.png purely so the animation can be looked at by eye. CI already installs xvfb and mesa for the fullscreen aspect harness, so wiring this in is a two-liner; test/README.md carries the step verbatim. It is not applied here because the push token has no workflow scope. Opus 5 helped authoring this commit.
…ng-unit # Conflicts: # src/render/GameRenderUnits.cpp
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #230 — globs jump back one square when they enter a building.
Cause
PR #218 (
3ad40cf3b, merged today) changedGame::drawUnitto derive the sprite origin from the tile the draw loop found the unit on rather than from the unit's own position:That was needed: re-converting the canonical position collapses a wrapped tile visible at both screen edges into a single occurrence, so a unit standing on a map seam lost its opposite-edge copy.
The two tiles agree for every action that clears the old map slot and claims the destination one. Entering a building is the deliberate exception —
Unit::handleActionEnteringBuildingleaves the map slot on the tile the unit is leaving ("we don't delete the unit on the map because we have to draw it while it is entering") whileposX/posYalready name the building tile.Game::integrity()enforces that invariant, andtest/EnteringUnitSaveHarness.cppasserts both halves of it.So the arrival interpolation
px -= (dx*(255-delta))>>3started from the tile behind the unit: the glob snapped back a square and walked into the square it had just left.Fix
Keep the per-occurrence anchor, and recover the unit's own tile from it by folding in the signed wrapped offset to
posX/posY. This is the same anchor-recovery idiom PR #218 already uses for building footprints indrawMapGroundBuildings, made signed because a unit's slot can sit one square behind its position, not only ahead of it. When the two tiles coincide — every case but entering a building — the result isxunchanged, so #218's seam behaviour is preserved exactly.The arithmetic is in
src/render/UnitDrawGeometry.hso it is testable without aGraphicContext.Verification
Rendered proof.
test/EnteringUnitDrawHarness.cpp(new) draws the actual state throughGame::drawMapGroundUnitsand compares framebuffers. Nothing inGame::drawUnitreadsdisplacement, so at equaldeltaan entering unit must render pixel-for-pixel like the same step expressed as an ordinary walk onto the destination tile. Against the pre-fix anchor it reports exactly-32 pxatdelta 0— one tile backwards, the reported symptom. With the fix all five sampled deltas match:A second pass in the harness captures terrain, glob and inn together so the animation can be looked at by eye. One step into an inn, before and after — top row is
masterat87bdee0fd, where the glob starts a square left of where it is and arrives on the grass instead of the inn:Also:
test/UnitDrawGeometryTest.cpp(new, 4 cases): the coinciding case over every viewport/tile combination; the entering-building lag in all eight directions over every viewport/tile combination; the two seam occurrences staying distinct; and both properties together for an entry across the seam.return slotTile,testSlotLagsWhileEnteringBuildingandtestSeamOccurrencesWhileEnteringBuildingfail; with the fix,./TestsRunnerreportsOK (175 tests).scons -j4 release=1 server=0builds clean, andEnteringUnitSaveHarnesspasses both CI invocations (40 directions/positions and corruption controls, plus the saved-fixture load).One thing to apply by hand: the draw harness should run in CI, but the push token has no
workflowscope so this branch cannot touch.github/workflows/build.yml. The job already installsxvfband mesa for the fullscreen aspect harness, so the step is:test/README.mdcarries the same snippet.Base:
87bdee0fd.🤖 Generated with Claude Code