feat: Scope clear-pause-point resume to pause-point-owned pauses#1910
Conversation
clear-pause-point previously called ResumeEditorPause() unconditionally, so clearing a marker also resumed a manual pause the user had set outside the pause-point workflow (control-play-mode --action Pause or the Editor pause button). This lost the user's manual pause state. Clear/ClearAll now resume only while a pause window is open, i.e. only when a pause-point hit is what is holding the Editor paused. A manual pause leaves no open window and is left untouched. Client-disconnect and expiry paths still resume unconditionally, because those must guarantee release of the Editor even when the pause was manual. The registry reports whether it actually resumed (Clear returns a tuple, ClearAll adds ResumedFromPause to its result DTO), and the clear-pause-point tool response surfaces a warning when the clear resumed Play Mode as a side effect. Rewrote the stale "Option B" comments accordingly.
|
Warning Review limit reached
Next review available in: 46 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughPause-point clear operations now return whether they resumed a pause-point-owned editor pause. Tool responses expose a warning for that case, while manual pauses remain active. Runtime result types, callers, and editor tests were updated for the new contracts. ChangesPause-point clear flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant PausePointTool
participant UloopPausePointRegistry
participant UnityEditorPausePointPauseController
PausePointTool->>UloopPausePointRegistry: Clear or ClearAll
UloopPausePointRegistry->>UnityEditorPausePointPauseController: Resume only for owned pause
UnityEditorPausePointPauseController-->>UloopPausePointRegistry: Pause state updated
UloopPausePointRegistry-->>PausePointTool: Snapshot and resumedFromPause
PausePointTool-->>PausePointTool: Set clear warning when resumed
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ResumeEditorPauseIfOwnedByPausePoint checked only _pauseWindowStartUtc, which is closed by ClosePauseWindowIfEditorResumedExternally on the Editor update tick. If a hit opened the window and the Editor was then unpaused manually (pause button / control-play-mode) before that tick ran, a clear in between would report ResumedFromPause=true even though Resume is a no-op on an already-unpaused Editor, emitting a false "resumed Play Mode" warning, and the stale window would keep freezing every marker's expiry countdown. Reconcile via ClosePauseWindowIfEditorResumedExternally at the start of the helper so the stale window is credited and closed, and the clear correctly reports it did not resume. Covers both Clear and ClearAll (shared path).
570f18a
into
feature/round6-pause-point-improvements
Summary
Round-6 pause-point improvements, PR-1 of 5. Base branch:
feature/round6-pause-point-improvements(the Round-6 integration branch), notv3-beta.clear-pause-point(both--idand--all) previously calledResumeEditorPause()unconditionally. Clearing a marker therefore also resumed a manual pause the user had set outside the pause-point workflow (control-play-mode --action Pause, or the Editor pause button), discarding their run state.This scopes the resume so a clear only ever resumes a pause-point-owned pause.
Changes
UloopPausePointRegistry:Clear/ClearAllnow resume only while a pause window is open (_pauseWindowStartUtc.HasValue) — i.e. only when a pause-point hit is what is holding the Editor paused. A manual pause leaves no open window and is left untouched. New private helperResumeEditorPauseIfOwnedByPausePoint()centralizes the decision and reports whether it resumed.ApplyPendingClientDisconnectResumeandApplyCaptureWindowExpirationsstill resume unconditionally, because they must guarantee release of the Editor even when the pause was manual.out/ref, per repo convention):Clearreturns a tuple(UloopPausePointSnapshot Snapshot, bool ResumedFromPause);UloopPausePointClearAllResultgains aResumedFromPausefield.clear-pause-pointtool response sets a newSourcePausePointConstants.ClearResumedPlayModeWarningwhen the clear actually resumed Play Mode."Why always Resume: Option B ..."comments and theUnityEditorPausePointPauseController.Resume()comment to match the new design.Tests
Added EditMode coverage in
PausePointTests(via the pure-C#FakePauseController, newPauseExternally()seam):ResumedFromPause=truewhen a hit owns the pause.ResumeCount==0) and reportResumedFromPause=false.clear-pause-point/--alltool responses carry the resume warning when they resume, and no warning when a manual pause is preserved.Existing disconnect/expiry tests are unchanged and still assert unconditional resume.
Validation
uloop compile: 0 errors, 0 warnings.uloop run-tests(EditMode,.*PausePoint.*): 222 passed, 0 failed.Skill files, version fields, changelogs, and
protocolVersionare intentionally untouched (skill wording is handled separately in PR-5).