From 48c0155dc2c75748647923c79c7373ef5f9dff4a Mon Sep 17 00:00:00 2001 From: Masamichi Hatayama Date: Mon, 13 Jul 2026 19:20:43 +0900 Subject: [PATCH 1/2] fix: pause-point disconnect and expiry no longer leave Play Mode stuck paused (#1754) Co-authored-by: Cursor --- .../Editor/PausePointCaptureModeTests.cs | 6 ++ Assets/Tests/Editor/PausePointTests.cs | 78 +++++++++++++++++ .../Tests/Editor/PausePointToolModeTests.cs | 6 ++ .../SourcePausePointCaptureTests.cs | 6 ++ .../SourcePausePointPatcherTests.cs | 6 ++ ...oardInputSimulationResponseFactoryTests.cs | 5 ++ ...ouseInputSimulationResponseFactoryTests.cs | 5 ++ .../Tests/PlayMode/SimulateKeyboardTests.cs | 5 ++ .../Tests/PlayMode/SimulateMouseInputTests.cs | 5 ++ ...ityCliLoopBridgeClientDisconnectMonitor.cs | 4 + .../UnityCliLoopBridgeClientSessionManager.cs | 4 + .../IUloopPausePointPauseController.cs | 1 + .../PausePoints/UloopPausePointEntry.cs | 16 +++- .../UloopPausePointRawCaptureLifecycle.cs | 16 ++++ .../PausePoints/UloopPausePointRegistry.cs | 86 +++++++++++++++++-- .../UnityEditorPausePointPauseController.cs | 7 ++ 16 files changed, 247 insertions(+), 9 deletions(-) diff --git a/Assets/Tests/Editor/PausePointCaptureModeTests.cs b/Assets/Tests/Editor/PausePointCaptureModeTests.cs index 37e3102078..1c08ad9b7e 100644 --- a/Assets/Tests/Editor/PausePointCaptureModeTests.cs +++ b/Assets/Tests/Editor/PausePointCaptureModeTests.cs @@ -180,6 +180,12 @@ public void Pause() { PauseCount++; } + + public void Resume() + { + // Why zero: Unity's isPaused is a bool; Option B Resume must fully clear pause. + PauseCount = 0; + } } } } diff --git a/Assets/Tests/Editor/PausePointTests.cs b/Assets/Tests/Editor/PausePointTests.cs index a5370f42bd..eb09a7f0f1 100644 --- a/Assets/Tests/Editor/PausePointTests.cs +++ b/Assets/Tests/Editor/PausePointTests.cs @@ -285,6 +285,77 @@ public void Clear_WhenPausePointWasHit_ReportsAlreadyHitMessage() 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() + { + // Verifies Option B: Clear resumes even when the pause was left from a pause-point hit. + UloopPausePointRegistry.Enable("jump", 30); + UloopPausePoint.Pause("jump"); + Assert.That(_pauseController.IsPaused, Is.True); + + UloopPausePointRegistry.Clear("jump"); + + Assert.That(_pauseController.IsPaused, Is.False); + } + + [Test] + public void ClearAll_AfterHit_ShouldResumeEditorPause() + { + // Verifies ClearAll also resumes under Option B. + UloopPausePointRegistry.Enable("jump", 30); + UloopPausePoint.Pause("jump"); + + UloopPausePointRegistry.ClearAll(); + + Assert.That(_pauseController.IsPaused, Is.False); + } + + [Test] + public void ApplyCaptureWindowExpirations_WhenHitPastTimeout_ShouldExpireAndResume() + { + // Verifies abandoned SingleShot hits still expire at ExpiresAtUtc and resume without a Clear poll. + UloopPausePointRegistry.Enable("jump", 30); + UloopPausePoint.Pause("jump"); + _nowUtc = _nowUtc.AddSeconds(31); + + UloopPausePointRegistry.ApplyCaptureWindowExpirations(); + + UloopPausePointSnapshot status = UloopPausePointRegistry.GetStatus("jump"); + Assert.That(status.Status, Is.EqualTo(UloopPausePointStatus.Expired)); + Assert.That(_pauseController.IsPaused, Is.False); + } + + [Test] + public void ResumeEditorPauseForClientDisconnect_WhenPaused_ShouldResumeOnMainThreadApply() + { + // Verifies disconnect only arms a pending flag; main-thread apply resumes once (Option B). + UloopPausePointRegistry.Enable("jump", 30); + UloopPausePoint.Pause("jump"); + + UloopPausePointRegistry.ResumeEditorPauseForClientDisconnect(); + Assert.That(_pauseController.IsPaused, Is.True); + Assert.That(_pauseController.ResumeCount, Is.EqualTo(0)); + + UloopPausePointRegistry.ApplyPendingClientDisconnectResume(); + + Assert.That(_pauseController.IsPaused, Is.False); + Assert.That(_pauseController.ResumeCount, Is.EqualTo(1)); + } + + [Test] + public void ApplyPendingClientDisconnectResume_WhenNotPaused_ShouldDiscardPendingFlag() + { + // Verifies a disconnect request while already running does not call Resume. + Assert.That(_pauseController.IsPaused, Is.False); + + UloopPausePointRegistry.ResumeEditorPauseForClientDisconnect(); + UloopPausePointRegistry.ApplyPendingClientDisconnectResume(); + + Assert.That(_pauseController.ResumeCount, Is.EqualTo(0)); + Assert.That(_pauseController.IsPaused, Is.False); } [Test] @@ -924,12 +995,19 @@ private sealed class FakePauseController : IUloopPausePointPauseController public bool IsPlaying { get; private set; } = true; public bool IsPaused { get; private set; } public int PauseCount { get; private set; } + public int ResumeCount { get; private set; } public void Pause() { PauseCount++; IsPaused = true; } + + public void Resume() + { + ResumeCount++; + IsPaused = false; + } } } } diff --git a/Assets/Tests/Editor/PausePointToolModeTests.cs b/Assets/Tests/Editor/PausePointToolModeTests.cs index a02984b61c..d6bf0daee2 100644 --- a/Assets/Tests/Editor/PausePointToolModeTests.cs +++ b/Assets/Tests/Editor/PausePointToolModeTests.cs @@ -142,6 +142,12 @@ public void Pause() { PauseCount++; } + + public void Resume() + { + // Why zero: Unity's isPaused is a bool; Option B Resume must fully clear pause. + PauseCount = 0; + } } } } diff --git a/Assets/Tests/Editor/SourcePausePointCapture/SourcePausePointCaptureTests.cs b/Assets/Tests/Editor/SourcePausePointCapture/SourcePausePointCaptureTests.cs index 1072d4155e..bfb462f2d8 100644 --- a/Assets/Tests/Editor/SourcePausePointCapture/SourcePausePointCaptureTests.cs +++ b/Assets/Tests/Editor/SourcePausePointCapture/SourcePausePointCaptureTests.cs @@ -262,6 +262,12 @@ public void Pause() { PauseCount++; } + + public void Resume() + { + // Why zero: Unity's isPaused is a bool; Option B Resume must fully clear pause. + PauseCount = 0; + } } } } diff --git a/Assets/Tests/Editor/SourcePausePointPatcher/SourcePausePointPatcherTests.cs b/Assets/Tests/Editor/SourcePausePointPatcher/SourcePausePointPatcherTests.cs index c7e59c316e..6c5ed50f4c 100644 --- a/Assets/Tests/Editor/SourcePausePointPatcher/SourcePausePointPatcherTests.cs +++ b/Assets/Tests/Editor/SourcePausePointPatcher/SourcePausePointPatcherTests.cs @@ -477,6 +477,12 @@ public void Pause() { PauseCount++; } + + public void Resume() + { + // Why zero: Unity's isPaused is a bool; Option B Resume must fully clear pause. + PauseCount = 0; + } } } } diff --git a/Assets/Tests/PlayMode/KeyboardInputSimulationResponseFactoryTests.cs b/Assets/Tests/PlayMode/KeyboardInputSimulationResponseFactoryTests.cs index 744d8e36cf..90c19e78c0 100644 --- a/Assets/Tests/PlayMode/KeyboardInputSimulationResponseFactoryTests.cs +++ b/Assets/Tests/PlayMode/KeyboardInputSimulationResponseFactoryTests.cs @@ -124,6 +124,11 @@ public void Pause() { IsPaused = true; } + + public void Resume() + { + IsPaused = false; + } } } } diff --git a/Assets/Tests/PlayMode/MouseInputSimulationResponseFactoryTests.cs b/Assets/Tests/PlayMode/MouseInputSimulationResponseFactoryTests.cs index 16f7102ec7..e55f6229fd 100644 --- a/Assets/Tests/PlayMode/MouseInputSimulationResponseFactoryTests.cs +++ b/Assets/Tests/PlayMode/MouseInputSimulationResponseFactoryTests.cs @@ -104,6 +104,11 @@ public void Pause() { IsPaused = true; } + + public void Resume() + { + IsPaused = false; + } } } } diff --git a/Assets/Tests/PlayMode/SimulateKeyboardTests.cs b/Assets/Tests/PlayMode/SimulateKeyboardTests.cs index db4890ca8c..6f325b5673 100644 --- a/Assets/Tests/PlayMode/SimulateKeyboardTests.cs +++ b/Assets/Tests/PlayMode/SimulateKeyboardTests.cs @@ -1025,6 +1025,11 @@ public void Pause() { IsPaused = true; } + + public void Resume() + { + IsPaused = false; + } } private static BadgeVisual RequireBadgeVisual(string keyName) diff --git a/Assets/Tests/PlayMode/SimulateMouseInputTests.cs b/Assets/Tests/PlayMode/SimulateMouseInputTests.cs index 5aa7a59e37..d23e5d15d9 100644 --- a/Assets/Tests/PlayMode/SimulateMouseInputTests.cs +++ b/Assets/Tests/PlayMode/SimulateMouseInputTests.cs @@ -412,6 +412,11 @@ public void Pause() { IsPaused = true; } + + public void Resume() + { + IsPaused = false; + } } #endregion diff --git a/Packages/src/Editor/Infrastructure/UnityCliLoopBridgeClientDisconnectMonitor.cs b/Packages/src/Editor/Infrastructure/UnityCliLoopBridgeClientDisconnectMonitor.cs index f765870192..37aacab845 100644 --- a/Packages/src/Editor/Infrastructure/UnityCliLoopBridgeClientDisconnectMonitor.cs +++ b/Packages/src/Editor/Infrastructure/UnityCliLoopBridgeClientDisconnectMonitor.cs @@ -1,6 +1,7 @@ using System; using System.Threading; using System.Threading.Tasks; +using io.github.hatayama.UnityCliLoop.Runtime; namespace io.github.hatayama.UnityCliLoop.Infrastructure { @@ -22,6 +23,9 @@ internal async Task MonitorClientDisconnectAsync( { if (!client.IsConnected) { + // Why: Option B resumes on mid-request CLI disconnect so an abandoned pause + // does not leave frame-dependent tools stuck after the agent dies. + UloopPausePointRegistry.ResumeEditorPauseForClientDisconnect(); requestCancellationTokenSource.Cancel(); return; } diff --git a/Packages/src/Editor/Infrastructure/UnityCliLoopBridgeClientSessionManager.cs b/Packages/src/Editor/Infrastructure/UnityCliLoopBridgeClientSessionManager.cs index 26a42bcba9..5c2b218b83 100644 --- a/Packages/src/Editor/Infrastructure/UnityCliLoopBridgeClientSessionManager.cs +++ b/Packages/src/Editor/Infrastructure/UnityCliLoopBridgeClientSessionManager.cs @@ -9,6 +9,7 @@ using UnityEngine; using io.github.hatayama.UnityCliLoop.Application; +using io.github.hatayama.UnityCliLoop.Runtime; using io.github.hatayama.UnityCliLoop.ToolContracts; namespace io.github.hatayama.UnityCliLoop.Infrastructure @@ -86,6 +87,9 @@ internal void DisconnectAllClients() { _clientStreams.TryRemove(clientKey, out _); } + + // Why: bridge teardown must not leave Play Mode paused after clients are forced off. + UloopPausePointRegistry.ResumeEditorPauseForClientDisconnect(); } internal void StartClientHandler(BridgeClientConnection client, CancellationToken cancellationToken) diff --git a/Packages/src/Runtime/PausePoints/IUloopPausePointPauseController.cs b/Packages/src/Runtime/PausePoints/IUloopPausePointPauseController.cs index d95f1e57fe..2d593a0ee4 100644 --- a/Packages/src/Runtime/PausePoints/IUloopPausePointPauseController.cs +++ b/Packages/src/Runtime/PausePoints/IUloopPausePointPauseController.cs @@ -9,6 +9,7 @@ internal interface IUloopPausePointPauseController bool IsPlaying { get; } bool IsPaused { get; } void Pause(); + void Resume(); } } #endif diff --git a/Packages/src/Runtime/PausePoints/UloopPausePointEntry.cs b/Packages/src/Runtime/PausePoints/UloopPausePointEntry.cs index a153cfaec9..29b1afaa00 100644 --- a/Packages/src/Runtime/PausePoints/UloopPausePointEntry.cs +++ b/Packages/src/Runtime/PausePoints/UloopPausePointEntry.cs @@ -56,16 +56,23 @@ public UloopPausePointEntry( public string StatusBeforeClear { get; private set; } = string.Empty; public bool LateHitDiscardedAfterClear { get; private set; } - public void ExpireIfNeeded(DateTime nowUtc) + public bool ExpireIfNeeded(DateTime nowUtc) { - if (!IsEnabled) + if (Status == UloopPausePointStatus.Cleared || Status == UloopPausePointStatus.Expired) { - return; + return false; } if (nowUtc < ExpiresAtUtc) { - return; + return false; + } + + // Why also Hit: SingleShot disarms on hit (IsEnabled=false) but the capture window + // still ends at ExpiresAtUtc; without this, an abandoned pause after hit never expires. + if (!IsEnabled && Status != UloopPausePointStatus.Hit) + { + return false; } IsEnabled = false; @@ -73,6 +80,7 @@ public void ExpireIfNeeded(DateTime nowUtc) Message = HitCount == 0 ? "Pause point expired before it was hit." : $"Pause point capture window expired after {HitCount} hit(s); capture history is preserved."; + return true; } public void MarkCleared(string clearedReason, string message = "Pause point cleared.") diff --git a/Packages/src/Runtime/PausePoints/UloopPausePointRawCaptureLifecycle.cs b/Packages/src/Runtime/PausePoints/UloopPausePointRawCaptureLifecycle.cs index eaed9432dc..835a8b9fc8 100644 --- a/Packages/src/Runtime/PausePoints/UloopPausePointRawCaptureLifecycle.cs +++ b/Packages/src/Runtime/PausePoints/UloopPausePointRawCaptureLifecycle.cs @@ -13,6 +13,22 @@ static UloopPausePointRawCaptureLifecycle() { EditorApplication.pauseStateChanged += OnPauseStateChanged; EditorApplication.playModeStateChanged += OnPlayModeStateChanged; + EditorApplication.update += OnEditorUpdate; + } + + private static void OnEditorUpdate() + { + // Why before the isPaused gate: disconnect may request resume while already unpaused; + // the pending flag must still be consumed on the main thread. + UloopPausePointRegistry.ApplyPendingClientDisconnectResume(); + + if (!EditorApplication.isPaused) + { + return; + } + + // Why while paused only: abandoned Hit windows must expire without a CLI poll. + UloopPausePointRegistry.ApplyCaptureWindowExpirations(); } private static void OnPauseStateChanged(PauseState pauseState) diff --git a/Packages/src/Runtime/PausePoints/UloopPausePointRegistry.cs b/Packages/src/Runtime/PausePoints/UloopPausePointRegistry.cs index ab8491c443..f543912721 100644 --- a/Packages/src/Runtime/PausePoints/UloopPausePointRegistry.cs +++ b/Packages/src/Runtime/PausePoints/UloopPausePointRegistry.cs @@ -2,15 +2,16 @@ using System; using System.Collections.Concurrent; using System.Collections.Generic; +using System.Threading; using UnityEngine; namespace io.github.hatayama.UnityCliLoop.Runtime { /// /// Stores enabled pause point state for the current Editor domain. All members except - /// IsArmed are main-thread-only by convention; IsArmed is the one entry point an - /// off-main-thread Harmony-injected Capture call may reach, so Entries is a - /// ConcurrentDictionary to make that cross-thread read safe. + /// IsArmed and ResumeEditorPauseForClientDisconnect are main-thread-only by convention; + /// IsArmed is the Harmony Capture entry point, and the disconnect resume path only sets a + /// pending flag so thread-pool callers never touch EditorApplication.isPaused. /// internal static class UloopPausePointRegistry { @@ -23,6 +24,8 @@ internal static class UloopPausePointRegistry private static Func _nowProvider = () => DateTime.UtcNow; private static int _nextGeneration; private static int _nextHitSequence; + // Why Interlocked: disconnect monitor / DisconnectAllClients run on the thread pool. + private static int _pendingClientDisconnectResume; private static UloopPausePointSnapshot _latestHitSnapshot; // One input can hit several markers in the same frame; tools need the full list, // not just the latest hit, to report every marker that interrupted them. @@ -90,6 +93,8 @@ 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); } @@ -114,6 +119,8 @@ public static UloopPausePointClearAllResult ClearAll( } ClearLatestHitSnapshot(); UloopPausePointRawCaptureHolder.Clear(); + // Why always Resume: Option B does not distinguish manual vs pause-point pause. + ResumeEditorPause(); UloopPausePointEditorStateSnapshot editorState = UloopPausePointEditorStateSnapshot.FromController( _pauseController, @@ -132,7 +139,11 @@ public static UloopPausePointSnapshot GetStatus(string id) } UloopPausePointEntry entry = Entries[id]; - entry.ExpireIfNeeded(now); + if (entry.ExpireIfNeeded(now)) + { + ResumeEditorPause(); + } + return entry.ToSnapshot(now, _pauseController); } @@ -192,7 +203,12 @@ private static UloopPausePointSnapshot HitCore( } UloopPausePointEntry entry = Entries[id]; - entry.ExpireIfNeeded(now); + if (entry.ExpireIfNeeded(now)) + { + ResumeEditorPause(); + return entry.ToSnapshot(now, _pauseController); + } + if (!entry.IsEnabled) { // Why: delayed main-thread hits can lose the race to Clear/ClearAll; surface that race. @@ -247,6 +263,65 @@ public static void ClearLatestHitSnapshot() UloopPausePointRawCaptureHolder.Clear(); } + /// + /// Expires capture windows that have elapsed and resumes the Editor when any expire. + /// Used while paused so expiry still runs without a CLI status poll. + /// + public static void ApplyCaptureWindowExpirations() + { + DateTime now = NowUtc(); + bool anyExpired = false; + foreach (UloopPausePointEntry entry in Entries.Values) + { + if (entry.ExpireIfNeeded(now)) + { + anyExpired = true; + } + } + + if (anyExpired) + { + ResumeEditorPause(); + } + } + + /// + /// Requests Editor resume when a CLI client drops mid-request or the bridge disconnects all clients. + /// Why flag only: callers run on the thread pool where Unity Editor APIs are unsafe; the main-thread + /// Editor update pump applies the resume via ApplyPendingClientDisconnectResume. + /// Why not on every short-lived command close: that would resume immediately after await-pause-point + /// returns a Hit and break the paused inspection workflow. + /// + public static void ResumeEditorPauseForClientDisconnect() + { + Interlocked.Exchange(ref _pendingClientDisconnectResume, 1); + } + + /// + /// Consumes a pending client-disconnect resume on the Editor main thread. + /// + public static void ApplyPendingClientDisconnectResume() + { + if (Interlocked.Exchange(ref _pendingClientDisconnectResume, 0) == 0) + { + return; + } + + // Why discard when already running: Option B still clears a stale request without + // calling Resume on an Editor that is not paused. + if (!_pauseController.IsPaused) + { + return; + } + + ResumeEditorPause(); + } + + private static void ResumeEditorPause() + { + _pauseController.Resume(); + } + // Drops the id's own hit-history entry and, if it currently owns the latest hit, that // pointer too - but never touches the raw capture holder. Enable() uses this so a same-id // re-enable while paused only resets the entry's generation bookkeeping. @@ -290,6 +365,7 @@ public static void ResetForTests() _nextHitSequence = 0; _latestHitSnapshot = null; _hitSnapshots.Clear(); + Interlocked.Exchange(ref _pendingClientDisconnectResume, 0); _pauseController = new UnityEditorPausePointPauseController(); _nowProvider = () => DateTime.UtcNow; UloopPausePointRawCaptureHolder.Clear(); diff --git a/Packages/src/Runtime/PausePoints/UnityEditorPausePointPauseController.cs b/Packages/src/Runtime/PausePoints/UnityEditorPausePointPauseController.cs index cadaa13071..aa83ed69be 100644 --- a/Packages/src/Runtime/PausePoints/UnityEditorPausePointPauseController.cs +++ b/Packages/src/Runtime/PausePoints/UnityEditorPausePointPauseController.cs @@ -15,6 +15,13 @@ public void Pause() { EditorApplication.isPaused = true; } + + public void Resume() + { + // Why unconditional: Option B clears any Editor pause on disconnect/clear/expiry, + // including a manual pause that happens to be active. + EditorApplication.isPaused = false; + } } } #endif From b9a62166dc682ef0c3985108911549e19f11f723 Mon Sep 17 00:00:00 2001 From: Masamichi Hatayama Date: Mon, 13 Jul 2026 20:08:48 +0900 Subject: [PATCH 2/2] fix: quietly save dirty Scenes before CLI Play Mode start (#1755) Co-authored-by: Cursor --- .../Editor/ControlPlayModeUseCaseTests.cs | 81 +++++++++ .../EditorUnsavedChangesQuietSaver.cs | 141 ++++++++++++++++ .../EditorUnsavedChangesQuietSaver.cs.meta | 11 ++ .../IEditorUnsavedChangesQuietSaver.cs | 16 ++ .../IEditorUnsavedChangesQuietSaver.cs.meta | 11 ++ .../ControlPlayMode/ControlPlayModeUseCase.cs | 56 ++++++- ...stPartyTools.ControlPlayMode.Editor.asmdef | 1 + .../TestExecutionStateValidationService.cs | 158 +++--------------- 8 files changed, 338 insertions(+), 137 deletions(-) create mode 100644 Packages/src/Editor/FirstPartyTools/Common/EditorUtility/EditorUnsavedChangesQuietSaver.cs create mode 100644 Packages/src/Editor/FirstPartyTools/Common/EditorUtility/EditorUnsavedChangesQuietSaver.cs.meta create mode 100644 Packages/src/Editor/FirstPartyTools/Common/EditorUtility/IEditorUnsavedChangesQuietSaver.cs create mode 100644 Packages/src/Editor/FirstPartyTools/Common/EditorUtility/IEditorUnsavedChangesQuietSaver.cs.meta diff --git a/Assets/Tests/Editor/ControlPlayModeUseCaseTests.cs b/Assets/Tests/Editor/ControlPlayModeUseCaseTests.cs index 28f4a10172..70ee2358b4 100644 --- a/Assets/Tests/Editor/ControlPlayModeUseCaseTests.cs +++ b/Assets/Tests/Editor/ControlPlayModeUseCaseTests.cs @@ -260,6 +260,60 @@ public async Task ExecuteAsync_WhenPlayBlockedWithoutSavedErrors_ReturnsEmptyDia Assert.That(response.Message, Does.Contain("no saved compiler diagnostics")); } + [Test] + public async Task ExecuteAsync_WhenPlayStartSaveFails_DoesNotEnterPlayMode() + { + // Verifies dirty Scene/Prefab save failure blocks Edit→Play instead of prompting or hanging. + Assert.That(EditorApplication.isPlaying, Is.False); + StubEditorUnsavedChangesQuietSaver quietSaver = new( + saveFailures: new[] { "Scene: Assets/Scenes/Sample.unity" }, + remainingAfterSave: System.Array.Empty()); + ControlPlayModeUseCase useCase = new ControlPlayModeUseCase( + new StubCompilationFailureProvider(System.Array.Empty()), + new StubCompilationFailureGate(false), + quietSaver); + ControlPlayModeSchema schema = new ControlPlayModeSchema + { + Action = PlayModeAction.Play, + }; + + ControlPlayModeResponse response = await useCase.ExecuteAsync(schema, CancellationToken.None); + + Assert.That(quietSaver.SaveCallCount, Is.EqualTo(1)); + Assert.That(EditorApplication.isPlaying, Is.False); + Assert.That(response.Changed, Is.False); + Assert.That(response.IsPlaying, Is.False); + Assert.That(response.Message, Does.Contain("could not be saved")); + Assert.That(response.Message, Does.Contain("Scene: Assets/Scenes/Sample.unity")); + } + + [Test] + public async Task ExecuteAsync_WhenPlayStartLeavesUnsavedChanges_DoesNotEnterPlayMode() + { + // Verifies remaining dirty editor state after a quiet save still blocks Play start. + Assert.That(EditorApplication.isPlaying, Is.False); + StubEditorUnsavedChangesQuietSaver quietSaver = new( + saveFailures: System.Array.Empty(), + remainingAfterSave: new[] { "Prefab Stage: Assets/Prefabs/Hud.prefab" }); + ControlPlayModeUseCase useCase = new ControlPlayModeUseCase( + new StubCompilationFailureProvider(System.Array.Empty()), + new StubCompilationFailureGate(false), + quietSaver); + ControlPlayModeSchema schema = new ControlPlayModeSchema + { + Action = PlayModeAction.Play, + }; + + ControlPlayModeResponse response = await useCase.ExecuteAsync(schema, CancellationToken.None); + + Assert.That(quietSaver.SaveCallCount, Is.EqualTo(1)); + Assert.That(quietSaver.DetectCallCount, Is.EqualTo(1)); + Assert.That(EditorApplication.isPlaying, Is.False); + Assert.That(response.Changed, Is.False); + Assert.That(response.Message, Does.Contain("unsaved scene or prefab changes")); + Assert.That(response.Message, Does.Contain("Prefab Stage: Assets/Prefabs/Hud.prefab")); + } + private sealed class StubCompilationFailureProvider : IControlPlayModeCompilationFailureProvider { private readonly ControlPlayModeCompileError[] _errors; @@ -289,5 +343,32 @@ public bool HasScriptCompilationFailed() return _hasScriptCompilationFailed; } } + + private sealed class StubEditorUnsavedChangesQuietSaver : IEditorUnsavedChangesQuietSaver + { + private readonly string[] _saveFailures; + private readonly string[] _remainingAfterSave; + + public int SaveCallCount { get; private set; } + public int DetectCallCount { get; private set; } + + public StubEditorUnsavedChangesQuietSaver(string[] saveFailures, string[] remainingAfterSave) + { + _saveFailures = saveFailures; + _remainingAfterSave = remainingAfterSave; + } + + public string[] DetectUnsavedEditorChanges() + { + DetectCallCount++; + return _remainingAfterSave; + } + + public string[] SaveUnsavedEditorChanges() + { + SaveCallCount++; + return _saveFailures; + } + } } } diff --git a/Packages/src/Editor/FirstPartyTools/Common/EditorUtility/EditorUnsavedChangesQuietSaver.cs b/Packages/src/Editor/FirstPartyTools/Common/EditorUtility/EditorUnsavedChangesQuietSaver.cs new file mode 100644 index 0000000000..cc0ebeed56 --- /dev/null +++ b/Packages/src/Editor/FirstPartyTools/Common/EditorUtility/EditorUnsavedChangesQuietSaver.cs @@ -0,0 +1,141 @@ +using System.Collections.Generic; +using UnityEditor; +using UnityEditor.SceneManagement; +using UnityEngine; +using UnityEngine.SceneManagement; + +namespace io.github.hatayama.UnityCliLoop.FirstPartyTools +{ + /// + /// Quietly saves dirty loaded Scenes and the current Prefab Stage without prompting the user. + /// Shared by run-tests and control-play-mode so Play Mode start and test runs use the same path. + /// + public sealed class EditorUnsavedChangesQuietSaver : IEditorUnsavedChangesQuietSaver + { + public string[] DetectUnsavedEditorChanges() + { + List unsavedEditorChanges = new(); + AddDirtyLoadedScenes(unsavedEditorChanges); + AddDirtyPrefabStage(unsavedEditorChanges); + return unsavedEditorChanges.ToArray(); + } + + public string[] SaveUnsavedEditorChanges() + { + List failedChanges = new(); + SaveDirtyLoadedScenes(failedChanges); + SaveDirtyPrefabStage(failedChanges); + return failedChanges.ToArray(); + } + + private static void AddDirtyLoadedScenes(List unsavedEditorChanges) + { + Debug.Assert(unsavedEditorChanges != null, "unsavedEditorChanges must not be null"); + + for (int i = 0; i < SceneManager.sceneCount; i++) + { + Scene scene = SceneManager.GetSceneAt(i); + if (!scene.IsValid() || !scene.isLoaded || !scene.isDirty) + { + continue; + } + + unsavedEditorChanges.Add("Scene: " + GetSceneDisplayPath(scene)); + } + } + + private static void SaveDirtyLoadedScenes(List failedChanges) + { + Debug.Assert(failedChanges != null, "failedChanges must not be null"); + + for (int i = 0; i < SceneManager.sceneCount; i++) + { + Scene scene = SceneManager.GetSceneAt(i); + if (!scene.IsValid() || !scene.isLoaded || !scene.isDirty) + { + continue; + } + + if (string.IsNullOrEmpty(scene.path) || !EditorSceneManager.SaveScene(scene)) + { + failedChanges.Add("Scene: " + GetSceneDisplayPath(scene)); + } + } + } + + private static void AddDirtyPrefabStage(List unsavedEditorChanges) + { + Debug.Assert(unsavedEditorChanges != null, "unsavedEditorChanges must not be null"); + + PrefabStage prefabStage = PrefabStageUtility.GetCurrentPrefabStage(); + if (prefabStage == null || !prefabStage.scene.IsValid() || !prefabStage.scene.isDirty) + { + return; + } + + unsavedEditorChanges.Add("Prefab Stage: " + GetPrefabStageDisplayPath(prefabStage)); + } + + private static void SaveDirtyPrefabStage(List failedChanges) + { + Debug.Assert(failedChanges != null, "failedChanges must not be null"); + + PrefabStage prefabStage = PrefabStageUtility.GetCurrentPrefabStage(); + if (prefabStage == null || !prefabStage.scene.IsValid() || !prefabStage.scene.isDirty) + { + return; + } + + if (!SavePrefabStage(prefabStage)) + { + failedChanges.Add("Prefab Stage: " + GetPrefabStageDisplayPath(prefabStage)); + } + } + + private static bool SavePrefabStage(PrefabStage prefabStage) + { + Debug.Assert(prefabStage != null, "prefabStage must not be null"); + + if (string.IsNullOrEmpty(prefabStage.assetPath)) + { + return false; + } + + bool success; + PrefabUtility.SaveAsPrefabAsset(prefabStage.prefabContentsRoot, prefabStage.assetPath, out success); + if (success) + { + prefabStage.ClearDirtiness(); + } + + return success; + } + + private static string GetSceneDisplayPath(Scene scene) + { + if (!string.IsNullOrEmpty(scene.path)) + { + return scene.path; + } + + if (!string.IsNullOrEmpty(scene.name)) + { + return scene.name; + } + + return "Untitled scene"; + } + + private static string GetPrefabStageDisplayPath(PrefabStage prefabStage) + { + Debug.Assert(prefabStage != null, "prefabStage must not be null"); + + if (!string.IsNullOrEmpty(prefabStage.assetPath)) + { + return prefabStage.assetPath; + } + + return GetSceneDisplayPath(prefabStage.scene); + } + } +} diff --git a/Packages/src/Editor/FirstPartyTools/Common/EditorUtility/EditorUnsavedChangesQuietSaver.cs.meta b/Packages/src/Editor/FirstPartyTools/Common/EditorUtility/EditorUnsavedChangesQuietSaver.cs.meta new file mode 100644 index 0000000000..fb18f672b5 --- /dev/null +++ b/Packages/src/Editor/FirstPartyTools/Common/EditorUtility/EditorUnsavedChangesQuietSaver.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: 780f216b4d59344fe81ae313c7991353 +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: diff --git a/Packages/src/Editor/FirstPartyTools/Common/EditorUtility/IEditorUnsavedChangesQuietSaver.cs b/Packages/src/Editor/FirstPartyTools/Common/EditorUtility/IEditorUnsavedChangesQuietSaver.cs new file mode 100644 index 0000000000..a5274ae2d1 --- /dev/null +++ b/Packages/src/Editor/FirstPartyTools/Common/EditorUtility/IEditorUnsavedChangesQuietSaver.cs @@ -0,0 +1,16 @@ +namespace io.github.hatayama.UnityCliLoop.FirstPartyTools +{ + /// + /// Detects and quietly saves dirty Editor Scene / Prefab Stage state without user prompts. + /// + public interface IEditorUnsavedChangesQuietSaver + { + string[] DetectUnsavedEditorChanges(); + + /// + /// Saves dirty loaded Scenes and the current Prefab Stage. + /// Returns display paths that could not be saved; empty when every dirty item saved. + /// + string[] SaveUnsavedEditorChanges(); + } +} diff --git a/Packages/src/Editor/FirstPartyTools/Common/EditorUtility/IEditorUnsavedChangesQuietSaver.cs.meta b/Packages/src/Editor/FirstPartyTools/Common/EditorUtility/IEditorUnsavedChangesQuietSaver.cs.meta new file mode 100644 index 0000000000..0f1fb96265 --- /dev/null +++ b/Packages/src/Editor/FirstPartyTools/Common/EditorUtility/IEditorUnsavedChangesQuietSaver.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: c814c807e8042425899f1100d619e415 +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: diff --git a/Packages/src/Editor/FirstPartyTools/ControlPlayMode/ControlPlayModeUseCase.cs b/Packages/src/Editor/FirstPartyTools/ControlPlayMode/ControlPlayModeUseCase.cs index fec9689a89..94e3996f9f 100644 --- a/Packages/src/Editor/FirstPartyTools/ControlPlayMode/ControlPlayModeUseCase.cs +++ b/Packages/src/Editor/FirstPartyTools/ControlPlayMode/ControlPlayModeUseCase.cs @@ -2,6 +2,7 @@ using System.Threading; using System.Threading.Tasks; using UnityEditor; +using UnityEngine; namespace io.github.hatayama.UnityCliLoop.FirstPartyTools { @@ -12,17 +13,26 @@ public class ControlPlayModeUseCase { public const int DefaultTimeoutSeconds = 180; + private const string UnsavedEditorChangesSaveFailureMessage = + "Play mode could not start because unsaved scene or prefab changes could not be saved."; + private const string UnsavedEditorChangesRemainingFailureMessage = + "Play mode could not start while the editor has unsaved scene or prefab changes."; + private readonly IControlPlayModeCompilationFailureProvider _compilationFailureProvider; private readonly IControlPlayModeCompilationFailureGate _compilationFailureGate; + private readonly IEditorUnsavedChangesQuietSaver _unsavedChangesQuietSaver; public ControlPlayModeUseCase( IControlPlayModeCompilationFailureProvider compilationFailureProvider = null, - IControlPlayModeCompilationFailureGate compilationFailureGate = null) + IControlPlayModeCompilationFailureGate compilationFailureGate = null, + IEditorUnsavedChangesQuietSaver unsavedChangesQuietSaver = null) { _compilationFailureProvider = compilationFailureProvider ?? ControlPlayModeServices.CompilationFailureProvider; _compilationFailureGate = compilationFailureGate ?? ControlPlayModeServices.CompilationFailureGate; + _unsavedChangesQuietSaver = + unsavedChangesQuietSaver ?? new EditorUnsavedChangesQuietSaver(); } public Task ExecuteAsync(ControlPlayModeSchema parameters, CancellationToken ct) @@ -108,6 +118,17 @@ private ControlPlayModeActionResult ExecutePlayModeStart(bool wasPaused, bool wa true); } + // Why only when entering Play from Edit: SaveScene does not work while already playing, + // and resume-from-pause must not rewrite Scene assets. + if (!wasPlaying) + { + ControlPlayModeActionResult saveResult = SaveDirtyEditorChangesBeforePlayStart(); + if (saveResult.HasResponse) + { + return saveResult; + } + } + if (wasPaused) { EditorApplication.isPaused = false; @@ -124,6 +145,29 @@ private ControlPlayModeActionResult ExecutePlayModeStart(bool wasPaused, bool wa return ControlPlayModeActionResult.FromState(message, changed, false); } + private ControlPlayModeActionResult SaveDirtyEditorChangesBeforePlayStart() + { + string[] failedChanges = _unsavedChangesQuietSaver.SaveUnsavedEditorChanges(); + Debug.Assert(failedChanges != null, "Unsaved editor change save must return an array"); + if (failedChanges.Length > 0) + { + return ControlPlayModeActionResult.FromResponse( + CreateSaveFailedResponse(UnsavedEditorChangesSaveFailureMessage, failedChanges), + false); + } + + string[] remainingChanges = _unsavedChangesQuietSaver.DetectUnsavedEditorChanges(); + Debug.Assert(remainingChanges != null, "Unsaved editor change detection must return an array"); + if (remainingChanges.Length > 0) + { + return ControlPlayModeActionResult.FromResponse( + CreateSaveFailedResponse(UnsavedEditorChangesRemainingFailureMessage, remainingChanges), + false); + } + + return ControlPlayModeActionResult.FromState(string.Empty, false, false); + } + private static ControlPlayModeActionResult ExecutePlayModeStop(bool wasPaused, bool wasPlaying) { bool wasAlreadyStopped = !wasPlaying; @@ -172,6 +216,16 @@ private static ControlPlayModeResponse CreateResponse(string message, bool chang return response; } + private static ControlPlayModeResponse CreateSaveFailedResponse(string messagePrefix, string[] failedChanges) + { + Debug.Assert(!string.IsNullOrEmpty(messagePrefix), "messagePrefix must not be null or empty"); + Debug.Assert(failedChanges != null, "failedChanges must not be null"); + Debug.Assert(failedChanges.Length > 0, "failedChanges must not be empty"); + + string message = messagePrefix + " Unsaved changes: " + string.Join(", ", failedChanges); + return CreateResponse(message, false, false); + } + private static ControlPlayModeResponse CreateCompileErrorBlockedResponse( ControlPlayModeCompileError[] compileErrors) { diff --git a/Packages/src/Editor/FirstPartyTools/ControlPlayMode/UnityCLILoop.FirstPartyTools.ControlPlayMode.Editor.asmdef b/Packages/src/Editor/FirstPartyTools/ControlPlayMode/UnityCLILoop.FirstPartyTools.ControlPlayMode.Editor.asmdef index 98148f3f0e..ab77309c05 100644 --- a/Packages/src/Editor/FirstPartyTools/ControlPlayMode/UnityCLILoop.FirstPartyTools.ControlPlayMode.Editor.asmdef +++ b/Packages/src/Editor/FirstPartyTools/ControlPlayMode/UnityCLILoop.FirstPartyTools.ControlPlayMode.Editor.asmdef @@ -2,6 +2,7 @@ "name": "UnityCLILoop.FirstPartyTools.ControlPlayMode.Editor", "rootNamespace": "io.github.hatayama.UnityCliLoop.FirstPartyTools", "references": [ + "GUID:d427b32aad9cb44fc8e962437c9dbcd8", "GUID:fc3fd32eddbee40e39c2d76dc184957b" ], "includePlatforms": [ diff --git a/Packages/src/Editor/FirstPartyTools/RunTests/TestExecutionStateValidationService.cs b/Packages/src/Editor/FirstPartyTools/RunTests/TestExecutionStateValidationService.cs index b9bf539bb0..586be598c8 100644 --- a/Packages/src/Editor/FirstPartyTools/RunTests/TestExecutionStateValidationService.cs +++ b/Packages/src/Editor/FirstPartyTools/RunTests/TestExecutionStateValidationService.cs @@ -1,8 +1,5 @@ -using System.Collections.Generic; using UnityEditor; -using UnityEditor.SceneManagement; using UnityEngine; -using UnityEngine.SceneManagement; using io.github.hatayama.UnityCliLoop.ToolContracts; @@ -24,17 +21,37 @@ public class TestExecutionStateValidationService private const string UnsavedEditorChangesSaveFailureMessage = "Tests cannot save unsaved scene or prefab changes before running tests."; + private readonly IEditorUnsavedChangesQuietSaver _unsavedChangesQuietSaver; + + public TestExecutionStateValidationService() + : this(new EditorUnsavedChangesQuietSaver()) + { + } + + public TestExecutionStateValidationService(IEditorUnsavedChangesQuietSaver unsavedChangesQuietSaver) + { + Debug.Assert(unsavedChangesQuietSaver != null, "unsavedChangesQuietSaver must not be null"); + _unsavedChangesQuietSaver = unsavedChangesQuietSaver; + } + protected virtual bool IsPlaying => EditorApplication.isPlaying; protected virtual bool IsPaused => EditorApplication.isPaused; protected virtual bool IsCompiling => EditorApplication.isCompiling; protected virtual bool IsUpdating => EditorApplication.isUpdating; protected virtual string[] DetectUnsavedEditorChanges() { - return DetectCurrentUnsavedEditorChanges(); + return _unsavedChangesQuietSaver.DetectUnsavedEditorChanges(); } protected virtual ValidationResult SaveUnsavedEditorChanges() { - return SaveCurrentUnsavedEditorChanges(); + string[] failedChanges = _unsavedChangesQuietSaver.SaveUnsavedEditorChanges(); + Debug.Assert(failedChanges != null, "Unsaved editor change save must return an array"); + if (failedChanges.Length > 0) + { + return ValidationResult.Failure(CreateUnsavedEditorChangesSaveFailureMessage(failedChanges)); + } + + return ValidationResult.Success(); } public virtual ValidationResult Validate(UnityCliLoopTestMode testMode, bool saveBeforeRun) @@ -78,137 +95,6 @@ public virtual ValidationResult Validate(UnityCliLoopTestMode testMode, bool sav return ValidationResult.Success(); } - private static string[] DetectCurrentUnsavedEditorChanges() - { - List unsavedEditorChanges = new(); - AddDirtyLoadedScenes(unsavedEditorChanges); - AddDirtyPrefabStage(unsavedEditorChanges); - return unsavedEditorChanges.ToArray(); - } - - private static ValidationResult SaveCurrentUnsavedEditorChanges() - { - List failedChanges = new(); - SaveDirtyLoadedScenes(failedChanges); - SaveDirtyPrefabStage(failedChanges); - if (failedChanges.Count > 0) - { - return ValidationResult.Failure(CreateUnsavedEditorChangesSaveFailureMessage(failedChanges.ToArray())); - } - - return ValidationResult.Success(); - } - - private static void AddDirtyLoadedScenes(List unsavedEditorChanges) - { - Debug.Assert(unsavedEditorChanges != null, "unsavedEditorChanges must not be null"); - - for (int i = 0; i < SceneManager.sceneCount; i++) - { - Scene scene = SceneManager.GetSceneAt(i); - if (!scene.IsValid() || !scene.isLoaded || !scene.isDirty) - { - continue; - } - - unsavedEditorChanges.Add("Scene: " + GetSceneDisplayPath(scene)); - } - } - - private static void SaveDirtyLoadedScenes(List failedChanges) - { - Debug.Assert(failedChanges != null, "failedChanges must not be null"); - - for (int i = 0; i < SceneManager.sceneCount; i++) - { - Scene scene = SceneManager.GetSceneAt(i); - if (!scene.IsValid() || !scene.isLoaded || !scene.isDirty) - { - continue; - } - - if (string.IsNullOrEmpty(scene.path) || !EditorSceneManager.SaveScene(scene)) - { - failedChanges.Add("Scene: " + GetSceneDisplayPath(scene)); - } - } - } - - private static void AddDirtyPrefabStage(List unsavedEditorChanges) - { - Debug.Assert(unsavedEditorChanges != null, "unsavedEditorChanges must not be null"); - - PrefabStage prefabStage = PrefabStageUtility.GetCurrentPrefabStage(); - if (prefabStage == null || !prefabStage.scene.IsValid() || !prefabStage.scene.isDirty) - { - return; - } - - unsavedEditorChanges.Add("Prefab Stage: " + GetPrefabStageDisplayPath(prefabStage)); - } - - private static void SaveDirtyPrefabStage(List failedChanges) - { - Debug.Assert(failedChanges != null, "failedChanges must not be null"); - - PrefabStage prefabStage = PrefabStageUtility.GetCurrentPrefabStage(); - if (prefabStage == null || !prefabStage.scene.IsValid() || !prefabStage.scene.isDirty) - { - return; - } - - if (!SavePrefabStage(prefabStage)) - { - failedChanges.Add("Prefab Stage: " + GetPrefabStageDisplayPath(prefabStage)); - } - } - - private static bool SavePrefabStage(PrefabStage prefabStage) - { - Debug.Assert(prefabStage != null, "prefabStage must not be null"); - - if (string.IsNullOrEmpty(prefabStage.assetPath)) - { - return false; - } - - bool success; - PrefabUtility.SaveAsPrefabAsset(prefabStage.prefabContentsRoot, prefabStage.assetPath, out success); - if (success) - { - prefabStage.ClearDirtiness(); - } - - return success; - } - - private static string GetSceneDisplayPath(Scene scene) - { - if (!string.IsNullOrEmpty(scene.path)) - { - return scene.path; - } - - if (!string.IsNullOrEmpty(scene.name)) - { - return scene.name; - } - - return "Untitled scene"; - } - - private static string GetPrefabStageDisplayPath(PrefabStage prefabStage) - { - Debug.Assert(prefabStage != null, "prefabStage must not be null"); - - if (!string.IsNullOrEmpty(prefabStage.assetPath)) - { - return prefabStage.assetPath; - } - - return GetSceneDisplayPath(prefabStage.scene); - } - private static string CreateUnsavedEditorChangesFailureMessage(string[] unsavedEditorChanges) { Debug.Assert(unsavedEditorChanges != null, "unsavedEditorChanges must not be null");