diff --git a/Assets/Tests/Editor/PausePointCaptureModeTests.cs b/Assets/Tests/Editor/PausePointCaptureModeTests.cs index ef650afa0b..55b2c49f86 100644 --- a/Assets/Tests/Editor/PausePointCaptureModeTests.cs +++ b/Assets/Tests/Editor/PausePointCaptureModeTests.cs @@ -116,7 +116,7 @@ public void Clear_WhenContinuousMarkerHasHits_DisarmsAndPreservesHistory() UloopPausePointRegistry.Enable("jump", 30, UloopPausePointCaptureMode.Continuous, 20); UloopPausePointRegistry.Hit("jump"); - UloopPausePointSnapshot snapshot = UloopPausePointRegistry.Clear("jump"); + (UloopPausePointSnapshot snapshot, _) = UloopPausePointRegistry.Clear("jump"); Assert.That(snapshot.IsEnabled, Is.False); Assert.That(snapshot.CapturedVariableHistory, Has.Count.EqualTo(1)); diff --git a/Assets/Tests/Editor/PausePointTests.cs b/Assets/Tests/Editor/PausePointTests.cs index 80de6d90b1..8a90ccfe0f 100644 --- a/Assets/Tests/Editor/PausePointTests.cs +++ b/Assets/Tests/Editor/PausePointTests.cs @@ -315,7 +315,7 @@ public void Clear_WhenPausePointIsEnabled_DisablesWithoutPause() // Verifies explicit clear prevents later marker hits from pausing Unity. UloopPausePointRegistry.Enable("jump", 30); - UloopPausePointSnapshot snapshot = UloopPausePointRegistry.Clear("jump"); + (UloopPausePointSnapshot snapshot, _) = UloopPausePointRegistry.Clear("jump"); UloopPausePoint.Pause("jump"); Assert.That(snapshot.Status, Is.EqualTo(UloopPausePointStatus.Cleared)); @@ -330,37 +330,90 @@ public void Clear_WhenPausePointWasHit_ReportsAlreadyHitMessage() UloopPausePointRegistry.Enable("jump", 30); UloopPausePoint.Pause("jump"); - UloopPausePointSnapshot snapshot = UloopPausePointRegistry.Clear("jump"); + (UloopPausePointSnapshot snapshot, _) = UloopPausePointRegistry.Clear("jump"); Assert.That(snapshot.Message, Is.EqualTo("Pause point was already hit (auto-disarmed); nothing to clear.")); Assert.That(_pauseController.IsPaused, Is.False); } [Test] - public void Clear_AfterHit_ShouldResumeEditorPause() + public void Clear_AfterHit_ResumesPausePointOwnedPauseAndReportsResumed() { - // Verifies Option B: Clear resumes even when the pause was left from a pause-point hit. + // Verifies Clear resumes and reports ResumedFromPause when a pause-point hit owns the pause. UloopPausePointRegistry.Enable("jump", 30); UloopPausePoint.Pause("jump"); Assert.That(_pauseController.IsPaused, Is.True); - UloopPausePointRegistry.Clear("jump"); + (UloopPausePointSnapshot _, bool resumedFromPause) = UloopPausePointRegistry.Clear("jump"); + Assert.That(resumedFromPause, Is.True); Assert.That(_pauseController.IsPaused, Is.False); } [Test] - public void ClearAll_AfterHit_ShouldResumeEditorPause() + public void Clear_WhenEditorManuallyPaused_LeavesPauseUntouchedAndReportsNotResumed() + { + // Verifies Clear preserves a manual pause (no open pause window) instead of resuming it. + UloopPausePointRegistry.Enable("jump", 30); + _pauseController.PauseExternally(); + + (UloopPausePointSnapshot _, bool resumedFromPause) = UloopPausePointRegistry.Clear("jump"); + + Assert.That(resumedFromPause, Is.False); + Assert.That(_pauseController.IsPaused, Is.True); + Assert.That(_pauseController.ResumeCount, Is.EqualTo(0)); + } + + [Test] + public void ClearAll_AfterHit_ResumesPausePointOwnedPauseAndReportsResumed() { - // Verifies ClearAll also resumes under Option B. + // Verifies ClearAll resumes and reports ResumedFromPause when a hit owns the pause. UloopPausePointRegistry.Enable("jump", 30); UloopPausePoint.Pause("jump"); - UloopPausePointRegistry.ClearAll(); + UloopPausePointClearAllResult result = UloopPausePointRegistry.ClearAll(); + Assert.That(result.ResumedFromPause, Is.True); Assert.That(_pauseController.IsPaused, Is.False); } + [Test] + public void ClearAll_WhenEditorManuallyPaused_LeavesPauseUntouchedAndReportsNotResumed() + { + // Verifies ClearAll preserves a manual pause (no open pause window) instead of resuming it. + UloopPausePointRegistry.Enable("jump", 30); + _pauseController.PauseExternally(); + + UloopPausePointClearAllResult result = UloopPausePointRegistry.ClearAll(); + + Assert.That(result.ResumedFromPause, Is.False); + Assert.That(_pauseController.IsPaused, Is.True); + Assert.That(_pauseController.ResumeCount, Is.EqualTo(0)); + } + + [Test] + public void Clear_WhenWindowOpenButEditorAlreadyExternallyUnpaused_DoesNotReportResumeAndClosesStaleWindow() + { + // Verifies the still-open window is reconciled before deciding: a hit opened the + // window, then the Editor was unpaused externally before the update tick observed it. + // Clear must not claim it resumed Play Mode (Resume is a no-op on an unpaused Editor) + // and must close the stale window so it stops freezing expiry. ClearAll shares the same + // ResumeEditorPauseIfOwnedByPausePoint path, so this covers both entry points. + UloopPausePointRegistry.Enable("jump", 30); + UloopPausePoint.Pause("jump"); + _pauseController.ResumeExternally(); + + (UloopPausePointSnapshot _, bool resumedFromPause) = UloopPausePointRegistry.Clear("jump"); + + Assert.That(resumedFromPause, Is.False); + Assert.That(_pauseController.ResumeCount, Is.EqualTo(0)); + // The stale window is closed: an unrelated marker's countdown is no longer frozen. + UloopPausePointRegistry.Enable("dash", 1); + _nowUtc = _nowUtc.AddSeconds(2); + UloopPausePointRegistry.ApplyCaptureWindowExpirations(); + Assert.That(UloopPausePointRegistry.GetStatus("dash").Status, Is.EqualTo(UloopPausePointStatus.Expired)); + } + [Test] public void ApplyCaptureWindowExpirations_WhenHitPastTimeoutWhilePaused_DoesNotExpireUntilResumed() { @@ -549,7 +602,7 @@ public void Clear_WhenPausePointExpired_ReportsAlreadyExpiredMessage() UloopPausePointRegistry.Enable("jump", 1); _nowUtc = _nowUtc.AddSeconds(2); - UloopPausePointSnapshot snapshot = UloopPausePointRegistry.Clear("jump"); + (UloopPausePointSnapshot snapshot, _) = UloopPausePointRegistry.Clear("jump"); Assert.That(snapshot.Message, Is.EqualTo("Pause point had already expired before being hit; nothing to clear.")); } @@ -561,7 +614,7 @@ public void Clear_WhenPausePointAlreadyCleared_ReportsAlreadyClearedMessage() UloopPausePointRegistry.Enable("jump", 30); UloopPausePointRegistry.Clear("jump"); - UloopPausePointSnapshot snapshot = UloopPausePointRegistry.Clear("jump"); + (UloopPausePointSnapshot snapshot, _) = UloopPausePointRegistry.Clear("jump"); Assert.That(snapshot.Message, Is.EqualTo("Pause point was already cleared.")); } @@ -585,6 +638,48 @@ public async Task ClearAll_WhenNothingActive_ReportsNoActiveMessage() Assert.That(response.Message, Is.EqualTo("No active pause points to clear.")); } + [Test] + public async Task Clear_WhenResumingPausePointOwnedPause_SetsResumedPlayModeWarning() + { + // Verifies the clear-pause-point tool warns when the clear resumed a pause-point-owned pause. + UloopPausePointRegistry.Enable("jump", 30); + UloopPausePoint.Pause("jump"); + + ClearPausePointTool tool = new(); + JObject parameters = new() { ["id"] = "jump" }; + PausePointResponse response = (PausePointResponse)await tool.ExecuteAsync(parameters, CancellationToken.None); + + Assert.That(response.Warning, Is.EqualTo(SourcePausePointConstants.ClearResumedPlayModeWarning)); + } + + [Test] + public async Task Clear_WhenManualPausePreserved_SetsNoWarning() + { + // Verifies the clear-pause-point tool emits no resume warning when it preserves a manual pause. + UloopPausePointRegistry.Enable("jump", 30); + _pauseController.PauseExternally(); + + ClearPausePointTool tool = new(); + JObject parameters = new() { ["id"] = "jump" }; + PausePointResponse response = (PausePointResponse)await tool.ExecuteAsync(parameters, CancellationToken.None); + + Assert.That(response.Warning, Is.Empty); + } + + [Test] + public async Task ClearAll_WhenResumingPausePointOwnedPause_SetsResumedPlayModeWarning() + { + // Verifies clear-pause-point --all warns when the bulk clear resumed a pause-point-owned pause. + UloopPausePointRegistry.Enable("jump", 30); + UloopPausePoint.Pause("jump"); + + ClearPausePointTool tool = new(); + JObject parameters = new() { ["all"] = true }; + PausePointResponse response = (PausePointResponse)await tool.ExecuteAsync(parameters, CancellationToken.None); + + Assert.That(response.Warning, Is.EqualTo(SourcePausePointConstants.ClearResumedPlayModeWarning)); + } + [Test] public async Task Enable_WhenMarkerCreated_ReturnsStateManagementFields() { @@ -1268,6 +1363,15 @@ public void ResumeExternally() { IsPaused = false; } + + // Simulates a manual pause set outside the pause-point workflow (control-play-mode + // --action Pause, or the Editor's own pause button). It never opens a pause window, so + // clear must leave it untouched. PauseCount is not incremented because no pause-point + // hit requested it. + public void PauseExternally() + { + IsPaused = true; + } } } } diff --git a/Packages/src/Editor/FirstPartyTools/PausePoint/PausePointTools.cs b/Packages/src/Editor/FirstPartyTools/PausePoint/PausePointTools.cs index d05767c5ee..97da069b81 100644 --- a/Packages/src/Editor/FirstPartyTools/PausePoint/PausePointTools.cs +++ b/Packages/src/Editor/FirstPartyTools/PausePoint/PausePointTools.cs @@ -133,7 +133,10 @@ internal static PausePointResponse FromClearAll(UloopPausePointClearAllResult re EditorState = PausePointEditorState.FromSnapshot(result.EditorState), Message = result.ClearedCount == 0 ? "No active pause points to clear." - : "Pause points cleared." + : "Pause points cleared.", + Warning = result.ResumedFromPause + ? SourcePausePointConstants.ClearResumedPlayModeWarning + : string.Empty }; } } @@ -321,14 +324,20 @@ public PausePointResponse Clear(ClearPausePointSchema parameters) return CreateValidationFailure(idError); } - UloopPausePointSnapshot snapshot = UloopPausePointRegistry.Clear(parameters.Id); + (UloopPausePointSnapshot snapshot, bool resumedFromPause) = UloopPausePointRegistry.Clear(parameters.Id); LogCleared(snapshot.Id, snapshot.StatusBeforeClear); if (snapshot.StatusBeforeClear == UloopPausePointStatus.Expired) { LogExpired(snapshot.Id, snapshot.ElapsedSinceEnabledMilliseconds); } - return PausePointResponse.FromSnapshot(snapshot); + PausePointResponse response = PausePointResponse.FromSnapshot(snapshot); + if (resumedFromPause) + { + response.Warning = SourcePausePointConstants.ClearResumedPlayModeWarning; + } + + return response; } // Why: PausePointStatusBridgeCommand duplicates this instead of sharing it, since that diff --git a/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointConstants.cs b/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointConstants.cs index 01d1fec466..72703aadda 100644 --- a/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointConstants.cs +++ b/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointConstants.cs @@ -84,6 +84,13 @@ internal static class SourcePausePointConstants public const string PreLineSnapshotTimingNote = "pre-line: variables are captured before ResolvedLine executes"; + // Surfaced only when a clear actually resumed Play Mode, so the caller understands the + // clear had a side effect on run state. A manual pause (control-play-mode --action Pause + // or the Editor pause button) is left untouched by clear and never triggers this. + public const string ClearResumedPlayModeWarning = + "This clear resumed Play Mode because the pause was owned by a pause-point hit. " + + "A manual pause set outside the pause-point workflow would have been left untouched."; + // Release code optimization strips most sequence points and hoists/elides locals, so the // Resolver's PDB-driven lookup cannot reliably find a patch location; rejecting up front // avoids patching the wrong instruction instead of failing later in a confusing way. diff --git a/Packages/src/Editor/Infrastructure/Api/PausePointStatusBridgeCommand.cs b/Packages/src/Editor/Infrastructure/Api/PausePointStatusBridgeCommand.cs index 3cdd19f55d..f3a5dd7942 100644 --- a/Packages/src/Editor/Infrastructure/Api/PausePointStatusBridgeCommand.cs +++ b/Packages/src/Editor/Infrastructure/Api/PausePointStatusBridgeCommand.cs @@ -40,7 +40,9 @@ public static PausePointStatusResponse Clear(JToken paramsToken) // SourcePausePointPatcher wires into it, so this bridge - which must not reference // that Editor-only tool assembly directly - never leaves a Harmony injection attached // after the marker itself reports Cleared. - UloopPausePointSnapshot snapshot = UloopPausePointRegistry.Clear(id); + // The CLI polling bridge only reports marker status; the resumed-from-pause side + // effect is surfaced through the clear-pause-point tool response, not this path. + (UloopPausePointSnapshot snapshot, bool _) = UloopPausePointRegistry.Clear(id); LogCleared(id, snapshot.StatusBeforeClear); if (snapshot.StatusBeforeClear == UloopPausePointStatus.Expired) { diff --git a/Packages/src/Runtime/PausePoints/UloopPausePointClearAllResult.cs b/Packages/src/Runtime/PausePoints/UloopPausePointClearAllResult.cs index f7c75395c7..81fb3fd4a6 100644 --- a/Packages/src/Runtime/PausePoints/UloopPausePointClearAllResult.cs +++ b/Packages/src/Runtime/PausePoints/UloopPausePointClearAllResult.cs @@ -13,7 +13,8 @@ public UloopPausePointClearAllResult( int clearedCount, DateTime clearedAtUtc, UloopPausePointEditorStateSnapshot editorState, - string[] clearedIds) + string[] clearedIds, + bool resumedFromPause) { Debug.Assert(editorState != null, "editorState must not be null"); Debug.Assert(clearedIds != null, "clearedIds must not be null"); @@ -22,12 +23,18 @@ public UloopPausePointClearAllResult( ClearedAtUtc = clearedAtUtc; EditorState = editorState; ClearedIds = clearedIds; + ResumedFromPause = resumedFromPause; } public int ClearedCount { get; } public DateTime ClearedAtUtc { get; } public UloopPausePointEditorStateSnapshot EditorState { get; } public string[] ClearedIds { get; } + + // True when the bulk clear actually resumed a pause-point-owned Editor pause, so callers + // can warn that Play Mode was resumed as a side effect. False when no pause window was + // open (e.g. nothing was paused, or only a manual pause the clear intentionally preserved). + public bool ResumedFromPause { get; } } } #endif diff --git a/Packages/src/Runtime/PausePoints/UloopPausePointRegistry.cs b/Packages/src/Runtime/PausePoints/UloopPausePointRegistry.cs index 7a48565d46..34d6e0aa44 100644 --- a/Packages/src/Runtime/PausePoints/UloopPausePointRegistry.cs +++ b/Packages/src/Runtime/PausePoints/UloopPausePointRegistry.cs @@ -75,7 +75,7 @@ public static UloopPausePointSnapshot Enable( return entry.ToSnapshot(now, _pauseController); } - public static UloopPausePointSnapshot Clear(string id) + public static (UloopPausePointSnapshot Snapshot, bool ResumedFromPause) Clear(string id) { Debug.Assert(!string.IsNullOrWhiteSpace(id), "id must not be null or empty"); @@ -84,7 +84,7 @@ public static UloopPausePointSnapshot Clear(string id) DateTime now = NowUtc(); if (!Entries.ContainsKey(id)) { - return UloopPausePointSnapshot.NotEnabled(id, _pauseController); + return (UloopPausePointSnapshot.NotEnabled(id, _pauseController), false); } UloopPausePointEntry entry = Entries[id]; @@ -105,9 +105,14 @@ public static UloopPausePointSnapshot Clear(string id) }; entry.MarkCleared(UloopPausePointClearedReason.ExplicitClear, message); ClearHitSnapshotAndRawCaptureForId(id); - // Why always Resume: Option B does not distinguish manual vs pause-point pause. - ResumeEditorPause(); - return entry.ToSnapshot(now, _pauseController); + // Why only when pause-point-owned: a clear must not steal ownership of a manual pause + // the user set outside the pause-point workflow (control-play-mode --action Pause or + // the Editor pause button). It resumes only while a pause window is open - i.e. while + // a pause-point hit is what is holding the Editor paused. Manual pauses leave no open + // window, so they are left untouched. (Client disconnect and expiry still resume + // unconditionally: those paths must guarantee release even for a manual pause.) + bool resumedFromPause = ResumeEditorPauseIfOwnedByPausePoint(); + return (entry.ToSnapshot(now, _pauseController), resumedFromPause); } public static UloopPausePointClearAllResult ClearAll( @@ -131,13 +136,17 @@ public static UloopPausePointClearAllResult ClearAll( } ClearLatestHitSnapshot(); UloopPausePointRawCaptureHolder.Clear(); - // Why always Resume: Option B does not distinguish manual vs pause-point pause. - ResumeEditorPause(); + // Why only when pause-point-owned: like Clear, a bulk clear must not resume a manual + // pause the user set outside the pause-point workflow. It resumes only while a pause + // window is open (a pause-point hit is holding the Editor paused). Client disconnect + // and expiry still resume unconditionally to guarantee release. + bool resumedFromPause = ResumeEditorPauseIfOwnedByPausePoint(); UloopPausePointEditorStateSnapshot editorState = UloopPausePointEditorStateSnapshot.FromController( _pauseController, UloopPausePointEditorStateCapturedAt.ClearAll); - return new UloopPausePointClearAllResult(clearedIds.Count, now, editorState, clearedIds.ToArray()); + return new UloopPausePointClearAllResult( + clearedIds.Count, now, editorState, clearedIds.ToArray(), resumedFromPause); } public static UloopPausePointSnapshot GetStatus(string id) @@ -396,6 +405,28 @@ private static void ResumeEditorPause() _pauseController.Resume(); } + // Resumes the Editor only when a pause-point hit currently owns the pause (a pause window + // is open) and reports whether it did. Clear/ClearAll use this so they never resume a + // manual pause that no pause-point hit is responsible for. Disconnect and expiry paths + // deliberately call ResumeEditorPause directly instead, because they must release the + // Editor even when the pause was manual. + private static bool ResumeEditorPauseIfOwnedByPausePoint() + { + // A manual unpause (Editor pause button / control-play-mode) may not have been observed + // yet, because the external-resume sync runs on the Editor update tick. Reconcile first + // so a window left open by that unobserved unpause is credited and closed here instead + // of being misreported as a resume this clear performed (it would be a no-op resume of + // an already-unpaused Editor) and instead of leaving the stale window freezing expiry. + ClosePauseWindowIfEditorResumedExternally(); + if (!_pauseWindowStartUtc.HasValue) + { + return false; + } + + ResumeEditorPause(); + return true; + } + /// /// Closes an open pause window when the Editor was unpaused through a path that never /// calls back into this registry - control-play-mode's Play/Stop diff --git a/Packages/src/Runtime/PausePoints/UnityEditorPausePointPauseController.cs b/Packages/src/Runtime/PausePoints/UnityEditorPausePointPauseController.cs index aa83ed69be..ff79a6ced7 100644 --- a/Packages/src/Runtime/PausePoints/UnityEditorPausePointPauseController.cs +++ b/Packages/src/Runtime/PausePoints/UnityEditorPausePointPauseController.cs @@ -18,8 +18,9 @@ public void Pause() public void Resume() { - // Why unconditional: Option B clears any Editor pause on disconnect/clear/expiry, - // including a manual pause that happens to be active. + // This is the low-level unpause primitive and is intentionally unconditional; it does + // not know why the Editor is paused. The registry decides when to call it: clear only + // resumes a pause-point-owned pause, while disconnect and expiry resume unconditionally. EditorApplication.isPaused = false; } }