fix(client): address PR #109 review — 6 warnings + 5 suggestions
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 <noreply@anthropic.com>
This commit is contained in:
Claude Opus 4.6
@@ -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)
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
@@ -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"
|
||||
]
|
||||
]
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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).
|
||||
|
||||
@@ -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)
|
||||
|
||||
|
||||
@@ -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()
|
||||
|
||||
|
||||
+29
-25
@@ -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
|
||||
|
||||
Generated
+1
-1
@@ -1236,7 +1236,7 @@ dependencies = [
|
||||
|
||||
[[package]]
|
||||
name = "settled-reach-server"
|
||||
version = "0.1.28"
|
||||
version = "0.1.29"
|
||||
dependencies = [
|
||||
"bevy_app",
|
||||
"bevy_ecs",
|
||||
|
||||
@@ -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__":
|
||||
|
||||
Reference in New Issue
Block a user