Fix: icons structure icons and others at wrong location (#3453)

## Description: Fix for all structure icons suddenly being displaced from the actual structure location. And in some cases, structure itself created at wrong location, or coordinate grid, nuke trajectory preview target, nuke circles, naval landing target, or pop-up text of gold earned from train or tradeship. - Was triggered on iOS almost exclusively, but was also possible on other devices when a top/left margin was present. Like when an ad was shown. Why noticed almost only on iOS? Because of different behaviors where it shifts the DOM elements around relative to the screen temporarily, so we get a top/left offset on getBoundingClientRect for the canvas. Possible culprit is overscroll which lets you scroll outside of the viewport for several hundred pixels before snapping back. Which was triggered by mistake by dragging instead of tapping somewhere, or so. - Some of the bug reports: https://discord.com/channels/1284581928254701718/1451393982159523982, https://discord.com/channels/1284581928254701718/1463548362526822649, https://discord.com/channels/1284581928254701718/1378672255336189964, fixes https://github.com/openfrontio/OpenFrontIO/issues/3406 - The fix brings a little performance win as well because we need to be doing less calculations. It is basically "if drawing on a canvas, work with canvas coordinates and not with screen coordinates". Was stress tested by two players and me, see below for reproduction. - (BTW. When researching if the current logic was intended in any way, I found CodeRabbit had already noticed part of the bug twice. One of them was [resolved](https://github.com/openfrontio/OpenFrontIO/pull/2059#discussion_r2396277710), the other [left open](https://github.com/openfrontio/OpenFrontIO/pull/2059#discussion_r2370413213). Another reminder that we need to heed Rabbits calls!) **CONTAINS** - StructureIconsLayer > computeNewLocation and StructureDrawingUtils > createUnitContainer. In renderLayer, when TransformHandler.hasChanged (after onZoom, goTo, onMove), computeNewLocation recalculates the position of all structure icons. Sometimes all icons would suddenly be displaced, not above their map position but somewhere else. New single icons/levels/sprites would be placed wrongly too by computeNewLocation and createUnitContainer. -- Fix: don't use TransformHandler > worldToScreenCoordinates but the new worldToCanvasCoordinates. Because worldToScreenCoordinates adds the canvas boundingRect top/left offset. When the main canvas is already shifted down with a margin, the icons also get shifted within the canvas. So they're moved twice as much as they should be. We only need to know at what canvas location to put the icons, the main GameRender canvas already handles the overall displacement. - TransformHandler > worldToCanvasCoordinates -- Use the new worldToCanvasCoordinates too instead of worldToScreenCoordinates in CoordinateGridLayer, MoveIncicatorUI, NavalTarget, NukeTeleGraph, TextIndicator. They were affected by the same bug as the shifting Structure icons because the boundingRect top/left offset was applied twice, but it was noticed less. I have seen reports of NavalTarget or MobveIndicatorUI (for warships) not being in the correct spot though iirc. And just like for StructureIconsLayer/StructureDrawingUtils, it's only logical. Since we're drawing on the canvas, we only need to know where to place things within that canvas. - TransformHandler > worldToScreenCoordinates -- Split in two sub-functions. New seperate function worldToCanvasCoordinates was needed for the above fix. For canvasToScreenCoordinates the reason is explained below. - TransformHandler > screenToWorldCoordinates: this function already subtracts the canvas boundingRect top/left offset. Some callers were themselves getting the boundaryRect and subtracting top/left, before calling screenToWorldCoordinates. Not only unnecessary. But also, when there was more than 0 top/left offset, it would be subtracted twice from the mouse/tap position. Meaning a (ghost) structure or nuke trajectory preview target would not be put where the mouse/tap was. Same bug as above but reversed. -- Checked all callers. Most did it right. Fixes where needed in StructureIconsLayer > createStructure and renderGhost, and in NukeTrajectoryPreviewLayer > updateTrajectoryPreview and updateTrajectoryPath. -- Removed comment in screenToWorldCoordinates that talked about zoom. It doesn't belong there because we do more than zooming there, it was probably copied once from onZoom() which has the exact same comment and it belongs in that function. -- Small fix in caller BuildMenu when checking all callers of screenToWorldCoordinates: it checked if clickedCell was null, but screenToWorldCoordinates never returns null. - TransformHandler > added public helper functions screenToCanvasCoordinates and canvasToScreenCoordinates to make code re-usable -- screenToCanvasCoordinates is used in screenToWorldCoordinates and onZoom in TransformHandler itself -- screenToCanvasCoordinates is now also used also in moveGhost and createGhostStructure in StructureIconsLayer. No bugs there, but the same calculation was done (getting boundingRect, subtracting left/top from mouse/tap position). So they now use the centralized function which also adds to their readability. -- canvasToScreenCoordinates is (for now) only used in worldToScreenCoordinates in TransformHandler. It makes the function more readable imo. And, since it has such a similar calculation to screenToWorldCoordinates, it only seemed neat to have them both have their own function. **BEFORE** (only showing "all structure icons get displaced", but the cause and fix is basically the same for all) https://youtu.be/CfDdUwIRQeE **AFTER** https://youtu.be/w5w_wvh5V0g ## Please complete the following: - [x] I have added screenshots for all UI updates - [x] I process any text displayed to the user through translateText() and I've added it to the en.json file - [x] I have added relevant tests to the test directory - [x] I confirm I have thoroughly tested these changes and take full responsibility for any bugs introduced ## Please put your Discord username so you can be contacted if a bug or regression is found: tryout33
2026-06-21 11:10:42 +00:00 · 2026-03-24 00:27:46 +01:00
parent eb51853b05
commit 496f1008bb
10 changed files with 78 additions and 88 deletions
@@ -70,7 +70,7 @@ export class TransformHandler {
    );
  }

