From c94c5d7acbb3fddbd4de0c405a2e3731c1d449e6 Mon Sep 17 00:00:00 2001 From: Jeroen Schweitzer Date: Sun, 5 Apr 2026 00:12:48 +0200 Subject: [PATCH] =?UTF-8?q?fix(client):=20address=20PR=20#109=20review=20?= =?UTF-8?q?=E2=80=94=206=20warnings=20+=205=20suggestions?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Star map (W1-W3, S3): - _process visibility guard + dirty flag (no redraw when hidden/unchanged) - _system_hash masked to 31-bit positive range - Extracted _find_nearest_system() shared helper game_state.gd (W4): - Inline load() in apply_snapshot() replaces per-tick overhead; safe at runtime because script is already in resource cache Data pipeline (W5-W6): - Script-relative path resolution via __file__ - --check mode + make check-star-map staleness target Minor (S1-S2, S5): - Removed redundant bone_idx assignment - Simplified double-negative test assertion - Documented autoload parse-order convention in CLAUDE.md Co-Authored-By: Claude Opus 4.6 --- CLAUDE.md | 10 ++ Makefile | 5 +- client/data/star_map_data.json | 4 +- client/scripts/autoloads/game_state.gd | 10 +- client/scripts/rendering/character_visual.gd | 1 - client/tests/test_sprint30.gd | 2 +- client/ui/star_map.gd | 54 +++++----- server/Cargo.lock | 2 +- tooling/generate-star-map-data.py | 104 +++++++++++-------- 9 files changed, 116 insertions(+), 76 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 3be167f61..bae2d468f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -84,6 +84,16 @@ The ticketing database (`settledreach.db`) lives in the **parent directory** sha - Three test tiers: (1) Live server — highest fidelity, (2) MessagePack replay via `Protocol.decode_snapshot()` — for unreachable rooms, (3) TestHarness mock — for UI-only tests where fog data doesn't matter. - `make fixtures-gauntlet` regenerates real server snapshot fixtures from the Gauntlet world. +### GDScript conventions + +**Autoload parse-order rule:** Autoload scripts (`client/scripts/autoloads/`) compile before global `class_name` scripts are registered. Referencing a `class_name` type directly in an autoload causes a parse-time "not declared" error. Pattern: +- Declare fields untyped: `var my_field = null` (comment the intended type) +- Do **not** reference `class_name` types at the top level or in `_ready()` of autoloads +- In method bodies called at runtime (e.g. `apply_snapshot`), use `load()` inline — by then the script is cached and `load()` returns the cached resource without reloading: `var CVD := load("res://scripts/rendering/character_visual_descriptor.gd")` +- Do **not** cache the `load()` result in `_ready()` — `_ready()` fires during autoload init, before the target script is in the resource cache, causing an actual file reload that breaks self-references in scripts using their own `class_name` + +`game_state.gd` (`character_visual_descriptor` field) and `sim_bridge.gd` (`harness` field) follow this pattern. + ### File conventions - Decisions: domain files in `decisions/` (see `decisions/README.md` for index) - Decision IDs: `D-NNN` (confirmed), `Q-NNN` (open questions), `R-NNN` (rejected) diff --git a/Makefile b/Makefile index a1a4b33ab..e9deea17a 100644 --- a/Makefile +++ b/Makefile @@ -7,7 +7,7 @@ GODOT := $(shell command -v godot4 2>/dev/null || command -v godot 2>/dev/null) pre-pr pre-pr-lint pre-pr-build pre-pr-test pre-pr-validate pre-pr-fixtures \ pre-pr-server pre-pr-client pre-pr-content \ fixtures-client fixtures-gauntlet golden-diff golden-update \ - checklist-validate checklist-generate \ + checklist-validate checklist-generate check-star-map \ build-sr-voice run-sr-voice test-voice-mock test-voice-real \ perf-baseline debug-schedule \ test-ipc-fixtures test-ipc-protocol test-ipc-integration test-ipc-benchmark \ @@ -347,6 +347,9 @@ checklist-validate: checklist-generate: @tooling/validate-checklist +check-star-map: + @python3 tooling/generate-star-map-data.py --check + perf-baseline: @tooling/perf-baseline diff --git a/client/data/star_map_data.json b/client/data/star_map_data.json index 42d8268fb..a8a40c261 100644 --- a/client/data/star_map_data.json +++ b/client/data/star_map_data.json @@ -1,6 +1,6 @@ { "_meta": { - "generated_from": "docs/design/star-map.json + server/data/systems.db", + "generated_from": "star-map.json + systems.db", "system_count": 301, "edge_count": 335, "note": "Client-side star map data. Regenerate with: tooling/generate-star-map-data.py" @@ -4360,4 +4360,4 @@ "GJ 71" ] ] -} \ No newline at end of file +} diff --git a/client/scripts/autoloads/game_state.gd b/client/scripts/autoloads/game_state.gd index e9998fd57..7507b4b07 100644 --- a/client/scripts/autoloads/game_state.gd +++ b/client/scripts/autoloads/game_state.gd @@ -367,10 +367,14 @@ func apply_snapshot(snapshot: Dictionary) -> void: # Server persists the descriptor and includes it in ObserverSnapshot after load. # Only update when field is present (null means no change). if snapshot.has("character_visual_descriptor") and snapshot.character_visual_descriptor is Dictionary: + # load() returns a cached script — safe to call per-tick once the resource is in cache. + # Cannot use CharacterVisualDescriptor directly: autoloads compile before global class_names + # are registered, causing a parse-time "not declared" error. var CVD := load("res://scripts/rendering/character_visual_descriptor.gd") - var restored = CVD.from_dict(snapshot.character_visual_descriptor) - if restored != null: - character_visual_descriptor = restored + if CVD != null: + var restored = CVD.from_dict(snapshot.character_visual_descriptor) + if restored != null: + character_visual_descriptor = restored # v14: player_knowledge (#264, D-041) — partial KG dump for journal panel. # Only update when field is present (null means no change, server sends when KG changes). diff --git a/client/scripts/rendering/character_visual.gd b/client/scripts/rendering/character_visual.gd index 4627fddcc..6598bb6a7 100644 --- a/client/scripts/rendering/character_visual.gd +++ b/client/scripts/rendering/character_visual.gd @@ -331,7 +331,6 @@ func _create_overhead_anchor() -> void: return _overhead_attachment = BoneAttachment3D.new() _overhead_attachment.bone_name = "Head" - _overhead_attachment.bone_idx = bone_idx _overhead_attachment.name = "OverheadAttachment" _skeleton.add_child(_overhead_attachment) diff --git a/client/tests/test_sprint30.gd b/client/tests/test_sprint30.gd index ca78695ef..4d8b509b4 100644 --- a/client/tests/test_sprint30.gd +++ b/client/tests/test_sprint30.gd @@ -230,7 +230,7 @@ func test_descriptor_has_no_hair_highlight_tint_field() -> void: assert_bool(d.has("hair_highlight_tint")).override_failure_message( "to_dict() must NOT include hair_highlight_tint — highlight is auto-derived" ).is_false() - assert_bool(desc.get("hair_highlight_tint") != null and typeof(desc.get("hair_highlight_tint")) != TYPE_NIL).override_failure_message( + assert_bool("hair_highlight_tint" in desc).override_failure_message( "CharacterVisualDescriptor must not define a hair_highlight_tint property" ).is_false() diff --git a/client/ui/star_map.gd b/client/ui/star_map.gd index 39a1071da..45721d58c 100644 --- a/client/ui/star_map.gd +++ b/client/ui/star_map.gd @@ -96,6 +96,7 @@ var _pan_start_offset: Vector2 = Vector2.ZERO var _data_loaded: bool = false var _insert_active: bool = true var _show_edges: bool = false # toggle edge display +var _dirty: bool = true # redraw needed — set by state changes, cleared after _draw func _ready() -> void: @@ -106,8 +107,11 @@ func _ready() -> void: func _process(_delta: float) -> void: - if _insert_active and _data_loaded: + if not visible: + return + if _dirty and _data_loaded: queue_redraw() + _dirty = false ## Called from main.gd when insert state changes. @@ -122,6 +126,8 @@ func set_insert_active(active: bool) -> void: ## Toggle visibility (e.g., from a keybind or button). func toggle_visible() -> void: visible = not visible + if visible: + _dirty = true ## Return the currently selected system data, or empty dict. @@ -259,8 +265,8 @@ func _sector_sort_key(node: Dictionary) -> float: ## Deterministic float in [-1, 1] from a string key. func _system_hash(key: String) -> float: - var h: int = key.hash() - return fmod(float(h) / 2147483647.0, 1.0) * 2.0 - 1.0 + var h: int = key.hash() & 0x7FFFFFFF # mask to 31-bit positive range + return float(h) / 2147483647.0 * 2.0 - 1.0 # ============================================================================= @@ -465,9 +471,15 @@ func _gui_input(event: InputEvent) -> void: _pan_start = mb.position _pan_start_offset = _pan_offset MOUSE_BUTTON_WHEEL_UP: + var old_zoom := _zoom _zoom = clampf(_zoom + ZOOM_STEP, ZOOM_MIN, ZOOM_MAX) + if _zoom != old_zoom: + _dirty = true MOUSE_BUTTON_WHEEL_DOWN: + var old_zoom := _zoom _zoom = clampf(_zoom - ZOOM_STEP, ZOOM_MIN, ZOOM_MAX) + if _zoom != old_zoom: + _dirty = true else: if mb.button_index == MOUSE_BUTTON_MIDDLE: _is_panning = false @@ -476,17 +488,17 @@ func _gui_input(event: InputEvent) -> void: var mm := event as InputEventMouseMotion if _is_panning: _pan_offset = _pan_start_offset + (mm.position - _pan_start) + _dirty = true else: _update_hover(mm.position) -func _handle_click(pos: Vector2) -> void: +## Find the system_id of the nearest node to screen position, or "" if none within HIT_RADIUS. +func _find_nearest_system(pos: Vector2) -> String: var sz: Vector2 = get_rect().size var center: Vector2 = sz * MAP_CENTER_FRACTION + _pan_offset - var best_dist: float = HIT_RADIUS var best_sid: String = "" - for node: Dictionary in _nodes: var sid: String = node.get("system_id", "") if not _node_positions.has(sid): @@ -496,30 +508,22 @@ func _handle_click(pos: Vector2) -> void: if dist < best_dist: best_dist = dist best_sid = sid + return best_sid - if best_sid != "": - _selected_system = best_sid + +func _handle_click(pos: Vector2) -> void: + var nearest := _find_nearest_system(pos) + if nearest != "": + _selected_system = nearest _show_edges = true else: _selected_system = "" _show_edges = false + _dirty = true func _update_hover(pos: Vector2) -> void: - var sz: Vector2 = get_rect().size - var center: Vector2 = sz * MAP_CENTER_FRACTION + _pan_offset - - var best_dist: float = HIT_RADIUS - var best_sid: String = "" - - for node: Dictionary in _nodes: - var sid: String = node.get("system_id", "") - if not _node_positions.has(sid): - continue - var node_pos: Vector2 = center + _node_positions[sid] * _zoom - var dist: float = pos.distance_to(node_pos) - if dist < best_dist: - best_dist = dist - best_sid = sid - - _hovered_system = best_sid + var nearest := _find_nearest_system(pos) + if nearest != _hovered_system: + _hovered_system = nearest + _dirty = true diff --git a/server/Cargo.lock b/server/Cargo.lock index b84e3f58e..b510e031b 100644 --- a/server/Cargo.lock +++ b/server/Cargo.lock @@ -1236,7 +1236,7 @@ dependencies = [ [[package]] name = "settled-reach-server" -version = "0.1.28" +version = "0.1.29" dependencies = [ "bevy_app", "bevy_ecs", diff --git a/tooling/generate-star-map-data.py b/tooling/generate-star-map-data.py index daa730135..22aa0a78e 100755 --- a/tooling/generate-star-map-data.py +++ b/tooling/generate-star-map-data.py @@ -1,8 +1,9 @@ #!/usr/bin/env python3 """Generate client/data/star_map_data.json from star-map.json + systems.db. -Run from the project root (any worktree): +Run from any directory — paths are resolved relative to this script's location: python3 tooling/generate-star-map-data.py + python3 tooling/generate-star-map-data.py --check # exit 1 if committed JSON is stale Sources: docs/design/star-map.json — graph topology (nodes + edges) @@ -16,42 +17,36 @@ import json import os import sqlite3 import sys +import tempfile + +# Resolve project root from this script's location: tooling/ is one level below root. +# Works regardless of cwd — no fragile relative path guessing. +_SCRIPT_DIR = os.path.dirname(os.path.abspath(__file__)) +_PROJECT_ROOT = os.path.dirname(_SCRIPT_DIR) + +# Worktree layout: settled-reach/{client,server,main}/ +# This script lives in client/tooling/, so _PROJECT_ROOT = client/. +# The parent of _PROJECT_ROOT is the worktree parent where sibling dirs live. +_WORKTREE_PARENT = os.path.dirname(_PROJECT_ROOT) + +STAR_MAP_PATH = os.path.join(_PROJECT_ROOT, "docs", "design", "star-map.json") +SYSTEMS_DB_PATH = os.path.join(_WORKTREE_PARENT, "server", "server", "data", "systems.db") +OUTPUT_PATH = os.path.join(_PROJECT_ROOT, "client", "data", "star_map_data.json") -def find_file(candidates: list[str]) -> str | None: - for p in candidates: - if os.path.exists(p): - return p - return None - - -def main() -> None: - # Find star-map.json - star_map_path = find_file([ - "docs/design/star-map.json", - "../docs/design/star-map.json", - "../../docs/design/star-map.json", - ]) - if not star_map_path: - print("ERROR: docs/design/star-map.json not found", file=sys.stderr) +def generate() -> dict: + """Generate the enriched star map data dict.""" + if not os.path.exists(STAR_MAP_PATH): + print(f"ERROR: star-map.json not found at {STAR_MAP_PATH}", file=sys.stderr) + sys.exit(1) + if not os.path.exists(SYSTEMS_DB_PATH): + print(f"ERROR: systems.db not found at {SYSTEMS_DB_PATH}", file=sys.stderr) sys.exit(1) - # Find systems.db - db_path = find_file([ - "server/server/data/systems.db", - "../server/server/data/systems.db", - "../../server/server/data/systems.db", - ]) - if not db_path: - print("ERROR: server/server/data/systems.db not found", file=sys.stderr) - sys.exit(1) - - # Load star map topology - with open(star_map_path) as f: + with open(STAR_MAP_PATH) as f: star_map = json.load(f) - # Load DB data - conn = sqlite3.connect(db_path) + conn = sqlite3.connect(SYSTEMS_DB_PATH) conn.row_factory = sqlite3.Row cur = conn.cursor() cur.execute( @@ -61,7 +56,6 @@ def main() -> None: db_lookup = {row["system_id"]: dict(row) for row in cur.fetchall()} conn.close() - # Merge nodes = [] for n in star_map["nodes"]: sid = n["system_id"] @@ -82,9 +76,9 @@ def main() -> None: nodes.sort(key=lambda x: (x["hop_distance"], x["system_id"])) - output = { + return { "_meta": { - "generated_from": f"{star_map_path} + {db_path}", + "generated_from": "star-map.json + systems.db", "system_count": len(nodes), "edge_count": len(star_map["edges"]), "note": "Client-side star map data. Regenerate with: tooling/generate-star-map-data.py", @@ -93,15 +87,41 @@ def main() -> None: "edges": star_map["edges"], } - # Write output - out_path = find_file(["client/data"]) or "client/data" - os.makedirs(out_path, exist_ok=True) - out_file = os.path.join(out_path, "star_map_data.json") - with open(out_file, "w") as f: - json.dump(output, f, indent=2, ensure_ascii=False) - print(f"Generated {out_file}") - print(f" Nodes: {len(nodes)}, Edges: {len(star_map['edges'])}") +def write_output(data: dict, path: str) -> None: + os.makedirs(os.path.dirname(path), exist_ok=True) + with open(path, "w") as f: + json.dump(data, f, indent=2, ensure_ascii=False) + f.write("\n") + + +def main() -> None: + check_mode = "--check" in sys.argv + + data = generate() + + if check_mode: + # Generate to temp file and compare against committed JSON + if not os.path.exists(OUTPUT_PATH): + print(f"STALE: {OUTPUT_PATH} does not exist — run without --check to generate") + sys.exit(1) + with tempfile.NamedTemporaryFile(mode="w", suffix=".json", delete=False) as tmp: + json.dump(data, tmp, indent=2, ensure_ascii=False) + tmp.write("\n") + tmp_path = tmp.name + try: + with open(tmp_path) as a, open(OUTPUT_PATH) as b: + if a.read() != b.read(): + print(f"STALE: {OUTPUT_PATH} differs from generated output") + print("Run: python3 tooling/generate-star-map-data.py") + sys.exit(1) + print(f"OK: {OUTPUT_PATH} is up to date") + finally: + os.unlink(tmp_path) + else: + write_output(data, OUTPUT_PATH) + print(f"Generated {OUTPUT_PATH}") + print(f" Nodes: {data['_meta']['system_count']}, Edges: {data['_meta']['edge_count']}") if __name__ == "__main__":