fix(client): address PR review — invalidation, constants, tests

Review feedback from Hoshe + Tyre on PR #8:

- Extract shared TILE_SIZE to Constants class_name (Tyre #5, Hoshe #7)
- Fix tile invalidation: detect tile/visibility count changes instead
  of one-shot dirty flag, supports chunk loading (Tyre #3, #4)
- Fog dirty tracking: only re-render when visible_positions changes
- Add bounds warning for unknown tile types (Hoshe #1)
- Add player-not-found warning in GameState (Hoshe #5)
- Use Image.fill_rect() instead of pixel loops (Tyre #10)
- Document _player_pos as reserved for fog decay #113 (Tyre #6)
- Add TODO(#130) for D-033 relationship colors (Hoshe #3, Tyre #8)
- Add class_name to EntityRenderer, FogRenderer, TileRenderer
- 20 new rendering tests (D-030 Layer 1): entity lifecycle, fog
  registration, tile type mapping, snapshot completeness, constants
- 65 total tests passing, 0 failures

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

client/scripts/autoloads/game_state.gd

@@ -23,10 +23,15 @@ func apply_snapshot(snapshot: Dictionary) -> void:
 	if snapshot.has("entities"):
 		visible_entities = snapshot.entities
 		# Derive player position from the player entity
+		var found_player := false
 		for entity in visible_entities:
 			if entity.has("entity_id") and entity.entity_id == player_entity_id:
 				player_position = Vector2(entity.x, entity.y)
+				found_player = true
 				break
+		if not found_player and visible_entities.size() > 0:
+			push_warning("GameState: player entity_id %d not found in %d entities" % [
+				player_entity_id, visible_entities.size()])
 
 	if snapshot.has("tiles"):
 		visible_tiles = snapshot.tiles

client/scripts/constants.gd

@@ -0,0 +1,6 @@
+class_name Constants
+
+# Shared constants used across rendering, game state, and protocol layers.
+
+## Tile size in pixels — all renderers and coordinate conversions use this.
+const TILE_SIZE: int = 32

client/scripts/main.gd

@@ -23,7 +23,7 @@ func _process(_delta: float) -> void:
 
 	# Track camera to player position every frame (D-015: locked, no panning)
 	# Camera2D smoothing handles interpolation — we just set the target
-	camera.global_position = GameState.player_position * 32.0
+	camera.global_position = GameState.player_position * Constants.TILE_SIZE
 
 	# Send queued input to simulation
 	var inputs = InputMapper.flush_queue()

client/scripts/rendering/entity_renderer.gd

@@ -1,10 +1,11 @@
+class_name EntityRenderer
 extends Node2D
 
 # Entity renderer — manages entity sprites under the Entities node
 # Creates/updates/removes ColorRect children based on entity data
 # Entity format (from Protocol): {entity_id, x, y, z, kind: {variant, data}}
 
-const TILE_SIZE: int = 32
+const TILE_SIZE: int = Constants.TILE_SIZE
 const ENTITY_SIZE: int = 24
 const ENTITY_OFFSET: float = (TILE_SIZE - ENTITY_SIZE) / 2.0  # center within tile
 
@@ -47,13 +48,14 @@ func _create_entity_node(entity_id: int, entity_data: Dictionary) -> void:
 	entity_node.size = Vector2(ENTITY_SIZE, ENTITY_SIZE)
 	entity_node.pivot_offset = Vector2(ENTITY_SIZE / 2.0, ENTITY_SIZE / 2.0)
 
-	# Color based on entity kind (from Protocol decoded format)
+	# Color based on entity kind (from Protocol decoded format).
+	# TODO(#130): replace with D-033 relationship colors (teal/green/amber/red).
 	var kind_variant: String = entity_data.get("kind", {}).get("variant", "")
 	match kind_variant:
 		"Npc":
-			entity_node.color = Color(0.3, 0.6, 0.9)  # Blue for NPCs
+			entity_node.color = Color(0.3, 0.6, 0.9)  # Placeholder blue
 		"Player":
-			entity_node.color = Color(0.88, 0.91, 1.0)  # Light blue for player
+			entity_node.color = Color(0.88, 0.91, 1.0)  # Placeholder light blue
 		_:
 			entity_node.color = Color(0.8, 0.8, 0.8)  # Gray for unknown
 

client/scripts/rendering/fog_renderer.gd

@@ -1,3 +1,4 @@
+class_name FogRenderer
 extends TileMapLayer
 
 # Fog renderer — draws fog overlay on non-visible tiles (D-011)
@@ -9,8 +10,12 @@ extends TileMapLayer
 # Atlas layout:
 #   (0,0) = full fog (opaque black)
 #   (1,0) = fog edge (semi-transparent)
+#
+# Note: fog-edge uses 8-directional neighbors for visual smoothness.
+# Actual visibility boundaries come from the server's shadowcasting (D-011).
+# Fog-returns-over-time (D-011 decay) is tracked in #113, not here.
 
-const TILE_SIZE: int = 32
+const TILE_SIZE: int = Constants.TILE_SIZE
 
 var _initialized: bool = false
 var _all_tile_positions: Dictionary = {}  # Vector2i -> true, all known map tiles
@@ -28,9 +33,9 @@ func _setup_tileset() -> void:
 	var img := Image.create(TILE_SIZE * 2, TILE_SIZE, false, Image.FORMAT_RGBA8)
 
 	# Full fog (0,0) — opaque black
-	_fill_tile(img, 0, Color(0.02, 0.02, 0.05, 1.0))
+	img.fill_rect(Rect2i(0, 0, TILE_SIZE, TILE_SIZE), Color(0.02, 0.02, 0.05, 1.0))
 	# Fog edge (1,0) — semi-transparent dark
-	_fill_tile(img, 1, Color(0.02, 0.02, 0.05, 0.6))
+	img.fill_rect(Rect2i(TILE_SIZE, 0, TILE_SIZE, TILE_SIZE), Color(0.02, 0.02, 0.05, 0.6))
 
 	var tex := ImageTexture.create_from_image(img)
 	source.texture = tex
@@ -49,8 +54,8 @@ func register_tile_positions(tiles: Array) -> void:
 		if tile_data.has("x") and tile_data.has("y"):
 			_all_tile_positions[Vector2i(tile_data.x, tile_data.y)] = true
 
-# Update fog based on visible positions
+# Update fog based on visible positions.
-# visible_positions: Dictionary of Vector2i -> true
+# player_pos reserved for future fog-decay tracking (#113).
 func update_fog(visible_positions: Dictionary, _player_pos: Vector2) -> void:
 	if not _initialized:
 		return
@@ -60,7 +65,7 @@ func update_fog(visible_positions: Dictionary, _player_pos: Vector2) -> void:
 	if _all_tile_positions.is_empty() or visible_positions.is_empty():
 		return
 
-	# Build set of fog-edge positions (adjacent to visible but not visible themselves)
+	# Build set of fog-edge positions (8-directional neighbors of visible tiles)
 	var fog_edge: Dictionary = {}
 	var neighbors := [
 		Vector2i(-1, 0), Vector2i(1, 0), Vector2i(0, -1), Vector2i(0, 1),
@@ -81,9 +86,3 @@ func update_fog(visible_positions: Dictionary, _player_pos: Vector2) -> void:
 			set_cell(pos, 0, Vector2i(1, 0))  # Fog edge — semi-transparent
 		else:
 			set_cell(pos, 0, Vector2i(0, 0))  # Full fog — opaque
-
-func _fill_tile(img: Image, tile_index: int, color: Color) -> void:
-	var x_offset := tile_index * TILE_SIZE
-	for x in range(TILE_SIZE):
-		for y in range(TILE_SIZE):
-			img.set_pixel(x_offset + x, y, color)

client/scripts/rendering/tile_renderer.gd

@@ -1,3 +1,4 @@
+class_name TileRenderer
 extends TileMapLayer
 
 # Tile renderer — draws map tiles from ObserverSnapshot tile data
@@ -9,7 +10,7 @@ extends TileMapLayer
 #   (2,0) = door     — brown
 #   (3,0) = object   — teal
 
-const TILE_SIZE: int = 32
+const TILE_SIZE: int = Constants.TILE_SIZE
 
 enum TileType { FLOOR = 0, WALL = 1, DOOR = 2, OBJECT = 3 }
 
@@ -71,6 +72,8 @@ func update_tiles(tiles: Array) -> void:
 
 		var tile_type_str: String = tile_data.type
 		if not TILE_TYPE_MAP.has(tile_type_str):
+			push_warning("TileRenderer: unknown tile type '%s' at (%d, %d)" % [
+				tile_type_str, tile_data.x, tile_data.y])
 			continue
 
 		var atlas_x: int = TILE_TYPE_MAP[tile_type_str]
@@ -79,17 +82,13 @@ func update_tiles(tiles: Array) -> void:
 
 # Fill a tile region with a solid color
 func _fill_tile(img: Image, tile_index: int, color: Color) -> void:
-	var x_offset := tile_index * TILE_SIZE
+	var rect := Rect2i(tile_index * TILE_SIZE, 0, TILE_SIZE, TILE_SIZE)
-	for x in range(TILE_SIZE):
+	img.fill_rect(rect, color)
-		for y in range(TILE_SIZE):
-			img.set_pixel(x_offset + x, y, color)
 
 # Fill a tile region with a color and a 1px border
 func _fill_tile_with_border(img: Image, tile_index: int, fill: Color, border: Color) -> void:
 	var x_offset := tile_index * TILE_SIZE
-	for x in range(TILE_SIZE):
+	# Border (full tile)
-		for y in range(TILE_SIZE):
+	img.fill_rect(Rect2i(x_offset, 0, TILE_SIZE, TILE_SIZE), border)
-			if x == 0 or y == 0 or x == TILE_SIZE - 1 or y == TILE_SIZE - 1:
+	# Fill (inset by 1px)
-				img.set_pixel(x_offset + x, y, border)
+	img.fill_rect(Rect2i(x_offset + 1, 1, TILE_SIZE - 2, TILE_SIZE - 2), fill)
-			else:
-				img.set_pixel(x_offset + x, y, fill)

client/scripts/rendering/world_renderer.gd

@@ -8,30 +8,31 @@ extends Node2D
 @onready var fog_renderer = $FogOverlay
 @onready var entity_renderer = $Entities
 
-var _tiles_dirty: bool = true
+var _last_tile_count: int = 0
+var _last_visible_count: int = 0
 
 func _ready() -> void:
 	print("WorldRenderer: Initialized")
 
 # Called each frame to update visuals from game state
 func update_from_state() -> void:
-	# Update tiles (only when tile data changes)
+	var tile_count := GameState.visible_tiles.size()
+	var visible_count := GameState.visible_positions.size()
+
+	# Update tiles when tile data changes (new chunks loaded, D-012)
 	if tile_renderer and tile_renderer.has_method("update_tiles"):
-		if _tiles_dirty and GameState.visible_tiles.size() > 0:
+		if tile_count > 0 and tile_count != _last_tile_count:
 			tile_renderer.update_tiles(GameState.visible_tiles)
-			# Register tile positions with fog renderer for coverage
 			if fog_renderer and fog_renderer.has_method("register_tile_positions"):
 				fog_renderer.register_tile_positions(GameState.visible_tiles)
-			_tiles_dirty = false
+			_last_tile_count = tile_count
 
-	# Update fog overlay
+	# Update fog overlay when visibility changes
 	if fog_renderer and fog_renderer.has_method("update_fog"):
-		fog_renderer.update_fog(GameState.visible_positions, GameState.player_position)
+		if visible_count != _last_visible_count:
+			fog_renderer.update_fog(GameState.visible_positions, GameState.player_position)
+			_last_visible_count = visible_count
 
 	# Update entity sprites
 	if entity_renderer and entity_renderer.has_method("update_entities"):
 		entity_renderer.update_entities(GameState.visible_entities)
-
-# Mark tiles as needing re-render (call when tile data changes significantly)
-func invalidate_tiles() -> void:
-	_tiles_dirty = true

client/tests/test_rendering.gd

@@ -0,0 +1,216 @@
+## D-030 Layer 1: Tests for rendering components (tile, fog, entity renderers)
+## Validates renderers handle snapshot data correctly.
+class_name TestRendering
+extends GdUnitTestSuite
+
+var EntityRendererScript = load("res://scripts/rendering/entity_renderer.gd")
+var FogRendererScript = load("res://scripts/rendering/fog_renderer.gd")
+var TileRendererScript = load("res://scripts/rendering/tile_renderer.gd")
+
+# -- Test data matching Protocol decoded format --
+
+var _test_tiles: Array = [
+	{"x": 0, "y": 0, "z": 0, "type": "floor"},
+	{"x": 1, "y": 0, "z": 0, "type": "wall"},
+	{"x": 2, "y": 0, "z": 0, "type": "door"},
+	{"x": 3, "y": 0, "z": 0, "type": "object"},
+	{"x": 0, "y": 1, "z": 0, "type": "floor"},
+]
+
+var _test_entities: Array = [
+	{"entity_id": 1, "x": 5.0, "y": 5.0, "z": 0, "kind": {"variant": "Player", "data": null}},
+	{"entity_id": 2, "x": 7.0, "y": 5.0, "z": 0, "kind": {"variant": "Npc", "data": null}},
+]
+
+
+# -- Constants --
+
+func test_tile_size_constant() -> void:
+	assert_that(Constants.TILE_SIZE).is_equal(32)
+
+
+# -- GameState: tile and visibility data --
+
+func test_game_state_stores_tiles() -> void:
+	GameState.apply_snapshot({"tick": 1, "tiles": _test_tiles})
+	assert_that(GameState.visible_tiles.size()).is_equal(5)
+
+func test_game_state_stores_visible_positions() -> void:
+	var positions := [{"x": 5, "y": 5}, {"x": 6, "y": 5}]
+	GameState.apply_snapshot({"tick": 1, "visible_positions": positions})
+	assert_that(GameState.visible_positions.size()).is_equal(2)
+	assert_that(GameState.visible_positions.has(Vector2i(5, 5))).is_true()
+	assert_that(GameState.visible_positions.has(Vector2i(6, 5))).is_true()
+
+func test_game_state_clears_old_visible_positions() -> void:
+	GameState.apply_snapshot({"tick": 1, "visible_positions": [{"x": 1, "y": 1}]})
+	assert_that(GameState.visible_positions.size()).is_equal(1)
+	GameState.apply_snapshot({"tick": 2, "visible_positions": [{"x": 2, "y": 2}, {"x": 3, "y": 3}]})
+	assert_that(GameState.visible_positions.size()).is_equal(2)
+	assert_that(GameState.visible_positions.has(Vector2i(1, 1))).is_false()
+
+func test_game_state_warns_on_missing_player() -> void:
+	GameState.player_entity_id = 999
+	GameState.player_position = Vector2(5, 5)
+	GameState.apply_snapshot({"tick": 1, "entities": _test_entities})
+	assert_that(GameState.player_position).is_equal(Vector2(5, 5))
+
+
+# -- SimBridge: test data completeness --
+
+func test_sim_bridge_test_snapshot_has_tiles() -> void:
+	SimBridge._test_tick = 0
+	var snap = SimBridge._test_snapshot()
+	assert_that(snap.has("tiles")).is_true()
+	assert_that(snap.tiles.size()).is_greater(0)
+	var tile = snap.tiles[0]
+	assert_that(tile.has("x")).is_true()
+	assert_that(tile.has("y")).is_true()
+	assert_that(tile.has("type")).is_true()
+
+func test_sim_bridge_test_snapshot_has_visible_positions() -> void:
+	SimBridge._test_tick = 0
+	var snap = SimBridge._test_snapshot()
+	assert_that(snap.has("visible_positions")).is_true()
+	assert_that(snap.visible_positions.size()).is_greater(0)
+	var pos = snap.visible_positions[0]
+	assert_that(pos.has("x")).is_true()
+	assert_that(pos.has("y")).is_true()
+
+func test_sim_bridge_test_snapshot_has_player_entity() -> void:
+	SimBridge._test_tick = 0
+	var snap = SimBridge._test_snapshot()
+	var has_player := false
+	for entity in snap.entities:
+		if entity.kind.variant == "Player":
+			has_player = true
+			break
+	assert_that(has_player).is_true()
+
+func test_sim_bridge_test_snapshot_has_npc() -> void:
+	SimBridge._test_tick = 0
+	var snap = SimBridge._test_snapshot()
+	var has_npc := false
+	for entity in snap.entities:
+		if entity.kind.variant == "Npc":
+			has_npc = true
+			break
+	assert_that(has_npc).is_true()
+
+func test_sim_bridge_test_tiles_contain_all_types() -> void:
+	SimBridge._test_tick = 0
+	var snap = SimBridge._test_snapshot()
+	var types: Dictionary = {}
+	for tile in snap.tiles:
+		types[tile.type] = true
+	assert_that(types.has("floor")).is_true()
+	assert_that(types.has("wall")).is_true()
+	assert_that(types.has("door")).is_true()
+
+
+# -- EntityRenderer: lifecycle --
+
+func _make_entity_renderer() -> Node2D:
+	var renderer = Node2D.new()
+	renderer.set_script(EntityRendererScript)
+	add_child(renderer)
+	return renderer
+
+func test_entity_renderer_creates_nodes() -> void:
+	var renderer := _make_entity_renderer()
+	renderer.update_entities(_test_entities)
+
+	assert_that(renderer.entity_nodes.size()).is_equal(2)
+	assert_that(renderer.entity_nodes.has(1)).is_true()
+	assert_that(renderer.entity_nodes.has(2)).is_true()
+	renderer.queue_free()
+
+func test_entity_renderer_removes_stale_entities() -> void:
+	var renderer := _make_entity_renderer()
+	renderer.update_entities(_test_entities)
+	assert_that(renderer.entity_nodes.size()).is_equal(2)
+
+	renderer.update_entities([_test_entities[0]])
+	assert_that(renderer.entity_nodes.size()).is_equal(1)
+	assert_that(renderer.entity_nodes.has(1)).is_true()
+	renderer.queue_free()
+
+func test_entity_renderer_positions_centered() -> void:
+	var renderer := _make_entity_renderer()
+	renderer.update_entities([_test_entities[0]])
+
+	var node = renderer.entity_nodes[1]
+	var offset: float = (Constants.TILE_SIZE - 24) / 2.0
+	var expected_x: float = 5.0 * Constants.TILE_SIZE + offset
+	var expected_y: float = 5.0 * Constants.TILE_SIZE + offset
+	assert_that(node.position.x).is_equal_approx(expected_x, 0.01)
+	assert_that(node.position.y).is_equal_approx(expected_y, 0.01)
+	renderer.queue_free()
+
+func test_entity_renderer_skips_missing_entity_id() -> void:
+	var renderer := _make_entity_renderer()
+	renderer.update_entities([{"x": 1.0, "y": 1.0, "z": 0, "kind": {"variant": "Npc", "data": null}}])
+	assert_that(renderer.entity_nodes.size()).is_equal(0)
+	renderer.queue_free()
+
+func test_entity_renderer_player_color_differs_from_npc() -> void:
+	var renderer := _make_entity_renderer()
+	renderer.update_entities(_test_entities)
+
+	var player_node = renderer.entity_nodes[1] as ColorRect
+	var npc_node = renderer.entity_nodes[2] as ColorRect
+	assert_that(player_node.color != npc_node.color).is_true()
+	renderer.queue_free()
+
+func test_entity_renderer_empty_entities_clears_all() -> void:
+	var renderer := _make_entity_renderer()
+	renderer.update_entities(_test_entities)
+	assert_that(renderer.entity_nodes.size()).is_equal(2)
+
+	renderer.update_entities([])
+	assert_that(renderer.entity_nodes.size()).is_equal(0)
+	renderer.queue_free()
+
+
+# -- FogRenderer: position registration --
+
+func _make_fog_renderer() -> TileMapLayer:
+	var fog = TileMapLayer.new()
+	fog.set_script(FogRendererScript)
+	return fog
+
+func test_fog_renderer_registers_positions() -> void:
+	var fog := _make_fog_renderer()
+	fog.register_tile_positions(_test_tiles)
+	assert_that(fog._all_tile_positions.size()).is_equal(5)
+	assert_that(fog._all_tile_positions.has(Vector2i(0, 0))).is_true()
+	assert_that(fog._all_tile_positions.has(Vector2i(3, 0))).is_true()
+	assert_that(fog._all_tile_positions.has(Vector2i(0, 1))).is_true()
+	fog.free()
+
+func test_fog_renderer_clears_on_re_register() -> void:
+	var fog := _make_fog_renderer()
+	fog.register_tile_positions(_test_tiles)
+	assert_that(fog._all_tile_positions.size()).is_equal(5)
+
+	fog.register_tile_positions([{"x": 10, "y": 10, "z": 0, "type": "floor"}])
+	assert_that(fog._all_tile_positions.size()).is_equal(1)
+	assert_that(fog._all_tile_positions.has(Vector2i(0, 0))).is_false()
+	fog.free()
+
+func test_fog_renderer_handles_empty_data() -> void:
+	var fog := _make_fog_renderer()
+	fog._initialized = true
+	fog.update_fog({}, Vector2.ZERO)
+	fog.register_tile_positions([])
+	assert_that(fog._all_tile_positions.size()).is_equal(0)
+	fog.free()
+
+
+# -- TileRenderer: tile type constants --
+
+func test_tile_type_map_covers_required_types() -> void:
+	var tile_script = TileRendererScript
+	var required := ["floor", "wall", "door", "object"]
+	for tile_type in required:
+		assert_that(tile_script.TILE_TYPE_MAP.has(tile_type)).is_true()