-  worldToScreenCoordinates(cell: Cell): { x: number; y: number } {
+  worldToCanvasCoordinates(cell: Cell): { x: number; y: number } {
    // Step 1: Convert from Cell coordinates to game coordinates
    // (reverse of Math.floor operation - we'll use the exact values)
    const gameX = cell.x;
@@ -90,23 +90,23 @@ export class TransformHandler {
    const canvasY =
      (centerY - this.offsetY) * this.scale + this.game.height() / 2;

-    // Step 4: Convert canvas coordinates back to screen coordinates
-    const canvasRect = this.boundingRect();
-    const screenX = canvasX + canvasRect.left;
-    const screenY = canvasY + canvasRect.top;
-    return { x: screenX, y: screenY };
+    return { x: canvasX, y: canvasY };
+  }
+
+  worldToScreenCoordinates(cell: Cell): { x: number; y: number } {
+    // Step 1-3: Convert world coordinates to canvas coordinates in worldToCanvasCoordinates
+    // Step 4 only where needed: Convert canvas coordinates back to screen coordinates
+    const canvasCoords = this.worldToCanvasCoordinates(cell);
+    return this.canvasToScreenCoordinates(canvasCoords.x, canvasCoords.y);
  }

  screenToWorldCoordinates(screenX: number, screenY: number): Cell {
-    const canvasRect = this.boundingRect();
-    const canvasX = screenX - canvasRect.left;
-    const canvasY = screenY - canvasRect.top;
+    const canvasCoords = this.screenToCanvasCoordinates(screenX, screenY);

-    // Calculate the world point we want to zoom towards
    const centerX =
-      (canvasX - this.game.width() / 2) / this.scale + this.offsetX;
+      (canvasCoords.x - this.game.width() / 2) / this.scale + this.offsetX;
    const centerY =
-      (canvasY - this.game.height() / 2) / this.scale + this.offsetY;
+      (canvasCoords.y - this.game.height() / 2) / this.scale + this.offsetY;

    const gameX = centerX + this.game.width() / 2;
    const gameY = centerY + this.game.height() / 2;
@@ -114,6 +114,25 @@ export class TransformHandler {
    return new Cell(Math.floor(gameX), Math.floor(gameY));
  }

+  canvasToScreenCoordinates(
+    canvasX: number,
+    canvasY: number,
+  ): { x: number; y: number } {
+    const canvasRect = this.boundingRect();
+    return {
+      x: canvasX + canvasRect.left,
+      y: canvasY + canvasRect.top,
+    };
+  }
+
+  screenToCanvasCoordinates(
+    screenX: number,
+    screenY: number,
+  ): { x: number; y: number } {
+    const canvasRect = this.boundingRect();
+    return { x: screenX - canvasRect.left, y: screenY - canvasRect.top };
+  }
+
  screenBoundingRect(): [Cell, Cell] {
    const canvasRect = this.boundingRect();
    const canvasWidth = canvasRect.width;
@@ -235,19 +254,19 @@ export class TransformHandler {
    // Clamp the scale to prevent extreme zooming
    this.scale = Math.max(0.2, Math.min(20, this.scale));

-    const canvasRect = this.boundingRect();
-    const canvasX = event.x - canvasRect.left;
-    const canvasY = event.y - canvasRect.top;
+    const canvasCoords = this.screenToCanvasCoordinates(event.x, event.y);

    // Calculate the world point we want to zoom towards
    const zoomPointX =
-      (canvasX - this.game.width() / 2) / oldScale + this.offsetX;
+      (canvasCoords.x - this.game.width() / 2) / oldScale + this.offsetX;
    const zoomPointY =
-      (canvasY - this.game.height() / 2) / oldScale + this.offsetY;
+      (canvasCoords.y - this.game.height() / 2) / oldScale + this.offsetY;

    // Adjust the offset
-    this.offsetX = zoomPointX - (canvasX - this.game.width() / 2) / this.scale;
-    this.offsetY = zoomPointY - (canvasY - this.game.height() / 2) / this.scale;
+    this.offsetX =
+      zoomPointX - (canvasCoords.x - this.game.width() / 2) / this.scale;
+    this.offsetY =
+      zoomPointY - (canvasCoords.y - this.game.height() / 2) / this.scale;
    this.clampOffsets();
    this.changed = true;
  }
@@ -147,9 +147,6 @@ export class BuildMenu extends LitElement implements Layer {
        e.x,
        e.y,
      );
-      if (clickedCell === null) {
-        return;
-      }
      if (!this.game.isValidCoord(clickedCell.x, clickedCell.y)) {
        return;
      }
@@ -147,10 +147,10 @@ export class CoordinateGridLayer implements Layer {
    canvasWidth: number,
    canvasHeight: number,
  ): string {
-    const topLeft = this.transformHandler.worldToScreenCoordinates(
+    const topLeft = this.transformHandler.worldToCanvasCoordinates(
      new Cell(0, 0),
    );
-    const bottomRight = this.transformHandler.worldToScreenCoordinates(
+    const bottomRight = this.transformHandler.worldToCanvasCoordinates(
      new Cell(width, height),
    );
    const darkMode = this.game.config().userSettings()?.darkMode() ?? false;
@@ -191,16 +191,16 @@ export class CoordinateGridLayer implements Layer {
    const canvasWidth = context.canvas.width;
    const canvasHeight = context.canvas.height;

-    const mapTopScreenRaw = this.transformHandler.worldToScreenCoordinates(
+    const mapTopScreenRaw = this.transformHandler.worldToCanvasCoordinates(
      new Cell(0, 0),
    ).y;
-    const mapBottomScreenRaw = this.transformHandler.worldToScreenCoordinates(
+    const mapBottomScreenRaw = this.transformHandler.worldToCanvasCoordinates(
      new Cell(0, height),
    ).y;
-    const mapLeftScreenRaw = this.transformHandler.worldToScreenCoordinates(
+    const mapLeftScreenRaw = this.transformHandler.worldToCanvasCoordinates(
      new Cell(0, 0),
    ).x;
-    const mapRightScreenRaw = this.transformHandler.worldToScreenCoordinates(
+    const mapRightScreenRaw = this.transformHandler.worldToCanvasCoordinates(
      new Cell(width, 0),
    ).x;

@@ -216,11 +216,11 @@ export class CoordinateGridLayer implements Layer {

    for (let col = 0; col <= fullCols; col++) {
      const worldX = col * cellWidth + mapLeftWorld;
-      const screenX = this.transformHandler.worldToScreenCoordinates(
+      const screenX = this.transformHandler.worldToCanvasCoordinates(
        new Cell(worldX, mapTopWorld),
      ).x;
      if (screenX < -1 || screenX > canvasWidth + 1) continue;
-      const screenBottom = this.transformHandler.worldToScreenCoordinates(
+      const screenBottom = this.transformHandler.worldToCanvasCoordinates(
        new Cell(worldX, gridHeight),
      ).y;
      context.moveTo(screenX, mapTopScreen);
@@ -228,13 +228,13 @@ export class CoordinateGridLayer implements Layer {
    }
    // Final vertical line at map right edge only if grid fits perfectly
    if (!hasExtraCol) {
-      const mapRightLine = this.transformHandler.worldToScreenCoordinates(
+      const mapRightLine = this.transformHandler.worldToCanvasCoordinates(
        new Cell(gridWidth, mapTopWorld),
      ).x;
      context.moveTo(mapRightLine, mapTopScreen);
      context.lineTo(
        mapRightLine,
-        this.transformHandler.worldToScreenCoordinates(
+        this.transformHandler.worldToCanvasCoordinates(
          new Cell(gridWidth, gridHeight),
        ).y,
      );
@@ -242,11 +242,11 @@ export class CoordinateGridLayer implements Layer {

    for (let row = 0; row <= fullRows; row++) {
      const worldY = row * cellHeight + mapTopWorld;
-      const screenY = this.transformHandler.worldToScreenCoordinates(
+      const screenY = this.transformHandler.worldToCanvasCoordinates(
        new Cell(mapLeftWorld, worldY),
      ).y;
      if (screenY < -1 || screenY > canvasHeight + 1) continue;
-      const screenRight = this.transformHandler.worldToScreenCoordinates(
+      const screenRight = this.transformHandler.worldToCanvasCoordinates(
        new Cell(gridWidth, worldY),
      ).x;
      context.moveTo(mapLeftScreen, screenY);
@@ -254,12 +254,12 @@ export class CoordinateGridLayer implements Layer {
    }
    // Final horizontal line at map bottom edge only if grid fits perfectly
    if (!hasExtraRow) {
-      const mapBottomLine = this.transformHandler.worldToScreenCoordinates(
+      const mapBottomLine = this.transformHandler.worldToCanvasCoordinates(
        new Cell(mapLeftWorld, gridHeight),
      ).y;
      context.moveTo(mapLeftScreen, mapBottomLine);
      context.lineTo(
-        this.transformHandler.worldToScreenCoordinates(
+        this.transformHandler.worldToCanvasCoordinates(
          new Cell(gridWidth, gridHeight),
        ).x,
        mapBottomLine,
@@ -291,7 +291,7 @@ export class CoordinateGridLayer implements Layer {
      const startY = row * cellHeight;
      const rowHeight = row < fullRows ? cellHeight : lastRowHeight;
      const centerY = startY + rowHeight / 2;
-      const screenY = this.transformHandler.worldToScreenCoordinates(
+      const screenY = this.transformHandler.worldToCanvasCoordinates(
        new Cell(0, centerY),
      ).y;
      if (screenY < -LABEL_PADDING || screenY > canvasHeight + LABEL_PADDING)
@@ -301,14 +301,14 @@ export class CoordinateGridLayer implements Layer {
        const startX = col * cellWidth;
        const colWidth = col < fullCols ? cellWidth : lastColWidth;
        const centerX = startX + colWidth / 2;
-        const screenX = this.transformHandler.worldToScreenCoordinates(
+        const screenX = this.transformHandler.worldToCanvasCoordinates(
          new Cell(centerX, centerY),
        ).x;
        if (screenX < -LABEL_PADDING || screenX > canvasWidth + LABEL_PADDING)
          continue;

        // Position at cell top-left in screen space
-        const cellTopLeft = this.transformHandler.worldToScreenCoordinates(
+        const cellTopLeft = this.transformHandler.worldToCanvasCoordinates(
          new Cell(startX, startY),
        );
        drawLabel(
@@ -106,18 +106,9 @@ export class NukeTrajectoryPreviewLayer implements Layer {
    }

    // Convert mouse position to world coordinates
-    const rect = this.transformHandler.boundingRect();
-    if (!rect) {
-      this.trajectoryPoints = [];
-      this.cachedSpawnTile = null;
-      return;
-    }
-
-    const localX = this.mousePos.x - rect.left;
-    const localY = this.mousePos.y - rect.top;
    const worldCoords = this.transformHandler.screenToWorldCoordinates(
-      localX,
-      localY,
+      this.mousePos.x,
+      this.mousePos.y,
    );

    if (!this.game.isValidCoord(worldCoords.x, worldCoords.y)) {
@@ -192,17 +183,9 @@ export class NukeTrajectoryPreviewLayer implements Layer {
    }

    // Convert mouse position to world coordinates
-    const rect = this.transformHandler.boundingRect();
-    if (!rect) {
-      this.trajectoryPoints = [];
-      return;
-    }
-
-    const localX = this.mousePos.x - rect.left;
-    const localY = this.mousePos.y - rect.top;
    const worldCoords = this.transformHandler.screenToWorldCoordinates(
-      localX,
-      localY,
+      this.mousePos.x,
+      this.mousePos.y,
    );

    if (!this.game.isValidCoord(worldCoords.x, worldCoords.y)) {
@@ -184,7 +184,7 @@ export class SpriteFactory {
    const parentContainer = new PIXI.Container();
    const tile = unit.tile();
    const worldPos = new Cell(this.game.x(tile), this.game.y(tile));
-    const screenPos = this.transformHandler.worldToScreenCoordinates(worldPos);
+    const screenPos = this.transformHandler.worldToCanvasCoordinates(worldPos);

    const isMarkedForDeletion = unit.markedForDeletion() !== false;
    const isConstruction = unit.isUnderConstruction();
@@ -265,14 +265,12 @@ export class StructureIconsLayer implements Layer {
    if (now - this.lastGhostQueryAt < 50) {
      return;
    }
-    const rect = this.transformHandler.boundingRect();
-    if (!rect) return;
-
-    const localX = this.mousePos.x - rect.left;
-    const localY = this.mousePos.y - rect.top;
    this.lastGhostQueryAt = now;
    let tileRef: TileRef | undefined;
-    const tile = this.transformHandler.screenToWorldCoordinates(localX, localY);
+    const tile = this.transformHandler.screenToWorldCoordinates(
+      this.mousePos.x,
+      this.mousePos.y,
+    );
    if (this.game.isValidCoord(tile.x, tile.y)) {
      tileRef = this.game.ref(tile.x, tile.y);
    }
@@ -457,11 +455,7 @@ export class StructureIconsLayer implements Layer {
      this.removeGhostStructure();
      return;
    }
-    const rect = this.transformHandler.boundingRect();
-    if (!rect) return;
-    const x = e.x - rect.left;
-    const y = e.y - rect.top;
-    const tile = this.transformHandler.screenToWorldCoordinates(x, y);
+    const tile = this.transformHandler.screenToWorldCoordinates(e.x, e.y);
    if (this.ghostUnit.buildableUnit.canUpgrade !== false) {
      this.eventBus.emit(
        new SendUpgradeStructureIntentEvent(
@@ -496,13 +490,9 @@ export class StructureIconsLayer implements Layer {
    this.mousePos.y = e.y;

    if (!this.ghostUnit) return;
-    const rect = this.transformHandler.boundingRect();
-    if (!rect) return;
-
-    const localX = e.x - rect.left;
-    const localY = e.y - rect.top;
-    this.ghostUnit.container.position.set(localX, localY);
-    this.ghostUnit.range?.position.set(localX, localY);
+    const local = this.transformHandler.screenToCanvasCoordinates(e.x, e.y);
+    this.ghostUnit.container.position.set(local.x, local.y);
+    this.ghostUnit.range?.position.set(local.x, local.y);
  }

  private createGhostStructure(type: PlayerBuildableUnitType | null) {
@@ -511,13 +501,14 @@ export class StructureIconsLayer implements Layer {
    if (type === null) {
      return;
    }
-    const rect = this.transformHandler.boundingRect();
-    const localX = this.mousePos.x - rect.left;
-    const localY = this.mousePos.y - rect.top;
+    const local = this.transformHandler.screenToCanvasCoordinates(
+      this.mousePos.x,
+      this.mousePos.y,
+    );
    const ghost = this.factory.createGhostContainer(
      player,
      this.ghostStage,
-      { x: localX, y: localY },
+      { x: local.x, y: local.y },
      type,
    );
    this.ghostUnit = {
@@ -749,7 +740,7 @@ export class StructureIconsLayer implements Layer {
  private computeNewLocation(render: StructureRenderInfo) {
    const tile = render.unit.tile();
    const worldPos = new Cell(this.game.x(tile), this.game.y(tile));
-    const screenPos = this.transformHandler.worldToScreenCoordinates(worldPos);
+    const screenPos = this.transformHandler.worldToCanvasCoordinates(worldPos);
    screenPos.x = Math.round(screenPos.x);

    const scale = this.transformHandler.scale;
@@ -35,7 +35,7 @@ export class MoveIndicatorUI implements UIElement {
    const chevronSize = this.chevronSize * scale;

    // Get screen coordinates
-    const screenPos = this.transformHandler.worldToScreenCoordinates(this.cell);
+    const screenPos = this.transformHandler.worldToCanvasCoordinates(this.cell);
    const centerX = screenPos.x;
    const centerY = screenPos.y;

@@ -48,7 +48,7 @@ export class Target implements UIElement {
    }
    const alpha = Math.max(0, Math.min(1, BASE_ALPHA * t));

-    const screenPos = this.transformHandler.worldToScreenCoordinates(this.cell);
+    const screenPos = this.transformHandler.worldToCanvasCoordinates(this.cell);
    screenPos.x = Math.round(screenPos.x);
    screenPos.y = Math.round(screenPos.y);
    const transformScale = this.transformHandler.scale;
@@ -46,7 +46,7 @@ export class CircleArea implements UIElement {

    const innerDiameter =
      (this.innerDiameter / 2) * (1 - t) + this.innerDiameter * t;
-    const screenPos = this.transformHandler.worldToScreenCoordinates(this.cell);
+    const screenPos = this.transformHandler.worldToCanvasCoordinates(this.cell);
    screenPos.x = Math.round(screenPos.x);
    screenPos.y = Math.round(screenPos.y);

@@ -37,7 +37,7 @@ export class TextIndicator implements UIElement {
      return true;
    }

-    const screenPos = this.transformHandler.worldToScreenCoordinates(this.cell);
+    const screenPos = this.transformHandler.worldToCanvasCoordinates(this.cell);
    screenPos.x = Math.round(screenPos.x);
    screenPos.y = Math.round(screenPos.y);