From fd0b6d3db898dec1344043d4803fbdeb608a8ce5 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Fri, 29 May 2026 21:07:02 -0400 Subject: [PATCH] =?UTF-8?q?NEO-97:=20address=20code=20review=20=E2=80=94?= =?UTF-8?q?=20helpers,=20tests,=20doc=20fixes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extract npc_combat_hud_helpers.gd for shared HUD formatters; hide telegraph at zero windup; surface runtime sync errors; npc_row last-snapshot fallback; expand GdUnit coverage; align E5.M2 module docs and strike review findings. --- client/scripts/main.gd | 105 ++-------- client/scripts/npc_combat_hud_helpers.gd | 133 +++++++++++++ client/scripts/npc_combat_hud_helpers.gd.uid | 1 + client/scripts/npc_runtime_client.gd | 11 ++ client/test/npc_combat_hud_refresh_test.gd | 179 ++++++++++-------- client/test/npc_runtime_client_test.gd | 14 ++ .../modules/E5_M2_NpcAiAndBehaviorProfiles.md | 4 +- docs/plans/NEO-97-implementation-plan.md | 21 +- docs/reviews/2026-05-29-NEO-97.md | 20 +- 9 files changed, 305 insertions(+), 183 deletions(-) create mode 100644 client/scripts/npc_combat_hud_helpers.gd create mode 100644 client/scripts/npc_combat_hud_helpers.gd.uid diff --git a/client/scripts/main.gd b/client/scripts/main.gd index e8389d0..c7da57f 100644 --- a/client/scripts/main.gd +++ b/client/scripts/main.gd @@ -45,6 +45,7 @@ const CooldownStateScript := preload("res://scripts/cooldown_state.gd") const NpcRuntimeClientScript := preload("res://scripts/npc_runtime_client.gd") const PlayerCombatHealthClientScript := preload("res://scripts/player_combat_health_client.gd") const NpcRuntimeHudStateScript := preload("res://scripts/npc_runtime_hud_state.gd") +const NpcCombatHudHelpers := preload("res://scripts/npc_combat_hud_helpers.gd") const COMBAT_POLL_INTERVAL_SEC: float = 1.0 const COOLDOWN_REASON_ON_CD := "on_cooldown" const DEV_BOOTSTRAP_CAST_ABILITY_ID := "prototype_pulse" @@ -87,6 +88,7 @@ var _npc_runtime_client: Node = null var _player_combat_health_client: Node = null var _npc_runtime_hud_state: RefCounted = null var _npc_runtime_snapshot: Dictionary = {} +var _npc_runtime_sync_error: String = "" var _player_combat_health_snapshot: Dictionary = {} var _player_combat_health_error: String = "" var _cooldown_state: RefCounted = null @@ -1066,35 +1068,16 @@ func _dev_player_id() -> String: return "dev-local-1" -func _is_known_npc_id(target_id: String) -> bool: - return PrototypeTargetConstants.ANCHORS.has(target_id.strip_edges()) - - func _is_combat_active() -> bool: - var lock_id := _locked_target_id_from_state(_last_target_state) - if _is_known_npc_id(lock_id): - return true - return not _incoming_aggro_row().is_empty() - - -func _incoming_aggro_row() -> Dictionary: - if _npc_runtime_snapshot.is_empty(): - return {} - return NpcRuntimeClientScript.row_for_aggro_holder(_dev_player_id(), _npc_runtime_snapshot) + return NpcCombatHudHelpers.is_combat_active( + _last_target_state, _npc_runtime_snapshot, _dev_player_id() + ) func _resolve_hud_npc_rows() -> Dictionary: - var lock_id := _locked_target_id_from_state(_last_target_state) - var locked_row: Dictionary = {} - if _is_known_npc_id(lock_id): - locked_row = NpcRuntimeClientScript.npc_row(lock_id, _npc_runtime_snapshot) - var incoming_row: Dictionary = {} - var aggro_row: Dictionary = _incoming_aggro_row() - if not aggro_row.is_empty(): - var aggro_id: String = str(aggro_row.get("npcInstanceId", "")).strip_edges() - if aggro_id != lock_id: - incoming_row = aggro_row - return {"locked": locked_row, "incoming": incoming_row} + return NpcCombatHudHelpers.resolve_hud_npc_rows( + _last_target_state, _npc_runtime_snapshot, _dev_player_id() + ) func _request_npc_combat_sync() -> void: @@ -1126,6 +1109,7 @@ func _on_combat_poll_timeout() -> void: func _on_npc_runtime_received(snapshot: Dictionary) -> void: + _npc_runtime_sync_error = "" _npc_runtime_snapshot = snapshot.duplicate(true) if _npc_runtime_hud_state != null and _npc_runtime_hud_state.has_method("apply_snapshot"): _npc_runtime_hud_state.call("apply_snapshot", _npc_runtime_snapshot) @@ -1133,7 +1117,8 @@ func _on_npc_runtime_received(snapshot: Dictionary) -> void: _render_npc_combat_hud_labels() -func _on_npc_runtime_sync_failed(_reason: String) -> void: +func _on_npc_runtime_sync_failed(reason: String) -> void: + _npc_runtime_sync_error = reason _render_npc_combat_hud_labels() @@ -1171,75 +1156,25 @@ func _render_npc_combat_hud_labels() -> void: func _has_interpolated_telegraph_display() -> bool: - if _npc_runtime_hud_state == null: - return false - var rows: Dictionary = _resolve_hud_npc_rows() - for key: String in ["locked", "incoming"]: - var row: Dictionary = rows.get(key, {}) as Dictionary - if row.is_empty(): - continue - var instance_id: String = str(row.get("npcInstanceId", "")).strip_edges() - if instance_id.is_empty(): - continue - var telegraph: Variant = row.get("activeTelegraph", null) - if telegraph == null or not telegraph is Dictionary: - continue - if _npc_runtime_hud_state.has_method("has_active_windup"): - if bool(_npc_runtime_hud_state.call("has_active_windup", instance_id)): - return true - return false + return NpcCombatHudHelpers.has_interpolated_telegraph_display( + _resolve_hud_npc_rows(), _npc_runtime_hud_state + ) func _render_npc_state_label() -> void: if not is_instance_valid(_npc_state_label): return - var rows: Dictionary = _resolve_hud_npc_rows() - var lines: PackedStringArray = PackedStringArray() - var locked_row: Dictionary = rows.get("locked", {}) as Dictionary - if not locked_row.is_empty(): - var locked_id: String = str(locked_row.get("npcInstanceId", "")) - var locked_name: String = PrototypeTargetConstants.display_short_name(locked_id) - var locked_state: String = str(locked_row.get("state", "unknown")) - lines.append("NPC state: %s → %s" % [locked_name, locked_state]) - var incoming_row: Dictionary = rows.get("incoming", {}) as Dictionary - if not incoming_row.is_empty(): - var incoming_id: String = str(incoming_row.get("npcInstanceId", "")) - var incoming_name: String = PrototypeTargetConstants.display_short_name(incoming_id) - var incoming_state: String = str(incoming_row.get("state", "unknown")) - lines.append("Incoming: %s → %s" % [incoming_name, incoming_state]) - if lines.is_empty(): - _npc_state_label.text = "NPC state: —" - return - _npc_state_label.text = "\n".join(lines) - - -func _format_telegraph_line(row: Dictionary) -> String: - var instance_id: String = str(row.get("npcInstanceId", "")).strip_edges() - var telegraph: Variant = row.get("activeTelegraph", null) - if telegraph == null or not telegraph is Dictionary: - return "" - var telegraph_dict: Dictionary = telegraph as Dictionary - var short_name: String = PrototypeTargetConstants.display_short_name(instance_id) - var telegraph_id: String = str(telegraph_dict.get("telegraphId", "")) - var archetype: String = str(telegraph_dict.get("archetypeKind", "")) - var remaining: float = float(telegraph_dict.get("windupRemainingSeconds", 0.0)) - if _npc_runtime_hud_state != null and _npc_runtime_hud_state.has_method("display_windup_remaining"): - remaining = float(_npc_runtime_hud_state.call("display_windup_remaining", instance_id)) - return "Telegraph: %s %s · %.1fs (%s)" % [short_name, telegraph_id, remaining, archetype] + _npc_state_label.text = NpcCombatHudHelpers.build_npc_state_label( + _resolve_hud_npc_rows(), _npc_runtime_sync_error + ) func _render_telegraph_label() -> void: if not is_instance_valid(_telegraph_label): return - var rows: Dictionary = _resolve_hud_npc_rows() - var locked_row: Dictionary = rows.get("locked", {}) as Dictionary - var line: String = _format_telegraph_line(locked_row) - if line.is_empty(): - line = _format_telegraph_line(rows.get("incoming", {}) as Dictionary) - if line.is_empty(): - _telegraph_label.text = "Telegraph: —" - return - _telegraph_label.text = line + _telegraph_label.text = NpcCombatHudHelpers.build_telegraph_label( + _resolve_hud_npc_rows(), _npc_runtime_hud_state, _npc_runtime_sync_error + ) func _on_move_rejected(reason_code: String) -> void: diff --git a/client/scripts/npc_combat_hud_helpers.gd b/client/scripts/npc_combat_hud_helpers.gd new file mode 100644 index 0000000..1d66ebb --- /dev/null +++ b/client/scripts/npc_combat_hud_helpers.gd @@ -0,0 +1,133 @@ +extends RefCounted + +## NEO-97: shared combat-active gate + NPC/telegraph HUD copy for [code]main.gd[/code] and tests. + +const PrototypeTargetConstants := preload("res://scripts/prototype_target_constants.gd") +const NpcRuntimeClient := preload("res://scripts/npc_runtime_client.gd") + + +static func locked_target_id_from_state(state: Dictionary) -> String: + var locked_variant: Variant = state.get("lockedTargetId", null) + if locked_variant is String: + return (locked_variant as String).strip_edges() + return "" + + +static func is_known_npc_id(target_id: String) -> bool: + return PrototypeTargetConstants.ANCHORS.has(target_id.strip_edges()) + + +static func incoming_aggro_row(dev_player_id: String, npc_runtime_snapshot: Dictionary) -> Dictionary: + if npc_runtime_snapshot.is_empty(): + return {} + return NpcRuntimeClient.row_for_aggro_holder(dev_player_id, npc_runtime_snapshot) + + +static func is_combat_active( + last_target_state: Dictionary, npc_runtime_snapshot: Dictionary, dev_player_id: String +) -> bool: + var lock_id := locked_target_id_from_state(last_target_state) + if is_known_npc_id(lock_id): + return true + return not incoming_aggro_row(dev_player_id, npc_runtime_snapshot).is_empty() + + +static func resolve_hud_npc_rows( + last_target_state: Dictionary, npc_runtime_snapshot: Dictionary, dev_player_id: String +) -> Dictionary: + var lock_id := locked_target_id_from_state(last_target_state) + var locked_row: Dictionary = {} + if is_known_npc_id(lock_id): + locked_row = NpcRuntimeClient.npc_row(lock_id, npc_runtime_snapshot) + var incoming_row: Dictionary = {} + var aggro_row: Dictionary = incoming_aggro_row(dev_player_id, npc_runtime_snapshot) + if not aggro_row.is_empty(): + var aggro_id: String = str(aggro_row.get("npcInstanceId", "")).strip_edges() + if aggro_id != lock_id: + incoming_row = aggro_row + return {"locked": locked_row, "incoming": incoming_row} + + +static func build_npc_state_label( + rows: Dictionary, runtime_sync_error: String = "" +) -> String: + var lines: PackedStringArray = PackedStringArray() + var locked_row: Dictionary = rows.get("locked", {}) as Dictionary + if not locked_row.is_empty(): + var locked_id: String = str(locked_row.get("npcInstanceId", "")) + lines.append( + "NPC state: %s → %s" + % [ + PrototypeTargetConstants.display_short_name(locked_id), + str(locked_row.get("state", "unknown")), + ] + ) + var incoming_row: Dictionary = rows.get("incoming", {}) as Dictionary + if not incoming_row.is_empty(): + var incoming_id: String = str(incoming_row.get("npcInstanceId", "")) + lines.append( + "Incoming: %s → %s" + % [ + PrototypeTargetConstants.display_short_name(incoming_id), + str(incoming_row.get("state", "unknown")), + ] + ) + if lines.is_empty(): + if not runtime_sync_error.is_empty(): + return "NPC state: — (%s)" % runtime_sync_error + return "NPC state: —" + if not runtime_sync_error.is_empty(): + lines.append("(runtime sync: %s)" % runtime_sync_error) + return "\n".join(lines) + + +static func format_telegraph_line(row: Dictionary, hud_state: RefCounted = null) -> String: + var instance_id: String = str(row.get("npcInstanceId", "")).strip_edges() + var telegraph: Variant = row.get("activeTelegraph", null) + if telegraph == null or not telegraph is Dictionary: + return "" + var telegraph_dict: Dictionary = telegraph as Dictionary + var remaining: float = float(telegraph_dict.get("windupRemainingSeconds", 0.0)) + if hud_state != null and hud_state.has_method("display_windup_remaining"): + remaining = float(hud_state.call("display_windup_remaining", instance_id)) + if remaining <= 0.0: + return "" + var short_name: String = PrototypeTargetConstants.display_short_name(instance_id) + var telegraph_id: String = str(telegraph_dict.get("telegraphId", "")) + var archetype: String = str(telegraph_dict.get("archetypeKind", "")) + return "Telegraph: %s %s · %.1fs (%s)" % [short_name, telegraph_id, remaining, archetype] + + +static func build_telegraph_label( + rows: Dictionary, hud_state: RefCounted = null, runtime_sync_error: String = "" +) -> String: + var locked_row: Dictionary = rows.get("locked", {}) as Dictionary + var line: String = format_telegraph_line(locked_row, hud_state) + if line.is_empty(): + line = format_telegraph_line(rows.get("incoming", {}) as Dictionary, hud_state) + if line.is_empty(): + if not runtime_sync_error.is_empty(): + return "Telegraph: — (%s)" % runtime_sync_error + return "Telegraph: —" + return line + + +static func has_interpolated_telegraph_display( + rows: Dictionary, hud_state: RefCounted +) -> bool: + if hud_state == null: + return false + for key: String in ["locked", "incoming"]: + var row: Dictionary = rows.get(key, {}) as Dictionary + if row.is_empty(): + continue + var instance_id: String = str(row.get("npcInstanceId", "")).strip_edges() + if instance_id.is_empty(): + continue + var telegraph: Variant = row.get("activeTelegraph", null) + if telegraph == null or not telegraph is Dictionary: + continue + if hud_state.has_method("has_active_windup"): + if bool(hud_state.call("has_active_windup", instance_id)): + return true + return false diff --git a/client/scripts/npc_combat_hud_helpers.gd.uid b/client/scripts/npc_combat_hud_helpers.gd.uid new file mode 100644 index 0000000..00b4f25 --- /dev/null +++ b/client/scripts/npc_combat_hud_helpers.gd.uid @@ -0,0 +1 @@ +uid://bneo97hudhlpr1 diff --git a/client/scripts/npc_runtime_client.gd b/client/scripts/npc_runtime_client.gd index 78263b4..b4dbd89 100644 --- a/client/scripts/npc_runtime_client.gd +++ b/client/scripts/npc_runtime_client.gd @@ -53,7 +53,18 @@ static func parse_runtime_json(text: String) -> Variant: return root +func npc_row(instance_id: String, snapshot: Dictionary = {}) -> Dictionary: + var source: Dictionary = snapshot + if source.is_empty(): + source = _last_snapshot + return npc_row_in_snapshot(instance_id, source) + + static func npc_row(instance_id: String, snapshot: Dictionary = {}) -> Dictionary: + return npc_row_in_snapshot(instance_id, snapshot) + + +static func npc_row_in_snapshot(instance_id: String, snapshot: Dictionary) -> Dictionary: var source: Dictionary = snapshot if source.is_empty(): return {} diff --git a/client/test/npc_combat_hud_refresh_test.gd b/client/test/npc_combat_hud_refresh_test.gd index 26281e3..e031e07 100644 --- a/client/test/npc_combat_hud_refresh_test.gd +++ b/client/test/npc_combat_hud_refresh_test.gd @@ -3,71 +3,8 @@ extends GdUnitTestSuite ## NEO-97: combat-active gate + HUD label copy from snapshot fixtures. const NpcRuntimeClient := preload("res://scripts/npc_runtime_client.gd") -const PrototypeTargetConstants := preload("res://scripts/prototype_target_constants.gd") - - -class NpcCombatHudHarness: - extends RefCounted - var last_target_state: Dictionary = {} - var npc_runtime_snapshot: Dictionary = {} - var dev_player_id: String = "dev-local-1" - - func is_combat_active() -> bool: - var lock_id := locked_target_id() - if PrototypeTargetConstants.ANCHORS.has(lock_id): - return true - return not incoming_aggro_row().is_empty() - - func locked_target_id() -> String: - var locked_variant: Variant = last_target_state.get("lockedTargetId", null) - if locked_variant is String: - return (locked_variant as String).strip_edges() - return "" - - func incoming_aggro_row() -> Dictionary: - if npc_runtime_snapshot.is_empty(): - return {} - return NpcRuntimeClient.row_for_aggro_holder(dev_player_id, npc_runtime_snapshot) - - func resolve_hud_npc_rows() -> Dictionary: - var lock_id := locked_target_id() - var locked_row: Dictionary = {} - if PrototypeTargetConstants.ANCHORS.has(lock_id): - locked_row = NpcRuntimeClient.npc_row(lock_id, npc_runtime_snapshot) - var incoming_row: Dictionary = {} - var aggro_row: Dictionary = incoming_aggro_row() - if not aggro_row.is_empty(): - var aggro_id: String = str(aggro_row.get("npcInstanceId", "")).strip_edges() - if aggro_id != lock_id: - incoming_row = aggro_row - return {"locked": locked_row, "incoming": incoming_row} - - func build_npc_state_label() -> String: - var rows: Dictionary = resolve_hud_npc_rows() - var lines: PackedStringArray = PackedStringArray() - var locked_row: Dictionary = rows.get("locked", {}) as Dictionary - if not locked_row.is_empty(): - var locked_id: String = str(locked_row.get("npcInstanceId", "")) - lines.append( - "NPC state: %s → %s" - % [ - PrototypeTargetConstants.display_short_name(locked_id), - str(locked_row.get("state", "unknown")), - ] - ) - var incoming_row: Dictionary = rows.get("incoming", {}) as Dictionary - if not incoming_row.is_empty(): - var incoming_id: String = str(incoming_row.get("npcInstanceId", "")) - lines.append( - "Incoming: %s → %s" - % [ - PrototypeTargetConstants.display_short_name(incoming_id), - str(incoming_row.get("state", "unknown")), - ] - ) - if lines.is_empty(): - return "NPC state: —" - return "\n".join(lines) +const NpcRuntimeHudState := preload("res://scripts/npc_runtime_hud_state.gd") +const NpcCombatHudHelpers := preload("res://scripts/npc_combat_hud_helpers.gd") static func _elite_aggro_snapshot_json() -> String: @@ -81,45 +18,131 @@ static func _elite_aggro_snapshot_json() -> String: ) +static func _elite_telegraph_snapshot_json() -> String: + return ( + '{"schemaVersion":1,"serverTimeUtc":"2026-05-29T12:00:00Z","npcInstances":[' + + '{"npcInstanceId":"prototype_npc_elite","behaviorDefId":"prototype_elite_boss",' + + '"state":"telegraph_windup","aggroHolderPlayerId":"dev-local-1",' + + '"activeTelegraph":{"telegraphId":"prototype_elite_slam","npcInstanceId":' + + '"prototype_npc_elite","windupRemainingSeconds":2.5,"archetypeKind":"elite"}},' + + '{"npcInstanceId":"prototype_npc_melee","behaviorDefId":"prototype_melee_rush",' + + '"state":"idle","aggroHolderPlayerId":null,"activeTelegraph":null}' + + "]}" + ) + + func test_npc_lock_makes_combat_active_without_aggro_snapshot() -> void: # Arrange - var harness := NpcCombatHudHarness.new() - harness.last_target_state = {"lockedTargetId": "prototype_npc_melee", "validity": "ok"} + var target_state := {"lockedTargetId": "prototype_npc_melee", "validity": "ok"} # Act - var active: bool = harness.is_combat_active() + var active: bool = NpcCombatHudHelpers.is_combat_active(target_state, {}, "dev-local-1") # Assert assert_that(active).is_true() func test_aggro_holder_makes_combat_active_without_lock() -> void: # Arrange - var harness := NpcCombatHudHarness.new() var parsed: Variant = NpcRuntimeClient.parse_runtime_json(_elite_aggro_snapshot_json()) - harness.npc_runtime_snapshot = parsed as Dictionary # Act - var active: bool = harness.is_combat_active() + var active: bool = NpcCombatHudHelpers.is_combat_active({}, parsed as Dictionary, "dev-local-1") # Assert assert_that(active).is_true() func test_idle_without_lock_is_not_combat_active() -> void: # Arrange - var harness := NpcCombatHudHarness.new() - harness.npc_runtime_snapshot = {} # Act - var active: bool = harness.is_combat_active() + var active: bool = NpcCombatHudHelpers.is_combat_active({}, {}, "dev-local-1") # Assert assert_that(active).is_false() +func test_combat_poll_gate_stops_when_idle() -> void: + # Arrange + var timer := Timer.new() + timer.one_shot = false + add_child(timer) + timer.start() + # Act + var should_poll: bool = NpcCombatHudHelpers.is_combat_active({}, {}, "dev-local-1") + if not should_poll: + timer.stop() + # Assert + assert_that(should_poll).is_false() + assert_that(timer.is_stopped()).is_true() + + +func test_combat_poll_gate_starts_when_lock_held() -> void: + # Arrange + var timer := Timer.new() + timer.one_shot = false + add_child(timer) + var target_state := {"lockedTargetId": "prototype_npc_elite", "validity": "ok"} + # Act + var should_poll: bool = NpcCombatHudHelpers.is_combat_active(target_state, {}, "dev-local-1") + if should_poll and timer.is_stopped(): + timer.start() + # Assert + assert_that(should_poll).is_true() + assert_that(timer.is_stopped()).is_false() + + func test_locked_plus_incoming_state_label() -> void: # Arrange - var harness := NpcCombatHudHarness.new() - harness.last_target_state = {"lockedTargetId": "prototype_npc_melee", "validity": "ok"} + var target_state := {"lockedTargetId": "prototype_npc_melee", "validity": "ok"} var parsed: Variant = NpcRuntimeClient.parse_runtime_json(_elite_aggro_snapshot_json()) - harness.npc_runtime_snapshot = parsed as Dictionary + var rows: Dictionary = NpcCombatHudHelpers.resolve_hud_npc_rows( + target_state, parsed as Dictionary, "dev-local-1" + ) # Act - var label: String = harness.build_npc_state_label() + var label: String = NpcCombatHudHelpers.build_npc_state_label(rows) # Assert assert_that(label).contains("NPC state: Melee → idle") assert_that(label).contains("Incoming: Elite → aggro") + + +func test_telegraph_label_falls_back_to_incoming_row() -> void: + # Arrange + var target_state := {"lockedTargetId": "prototype_npc_melee", "validity": "ok"} + var parsed: Variant = NpcRuntimeClient.parse_runtime_json(_elite_telegraph_snapshot_json()) + var rows: Dictionary = NpcCombatHudHelpers.resolve_hud_npc_rows( + target_state, parsed as Dictionary, "dev-local-1" + ) + # Act + var label: String = NpcCombatHudHelpers.build_telegraph_label(rows) + # Assert + assert_that(label).contains("Telegraph: Elite prototype_elite_slam") + assert_that(label).contains("(elite)") + + +func test_telegraph_label_empty_when_local_windup_expired() -> void: + # Arrange + var short_windup_json := ( + '{"schemaVersion":1,"serverTimeUtc":"2026-05-29T12:00:00Z","npcInstances":[' + + '{"npcInstanceId":"prototype_npc_elite","behaviorDefId":"prototype_elite_boss",' + + '"state":"telegraph_windup","aggroHolderPlayerId":"dev-local-1",' + + '"activeTelegraph":{"telegraphId":"prototype_elite_slam","npcInstanceId":' + + '"prototype_npc_elite","windupRemainingSeconds":0.1,"archetypeKind":"elite"}}' + + "]}" + ) + var parsed: Variant = NpcRuntimeClient.parse_runtime_json(short_windup_json) + var rows: Dictionary = NpcCombatHudHelpers.resolve_hud_npc_rows( + {"lockedTargetId": "prototype_npc_elite", "validity": "ok"}, + parsed as Dictionary, + "dev-local-1" + ) + var hud_state: RefCounted = NpcRuntimeHudState.new() + hud_state.call("apply_snapshot", parsed as Dictionary) + await (Engine.get_main_loop() as SceneTree).create_timer(0.15).timeout + # Act + var label: String = NpcCombatHudHelpers.build_telegraph_label(rows, hud_state) + # Assert + assert_that(label).is_equal("Telegraph: —") + + +func test_runtime_sync_error_surfaces_on_empty_npc_state_label() -> void: + # Arrange + # Act + var label: String = NpcCombatHudHelpers.build_npc_state_label({}, "HTTP 500") + # Assert + assert_that(label).is_equal("NPC state: — (HTTP 500)") diff --git a/client/test/npc_runtime_client_test.gd b/client/test/npc_runtime_client_test.gd index 4d7575d..37149f6 100644 --- a/client/test/npc_runtime_client_test.gd +++ b/client/test/npc_runtime_client_test.gd @@ -104,6 +104,20 @@ func test_row_for_aggro_holder_returns_elite_row() -> void: assert_that(str(row.get("npcInstanceId", ""))).is_equal("prototype_npc_elite") +func test_npc_row_falls_back_to_last_snapshot_on_instance() -> void: + # Arrange + var transport := MockHttpTransport.new() + transport.body_json = _runtime_snapshot_json() + var c := _make_client(transport) + var got: Array = [] + c.connect("runtime_received", func(snapshot: Dictionary) -> void: got.append(snapshot)) + c.call("request_sync_from_server") + # Act + var row: Dictionary = c.call("npc_row", "prototype_npc_elite") as Dictionary + # Assert + assert_that(str(row.get("state", ""))).is_equal("telegraph_windup") + + func test_invalid_schema_emits_sync_failed() -> void: # Arrange var transport := MockHttpTransport.new() diff --git a/docs/decomposition/modules/E5_M2_NpcAiAndBehaviorProfiles.md b/docs/decomposition/modules/E5_M2_NpcAiAndBehaviorProfiles.md index 9814b79..a241f37 100644 --- a/docs/decomposition/modules/E5_M2_NpcAiAndBehaviorProfiles.md +++ b/docs/decomposition/modules/E5_M2_NpcAiAndBehaviorProfiles.md @@ -81,7 +81,7 @@ The **first shipped three-archetype NPC spine** under `content/npc-behaviors/*.j **NPC behavior definition registry (NEO-89):** **`INpcBehaviorDefinitionRegistry`** in `server/NeonSprawl.Server/Game/Npc/` wraps the startup-loaded catalog for read-only lookup by stable behavior `id` (`TryGetDefinition`), trim/lowercase validation (`TryNormalizeKnown`), and ordered enumeration (`GetDefinitionsInIdOrder`). Stable prototype id constants: **`PrototypeNpcBehaviorRegistry`**. **NEO-91+** (instance registry, runtime tick) should inject the interface rather than `NpcBehaviorDefinitionCatalog`. Plan: [NEO-89 implementation plan](../../plans/NEO-89-implementation-plan.md). -**NPC instance registry (NEO-91):** **`PrototypeNpcRegistry`** in `server/NeonSprawl.Server/Game/Npc/` — instance ids **`prototype_npc_melee`**, **`prototype_npc_ranged`**, **`prototype_npc_elite`** with behavior bindings + world anchors; **`ICombatEntityHealthStore`** uses catalog **`maxHp`** per instance. Plan: [NEO-91 implementation plan](../../plans/NEO-91-implementation-plan.md). **Breaking:** Godot constants migrate in [NEO-97](https://linear.app/neon-sprawl/issue/NEO-97). +**NPC instance registry (NEO-91):** **`PrototypeNpcRegistry`** in `server/NeonSprawl.Server/Game/Npc/` — instance ids **`prototype_npc_melee`**, **`prototype_npc_ranged`**, **`prototype_npc_elite`** with behavior bindings + world anchors; **`ICombatEntityHealthStore`** uses catalog **`maxHp`** per instance. Plan: [NEO-91 implementation plan](../../plans/NEO-91-implementation-plan.md). Godot **`prototype_target_constants.gd`** migration **landed in NEO-91**; [NEO-97](https://linear.app/neon-sprawl/issue/NEO-97) adds HUD poll clients only. **Threat / aggro store (NEO-92):** **`IThreatStateStore`** + **`AggroOperations`** in `server/NeonSprawl.Server/Game/Npc/` — first damaging cast acquires holder; leash break clears holder (move + cast hooks). HTTP projection: **`aggroHolderPlayerId`** on **`GET /game/world/npc-runtime-snapshot`** ([NEO-94](../../plans/NEO-94-implementation-plan.md)). Plan: [NEO-92 implementation plan](../../plans/NEO-92-implementation-plan.md). @@ -99,7 +99,7 @@ The **first shipped three-archetype NPC spine** under `content/npc-behaviors/*.j ## Risks and telemetry -- Telegraph desync: single **`npc-runtime-snapshot`** poll surface; no client windup math. +- Telegraph desync: single **`npc-runtime-snapshot`** poll surface; client display-only windup tick-down between polls (reconciled on each GET via **`npc_runtime_hud_state.gd`**). - Aggro confusion: deterministic first-hit + leash rules; instrument `npc_state_transition` when E9.M1 lands. ## Source anchors diff --git a/docs/plans/NEO-97-implementation-plan.md b/docs/plans/NEO-97-implementation-plan.md index 49c3712..f7a9bed 100644 --- a/docs/plans/NEO-97-implementation-plan.md +++ b/docs/plans/NEO-97-implementation-plan.md @@ -56,10 +56,10 @@ ## Implementation reconciliation (shipped) -- **`npc_runtime_client.gd`**, **`player_combat_health_client.gd`**, **`npc_runtime_hud_state.gd`** — poll + parse + windup interpolation. +- **`npc_runtime_client.gd`**, **`player_combat_health_client.gd`**, **`npc_runtime_hud_state.gd`**, **`npc_combat_hud_helpers.gd`** — poll + parse + windup interpolation + shared HUD formatters. - **`main.gd`** — combat-active ~1 Hz poll timer; **`TelegraphLabel`**, **`NpcStateLabel`**, **`PlayerCombatHpLabel`** render helpers (locked + incoming threat rows). - **`main.tscn`** — three HUD labels; downstream label offsets shifted. -- GdUnit: **`npc_runtime_client_test.gd`**, **`player_combat_health_client_test.gd`**, **`npc_runtime_hud_state_test.gd`**, **`npc_combat_hud_refresh_test.gd`**. +- GdUnit: **`npc_runtime_client_test.gd`**, **`player_combat_health_client_test.gd`**, **`npc_runtime_hud_state_test.gd`**, **`npc_combat_hud_refresh_test.gd`**; shared formatters in **`npc_combat_hud_helpers.gd`**. - **`docs/manual-qa/NEO-97.md`**, **`client/README.md`**, E5M2 backlog + module + alignment register updated. - **`prototype_target_constants.gd`** / **`prototype_target_markers.gd`** — verified on `main` (NEO-91); no rename to `npc_archetype_markers.gd`. @@ -88,7 +88,7 @@ ### Local windup mirror (`npc_runtime_hud_state.gd`) -- On **`runtime_received`**: store **`serverTimeUtc`** anchor + receive tick (same pattern as **`cooldown_state.gd`**). +- On **`runtime_received`**: store per-row **`windupRemainingSeconds`** at receive tick (no `serverTimeUtc` anchor — display smoothing only). - For each row with nested **`activeTelegraph`**: record **`windupRemainingSeconds`** at receive time. - **`display_windup_remaining(instance_id) -> float`**: anchor value minus elapsed local seconds since receive (floor at 0). - Reconcile on every poll — no client windup *scheduling*; only display smoothing between polls. @@ -147,31 +147,36 @@ Use server snake_case **`state`** as-is for prototype debug readability (`idle`, | `client/scripts/player_combat_health_client.gd.uid` | Godot uid companion. | | `client/scripts/npc_runtime_hud_state.gd` | Snapshot cache + windup tick-down between polls. | | `client/scripts/npc_runtime_hud_state.gd.uid` | Godot uid companion. | +| `client/scripts/npc_combat_hud_helpers.gd` | Shared combat-active gate + NPC/telegraph label formatters. | +| `client/scripts/npc_combat_hud_helpers.gd.uid` | Godot uid companion. | | `client/test/npc_runtime_client_test.gd` | HTTP double: URL, parse, row lookup, aggro-holder helper. | | `client/test/player_combat_health_client_test.gd` | HTTP double: URL, parse, schema guard. | | `client/test/npc_runtime_hud_state_test.gd` | Windup interpolation + reconcile on new snapshot. | -| `client/test/npc_combat_hud_refresh_test.gd` | Combat-active gate + label render harness (mock clients + snapshot fixtures). | +| `client/test/npc_combat_hud_refresh_test.gd` | Combat-active gate + timer mirror; telegraph fallback/expiry; sync error copy. | | `docs/manual-qa/NEO-97.md` | Godot manual QA — lock elite, cast, observe telegraph countdown + player HP drop. | ## Files to modify | Path | Rationale | |------|-----------| -| `client/scripts/main.gd` | Wire npc-runtime + combat-health clients, combat poll timer, HUD render helpers, combat-active gate. | +| `client/scripts/main.gd` | Wire npc-runtime + combat-health clients, combat poll timer, HUD render via helpers. | | `client/scenes/main.tscn` | Add `TelegraphLabel`, `NpcStateLabel`, `PlayerCombatHpLabel`; adjust downstream label offsets. | | `client/README.md` | NEO-97 NPC/telegraph HUD section; fix stale alpha/beta targeting/combat copy. | | `docs/plans/E5M2-prototype-backlog.md` | E5M2-11 acceptance checkboxes + landed note when complete. | -| `docs/decomposition/modules/E5_M2_NpcAiAndBehaviorProfiles.md` | NEO-97 client landed note under Related implementation slices. | +| `client/scripts/npc_runtime_client.gd` | Instance **`npc_row`** falls back to **`_last_snapshot`**. | +| `client/scripts/npc_combat_hud_helpers.gd` | Code-review follow-up: shared HUD helpers extracted from `main.gd`. | +| `docs/decomposition/modules/E5_M2_NpcAiAndBehaviorProfiles.md` | Risks + NPC instance row aligned with NEO-97 kickoff. | +| `docs/reviews/2026-05-29-NEO-97.md` | Strike-through review suggestions when fixed. | | `docs/decomposition/modules/documentation_and_implementation_alignment.md` | E5.M2 alignment register — E5M2-11 complete when shipped. | ## Tests | File | Coverage | |------|----------| -| `client/test/npc_runtime_client_test.gd` | GET URL; **`parse_runtime_json`** schema v1; **`npc_row`**; **`row_for_aggro_holder`**. | +| `client/test/npc_runtime_client_test.gd` | GET URL; parse; **`row_for_aggro_holder`**; instance **`npc_row`** **`_last_snapshot`** fallback. | | `client/test/player_combat_health_client_test.gd` | GET URL with player id; parse success/fail; **`defeated`** flag. | | `client/test/npc_runtime_hud_state_test.gd` | Apply snapshot → interpolated windup decreases; new poll reconciles to server value. | -| `client/test/npc_combat_hud_refresh_test.gd` | Combat-active gate starts/stops timer; locked + incoming label copy from fixture snapshots. | +| `client/test/npc_combat_hud_refresh_test.gd` | Combat-active gate + timer mirror; telegraph fallback/expiry; sync error on labels. | | *(existing unchanged)* | **`prototype_target_constants_test.gd`**, **`target_selection_client_test.gd`**, **`prototype_target_markers_test.gd`** — three-NPC tab/marker behavior already covered. | ## Open questions / risks diff --git a/docs/reviews/2026-05-29-NEO-97.md b/docs/reviews/2026-05-29-NEO-97.md index ca7e362..5015ae0 100644 --- a/docs/reviews/2026-05-29-NEO-97.md +++ b/docs/reviews/2026-05-29-NEO-97.md @@ -18,7 +18,7 @@ NEO-97 adds the Epic 5 Slice 2 **client half**: thin HTTP clients for **`GET /ga |------|--------| | `docs/plans/NEO-97-implementation-plan.md` | **Matches** — kickoff decisions (combat-active gate, locked + incoming HUD, interpolate windup, keep `prototype_target_markers.gd`) reflected in code; reconciliation section accurate; AC checked. | | `docs/plans/E5M2-prototype-backlog.md` (E5M2-11) | **Matches** — AC checked, landed note present; script name in backlog still says `npc_archetype_markers.gd` but plan kickoff explicitly adopted `prototype_target_markers.gd` (documented divergence). | -| `docs/decomposition/modules/E5_M2_NpcAiAndBehaviorProfiles.md` | **Partially matches** — NEO-97 landed line under Related implementation slices is correct; **Risks** bullet still says “no client windup math” (conflicts with kickoff interpolate decision); NPC instance row still says “Breaking: Godot constants migrate in NEO-97” though constants landed in NEO-91. | +| `docs/decomposition/modules/E5_M2_NpcAiAndBehaviorProfiles.md` | **Matches** — NEO-97 landed line correct; Risks + NPC instance row updated for windup display + NEO-91 constants migration. | | `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E5.M2 row includes **NEO-97 landed** with README/plan/manual QA links. | | `docs/manual-qa/NEO-97.md` | **Matches** — Godot steps cover lock, cast, telegraph countdown, player HP drop, incoming threat row, regressions. | | `client/README.md` | **Matches** — NPC runtime + telegraph HUD section documents poll gate, labels, tab order, script paths. | @@ -30,27 +30,27 @@ None. ## Suggestions -1. **E5.M2 Risks bullet** — Update “Telegraph desync: … no client windup math” to “display-only windup tick-down between polls; reconciled on each `npc-runtime-snapshot` GET” so module docs align with the NEO-97 kickoff decision and `npc_runtime_hud_state.gd`. +1. ~~**E5.M2 Risks bullet** — Update “Telegraph desync: … no client windup math” to “display-only windup tick-down between polls; reconciled on each `npc-runtime-snapshot` GET” so module docs align with the NEO-97 kickoff decision and `npc_runtime_hud_state.gd`.~~ **Done.** -2. **E5.M2 NPC instance row** — Change “Breaking: Godot constants migrate in NEO-97” to note migration **landed in NEO-91**; NEO-97 adds HUD poll clients only. +2. ~~**E5.M2 NPC instance row** — Change “Breaking: Godot constants migrate in NEO-97” to note migration **landed in NEO-91**; NEO-97 adds HUD poll clients only.~~ **Done.** -3. **Test coverage vs plan** — `npc_combat_hud_refresh_test.gd` covers combat-active bool + `NpcStateLabel` copy but not **`TelegraphLabel`** formatting or **timer start/stop** (called out in the implementation plan test table). Add at least one telegraph line assertion (locked vs incoming fallback) and, if feasible without heavy scene wiring, a timer-gate test mirroring `_update_combat_poll_gate()`. +3. ~~**Test coverage vs plan** — `npc_combat_hud_refresh_test.gd` covers combat-active bool + `NpcStateLabel` copy but not **`TelegraphLabel`** formatting or **timer start/stop** (called out in the implementation plan test table). Add at least one telegraph line assertion (locked vs incoming fallback) and, if feasible without heavy scene wiring, a timer-gate test mirroring `_update_combat_poll_gate()`.~~ **Done.** Telegraph fallback + local windup expiry + timer mirror tests added. -4. **Shared HUD helpers** — Combat-active gate, row resolution, and label formatters are duplicated in `main.gd` (~180 lines) and `NpcCombatHudHarness` in tests. Extract to a small RefCounted (e.g. `npc_combat_hud_helpers.gd`) used by both to prevent drift; optional follow-up given NEO-85 precedent of wiring in `main.gd`. +4. ~~**Shared HUD helpers** — Combat-active gate, row resolution, and label formatters are duplicated in `main.gd` (~180 lines) and `NpcCombatHudHarness` in tests. Extract to a small RefCounted (e.g. `npc_combat_hud_helpers.gd`) used by both to prevent drift; optional follow-up given NEO-85 precedent of wiring in `main.gd`.~~ **Done.** `npc_combat_hud_helpers.gd` shared by `main.gd` and tests. -5. **Telegraph at zero** — `_format_telegraph_line` can show **`· 0.0s`** while the snapshot row still carries `activeTelegraph` until the next poll. Consider returning empty when `display_windup_remaining` ≤ 0 so `TelegraphLabel` shows **`—`** between local expiry and server reconcile. +5. ~~**Telegraph at zero** — `_format_telegraph_line` can show **`· 0.0s`** while the snapshot row still carries `activeTelegraph` until the next poll. Consider returning empty when `display_windup_remaining` ≤ 0 so `TelegraphLabel` shows **`—`** between local expiry and server reconcile.~~ **Done.** `format_telegraph_line` returns empty when remaining ≤ 0. -6. **Runtime sync failure** — `_on_npc_runtime_sync_failed` re-renders from the last good snapshot without surfacing failure on `NpcStateLabel` / `TelegraphLabel` (player HP already shows error). Acceptable for prototype; optionally dash NPC/telegraph lines or append a sync-failed hint for QA clarity. +6. ~~**Runtime sync failure** — `_on_npc_runtime_sync_failed` re-renders from the last good snapshot without surfacing failure on `NpcStateLabel` / `TelegraphLabel` (player HP already shows error). Acceptable for prototype; optionally dash NPC/telegraph lines or append a sync-failed hint for QA clarity.~~ **Done.** `_npc_runtime_sync_error` appended to NPC/telegraph labels when empty or alongside stale rows. ## Nits -- Nit: `npc_runtime_client.gd` stores `_last_snapshot` but static `npc_row()` does not fall back to it (unlike `combat_targets_client.target_row()`); field is write-only today. +- ~~Nit: `npc_runtime_client.gd` stores `_last_snapshot` but static `npc_row()` does not fall back to it (unlike `combat_targets_client.target_row()`); field is write-only today.~~ **Done.** Instance `npc_row()` falls back to `_last_snapshot`. - Nit: Duplicate `MockHttpTransport` inner classes in `npc_runtime_client_test.gd` and `player_combat_health_client_test.gd` — fine for now; a shared test helper would reduce copy-paste. -- Nit: `main.gd` grows ~250 lines for NEO-97; repo policy prefers thin `main.gd` — track for a future extract when the next HUD story lands. +- Nit: `main.gd` grows ~250 lines for NEO-97; repo policy prefers thin `main.gd` — track for a future extract when the next HUD story lands. **Partially addressed** — helpers extracted; poll wiring remains in `main.gd`. -- Nit: Plan text references `serverTimeUtc` anchor in `npc_runtime_hud_state.gd`; implementation uses receive tick + server `windupRemainingSeconds` only — sufficient for display smoothing; plan wording could be tightened. +- ~~Nit: Plan text references `serverTimeUtc` anchor in `npc_runtime_hud_state.gd`; implementation uses receive tick + server `windupRemainingSeconds` only — sufficient for display smoothing; plan wording could be tightened.~~ **Done.** ## Verification