diff --git a/CLAUDE.md b/CLAUDE.md index d62ddf7..efc2887 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -482,9 +482,18 @@ should not be — see `docs/restructure-plan.md`. 2. **Serialized references cannot cross a scene boundary.** Unity nulls them. The split cost exactly seven: both NPCs' `dialogueRunner`, `dialogueUI` and `player`, plus the reveal - camera's tracking target. All are resolved at runtime now — `SceneServices.Resolve` for the - first, `CinemachineFollowsPlayer` for the last. Anything new that a level object needs from - Systems must follow that pattern, and `BootstrapTests` is where you prove it works. + camera's tracking target. All are resolved at runtime now — `SceneServices` for the first, + `CinemachineFollowsPlayer` for the last. + + **Resolve at the point of use, never in `Awake`.** A level object can wake up before the + scene holding its dependency exists — open the level on its own, or in the wrong order, and + `Awake` runs with no Systems scene in sight. That produced + `[SceneServices] Bartender: could not resolve dialogueRunner`, and because the result was + cached once, it stayed broken for the whole session. `SceneServices.TryResolve` is the silent + lookup for speculative calls; `SceneServices.Resolve` logs and belongs only where the + dependency is genuinely needed. `NPCStandIn.ResolvedDialogueRunner` is the pattern to copy. + `SceneOrderTests` holds the property, and asserts it is actually reproducing the race rather + than passing for free. 3. **The player's NavMeshAgent outlives the NavMesh.** The player is in Systems, the NavMesh is baked into the level, so there is a window during load where the agent exists and is not on a diff --git a/NightclubArcadia/Assets/Scripts/Core/SceneServices.cs b/NightclubArcadia/Assets/Scripts/Core/SceneServices.cs index d32cd05..9fdf3d4 100644 --- a/NightclubArcadia/Assets/Scripts/Core/SceneServices.cs +++ b/NightclubArcadia/Assets/Scripts/Core/SceneServices.cs @@ -17,22 +17,40 @@ namespace NightclubArcadia.Core /// public static class SceneServices { - /// Inspector value if set, otherwise the first one in any loaded scene. - public static T Resolve(T assigned, Object context, string field) where T : Object + /// + /// Inspector value if set, otherwise the first one in any loaded scene. Silent: + /// returns null when nothing is found. + /// + /// Use this when the answer may legitimately not exist yet — during Awake, for + /// instance, when the scene holding it may not have loaded. + /// + public static T TryResolve(T assigned) where T : Object { if (assigned != null) { return assigned; } - var found = Object.FindFirstObjectByType(); + // Include inactive: a system parked inactive at startup is still the object + // we mean, and excluding it produces a confusing "not found" for something + // sitting right there in the hierarchy. + return Object.FindFirstObjectByType(FindObjectsInactive.Include); + } + + /// + /// As TryResolve, but logs an error when nothing is found. Only call this at the + /// point the dependency is actually needed — calling it during Awake reports a + /// failure that may simply be a scene that has not finished loading. + /// + public static T Resolve(T assigned, Object context, string field) where T : Object + { + var found = TryResolve(assigned); if (found == null) { Debug.LogError( $"[SceneServices] {context?.name}: could not resolve {field} ({typeof(T).Name}). " + "Is the Systems scene loaded?", context); } - return found; } diff --git a/NightclubArcadia/Assets/Scripts/Dialogue/DialogueInteractable.cs b/NightclubArcadia/Assets/Scripts/Dialogue/DialogueInteractable.cs index 0f1cb2f..f2bef9b 100644 --- a/NightclubArcadia/Assets/Scripts/Dialogue/DialogueInteractable.cs +++ b/NightclubArcadia/Assets/Scripts/Dialogue/DialogueInteractable.cs @@ -21,8 +21,9 @@ namespace NightclubArcadia.Dialogue void Awake() { - // Same cross-scene resolution as NPCStandIn: the runner is in Systems. - dialogueRunner = SceneServices.Resolve(dialogueRunner, this, nameof(dialogueRunner)); + // Best-effort and silent; the real lookup happens at the point of use, since + // Systems may not have loaded when this level object awakes. See Runner. + dialogueRunner = SceneServices.TryResolve(dialogueRunner); interactAction = new InputAction("Interact"); interactAction.AddBinding("/e"); @@ -36,6 +37,15 @@ namespace NightclubArcadia.Dialogue }; } + /// + /// The dialogue runner, resolved on demand rather than in Awake, so that load + /// order between this level scene and Systems does not matter. Cached once found. + /// + DialogueRunner Runner => + dialogueRunner != null + ? dialogueRunner + : dialogueRunner = SceneServices.TryResolve(null); + void OnEnable() => interactAction?.Enable(); void OnDisable() => interactAction?.Disable(); @@ -57,6 +67,16 @@ namespace NightclubArcadia.Dialogue async void StartConversation() { + var runner = Runner; + if (runner == null) + { + Debug.LogError( + $"[DialogueInteractable] {name}: no DialogueRunner in any loaded scene. " + + "It lives in Systems.unity — open Bootstrap, or Tools → Nightclub Arcadia → " + + "Open Game Scenes.", this); + return; + } + // Player controls are locked by DialogueTransitionController off the // runner's onDialogueStart event — nothing to do for that here. if (DialogueUIVisibility.Show(dialogueUI)) @@ -67,7 +87,7 @@ namespace NightclubArcadia.Dialogue await Awaitable.NextFrameAsync(); } - await dialogueRunner.StartDialogue(yarnNodeName); + await runner.StartDialogue(yarnNodeName); } } } diff --git a/NightclubArcadia/Assets/Scripts/NPCs/NPCStandIn.cs b/NightclubArcadia/Assets/Scripts/NPCs/NPCStandIn.cs index b4e91ff..ba26c70 100644 --- a/NightclubArcadia/Assets/Scripts/NPCs/NPCStandIn.cs +++ b/NightclubArcadia/Assets/Scripts/NPCs/NPCStandIn.cs @@ -54,9 +54,9 @@ public class NPCStandIn : MonoBehaviour rend = GetComponent(); rend.material.color = npcColor; - // The dialogue runner lives in the Systems scene, so this reference cannot be - // serialized from a level scene. An Inspector value still wins if one is set. - dialogueRunner = SceneServices.Resolve(dialogueRunner, this, nameof(dialogueRunner)); + // Best-effort only, and silent. The runner lives in the Systems scene, which may + // not have loaded yet when this level's objects awake — see ResolvedDialogueRunner. + dialogueRunner = SceneServices.TryResolve(dialogueRunner); mainCam = Camera.main; @@ -78,6 +78,20 @@ public class NPCStandIn : MonoBehaviour }; } + /// + /// The dialogue runner, resolved on demand. + /// + /// Not resolved once in Awake: the runner lives in the Systems scene, and an NPC in + /// a level scene can wake up before that scene exists — loading the level on its own, + /// or in a different order, used to log "could not resolve dialogueRunner" and then + /// stay broken for the rest of the session. Looking it up at the point of use makes + /// load order irrelevant, and the result is cached, so this costs one search. + /// + public DialogueRunner ResolvedDialogueRunner => + dialogueRunner != null + ? dialogueRunner + : dialogueRunner = SceneServices.TryResolve(null); + void OnEnable() => interactAction?.Enable(); void OnDisable() => interactAction?.Disable(); @@ -153,9 +167,13 @@ public class NPCStandIn : MonoBehaviour public void Interact() { - if (dialogueRunner == null) + var runner = ResolvedDialogueRunner; + if (runner == null) { - Debug.LogWarning($"[NPCStandIn] No DialogueRunner assigned on {npcName}"); + Debug.LogError( + $"[NPCStandIn] {npcName}: no DialogueRunner in any loaded scene. " + + "It lives in Systems.unity — open Bootstrap, or Tools → Nightclub Arcadia → " + + "Open Game Scenes.", this); return; } @@ -164,7 +182,7 @@ public class NPCStandIn : MonoBehaviour return; } - if (!dialogueRunner.IsDialogueRunning) + if (!runner.IsDialogueRunning) { StartDialogueAsync(); } @@ -182,7 +200,7 @@ public class NPCStandIn : MonoBehaviour await Awaitable.NextFrameAsync(); } - await dialogueRunner.StartDialogue(yarnStartNode); + await ResolvedDialogueRunner.StartDialogue(yarnStartNode); } /// @@ -197,10 +215,16 @@ public class NPCStandIn : MonoBehaviour return dialogueUI; } - var canvas = dialogueRunner.GetComponentInChildren(true); + var runner = ResolvedDialogueRunner; + if (runner == null) + { + return null; + } + + var canvas = runner.GetComponentInChildren(true); if (canvas == null) { - Debug.LogWarning($"[NPCStandIn] No Canvas found under {dialogueRunner.name} for {npcName}"); + Debug.LogWarning($"[NPCStandIn] No Canvas found under {runner.name} for {npcName}"); return null; } diff --git a/NightclubArcadia/Assets/Tests/PlayMode/SceneOrderTests.cs b/NightclubArcadia/Assets/Tests/PlayMode/SceneOrderTests.cs new file mode 100644 index 0000000..88dd259 --- /dev/null +++ b/NightclubArcadia/Assets/Tests/PlayMode/SceneOrderTests.cs @@ -0,0 +1,69 @@ +using System.Collections; +using System.Linq; +using NUnit.Framework; +using UnityEngine; +using UnityEngine.SceneManagement; +using UnityEngine.TestTools; + +namespace NightclubArcadia.PlayMode.Tests +{ + /// + /// The level must survive being loaded BEFORE the systems it depends on. + /// + /// BootstrapTests only covers the happy order — Bootstrap loads Systems, then the + /// level, so by the time an NPC awakes the dialogue runner is already there. The + /// Editor does not guarantee that. Open the level on its own, or open scenes in a + /// different order, press Play, and NPCStandIn.Awake runs with no Systems scene in + /// sight. That produced: + /// + /// [SceneServices] Bartender: could not resolve dialogueRunner + /// + /// Resolving a cross-scene dependency in Awake is a race by construction. The fix + /// is to resolve at the point of use instead, so any load order works — and this is + /// the test that holds that property. + /// + public class SceneOrderTests + { + const string Systems = "Systems"; + const string Level = "SC101_ConferenceHall"; + + [UnityTearDown] + public IEnumerator TearDown() + { + yield return SceneManager.LoadSceneAsync("Bootstrap", LoadSceneMode.Single); + } + + [UnityTest] + public IEnumerator LevelLoadedBeforeSystems_NpcsStillResolveTheirRunner() + { + // deliberately the wrong way round + yield return SceneManager.LoadSceneAsync(Level, LoadSceneMode.Single); + yield return SceneManager.LoadSceneAsync(Systems, LoadSceneMode.Additive); + for (var i = 0; i < 3; i++) + { + yield return null; + } + + var npcs = Object.FindObjectsByType( + FindObjectsInactive.Include, FindObjectsSortMode.None); + Assert.IsNotEmpty(npcs, "Expected NPCs in the level scene"); + + // Read the serialized field BEFORE touching the lazy property, which caches + // into it. If this is null, Awake really did run with no Systems scene — i.e. + // the race this test exists for is genuinely reproduced here, and the + // assertion below is not passing for free. + var eager = npcs.Count(n => n.dialogueRunner == null); + Debug.Log($"[Order] NPCs whose Awake-time resolution failed: {eager}/{npcs.Length}"); + Assert.Greater(eager, 0, + "Awake-time resolution succeeded, so this test is not reproducing the race " + + "it was written for. Rewrite it before trusting it."); + + foreach (var npc in npcs) + { + Assert.IsNotNull(npc.ResolvedDialogueRunner, + $"'{npc.name}' could not reach the DialogueRunner when the level loaded " + + "before Systems. Cross-scene resolution must not depend on load order."); + } + } + } +} diff --git a/NightclubArcadia/Assets/Tests/PlayMode/SceneOrderTests.cs.meta b/NightclubArcadia/Assets/Tests/PlayMode/SceneOrderTests.cs.meta new file mode 100644 index 0000000..e7dac1a --- /dev/null +++ b/NightclubArcadia/Assets/Tests/PlayMode/SceneOrderTests.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: 959e3d8621d544168b89e4952a7668e0 +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: