From 14208da535f109eec1d49dcddbbdca92064e95bf Mon Sep 17 00:00:00 2001 From: hatayama Date: Thu, 20 Aug 2026 12:04:04 +0900 Subject: [PATCH 1/2] feat: warn at enable time when a trace marker sits in a per-frame Unity message Trace on Update/FixedUpdate/LateUpdate/OnGUI filled history before testers saw a status, and docs only recommended conditional lines after the fact. The enable notice states that fact without claiming overflow will happen. Co-authored-by: Cursor --- .../PausePointPerFrameTraceNoticeFixture.cs | 13 ++ ...usePointPerFrameTraceNoticeFixture.cs.meta | 11 + .../PausePointPerFrameTraceNoticeTests.cs | 193 ++++++++++++++++++ ...PausePointPerFrameTraceNoticeTests.cs.meta | 11 + .../PausePoint/PausePointEnableWarnings.cs | 74 +++++++ .../PausePoint/PausePointUseCase.cs | 6 + .../PausePoint/SourcePausePointConstants.cs | 6 + 7 files changed, 314 insertions(+) create mode 100644 Assets/Tests/Editor/PausePointPerFrameTraceNoticeFixture.cs create mode 100644 Assets/Tests/Editor/PausePointPerFrameTraceNoticeFixture.cs.meta create mode 100644 Assets/Tests/Editor/PausePointPerFrameTraceNoticeTests.cs create mode 100644 Assets/Tests/Editor/PausePointPerFrameTraceNoticeTests.cs.meta diff --git a/Assets/Tests/Editor/PausePointPerFrameTraceNoticeFixture.cs b/Assets/Tests/Editor/PausePointPerFrameTraceNoticeFixture.cs new file mode 100644 index 0000000000..bce62a2d73 --- /dev/null +++ b/Assets/Tests/Editor/PausePointPerFrameTraceNoticeFixture.cs @@ -0,0 +1,13 @@ +namespace io.github.hatayama.UnityCliLoop.Tests.PausePointToolsFixtures +{ + internal sealed class PerFrameTraceNoticeFixture + { + private int _probe; + + public void Update() + { + // per-frame-trace-notice-probe-unique + _probe = 1; + } + } +} diff --git a/Assets/Tests/Editor/PausePointPerFrameTraceNoticeFixture.cs.meta b/Assets/Tests/Editor/PausePointPerFrameTraceNoticeFixture.cs.meta new file mode 100644 index 0000000000..66062262d7 --- /dev/null +++ b/Assets/Tests/Editor/PausePointPerFrameTraceNoticeFixture.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: 43b9134fcf54f43518d1795eb650c946 +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: diff --git a/Assets/Tests/Editor/PausePointPerFrameTraceNoticeTests.cs b/Assets/Tests/Editor/PausePointPerFrameTraceNoticeTests.cs new file mode 100644 index 0000000000..82cebac45d --- /dev/null +++ b/Assets/Tests/Editor/PausePointPerFrameTraceNoticeTests.cs @@ -0,0 +1,193 @@ +using System; +using System.IO; + +using NUnit.Framework; + +using io.github.hatayama.UnityCliLoop.FirstPartyTools; +using io.github.hatayama.UnityCliLoop.Runtime; +using io.github.hatayama.UnityCliLoop.ToolContracts; + +namespace io.github.hatayama.UnityCliLoop.Tests.Editor +{ + /// + /// Verifies the enable-time notice when a trace marker sits in a per-frame Unity message. + /// + [TestFixture] + public sealed class PausePointPerFrameTraceNoticeTests + { + private const string FixtureFilePath = "Assets/Tests/Editor/PausePointPerFrameTraceNoticeFixture.cs"; + + private const string PlayerUpdateNotice = + "'Player.Update' is a per-frame Unity message; with capture mode 'trace' the history (max 8) can roll over within moments. Prefer a conditional line or a larger --max-history."; + + [SetUp] + public void SetUp() + { + UloopPausePointRegistry.ConfigureForTests(new FakePausePointPauseController(), () => DateTime.UtcNow); + } + + [TearDown] + public void TearDown() + { + SourcePausePointPatcher.UnpatchAll(); + UloopPausePointRegistry.ResetForTests(); + } + + /// + /// What: trace on Update / FixedUpdate / LateUpdate / OnGUI appends the per-frame notice. + /// + [TestCase( + "Player.Update", + "'Player.Update' is a per-frame Unity message; with capture mode 'trace' the history (max 8) can roll over within moments. Prefer a conditional line or a larger --max-history.")] + [TestCase( + "Player.FixedUpdate", + "'Player.FixedUpdate' is a per-frame Unity message; with capture mode 'trace' the history (max 8) can roll over within moments. Prefer a conditional line or a larger --max-history.")] + [TestCase( + "Player.LateUpdate", + "'Player.LateUpdate' is a per-frame Unity message; with capture mode 'trace' the history (max 8) can roll over within moments. Prefer a conditional line or a larger --max-history.")] + [TestCase( + "Player.OnGUI", + "'Player.OnGUI' is a per-frame Unity message; with capture mode 'trace' the history (max 8) can roll over within moments. Prefer a conditional line or a larger --max-history.")] + public void BuildPerFrameTraceWarningOrEmpty_WhenTraceAndPerFrameMessage_ReturnsNotice( + string resolvedMethod, + string expectedNotice) + { + string warning = PausePointEnableWarnings.BuildPerFrameTraceWarningOrEmpty( + UloopPausePointCaptureMode.Trace, + resolvedMethod, + 8); + + Assert.That(warning, Is.EqualTo(expectedNotice)); + } + + /// + /// What: a Cecil FullName resolved method still interpolates Type.Method into the notice. + /// + [Test] + public void BuildPerFrameTraceWarningOrEmpty_WhenResolvedMethodIsCecilFullName_FormatsTypeMethod() + { + string warning = PausePointEnableWarnings.BuildPerFrameTraceWarningOrEmpty( + UloopPausePointCaptureMode.Trace, + "System.Void Ns.Player::Update()", + 8); + + Assert.That(warning, Is.EqualTo(PlayerUpdateNotice)); + } + + /// + /// What: non-trace capture modes stay silent even when the method is a per-frame Unity message. + /// + [TestCase(UloopPausePointCaptureMode.SingleShot)] + [TestCase(UloopPausePointCaptureMode.Continuous)] + public void BuildPerFrameTraceWarningOrEmpty_WhenModeIsNotTrace_ReturnsEmpty(string captureMode) + { + string warning = PausePointEnableWarnings.BuildPerFrameTraceWarningOrEmpty( + captureMode, + "Player.Update", + 8); + + Assert.That(warning, Is.EqualTo(string.Empty)); + } + + /// + /// What: a method whose simple name is not a per-frame Unity message stays silent in trace. + /// + [TestCase("Player.Start")] + [TestCase("Player.UpdateWeather")] + [TestCase("Player.OnGUILayout")] + public void BuildPerFrameTraceWarningOrEmpty_WhenSimpleNameIsNotPerFrame_ReturnsEmpty(string resolvedMethod) + { + string warning = PausePointEnableWarnings.BuildPerFrameTraceWarningOrEmpty( + UloopPausePointCaptureMode.Trace, + resolvedMethod, + 8); + + Assert.That(warning, Is.EqualTo(string.Empty)); + } + + /// + /// What: the per-frame notice is appended after an existing enable warning. + /// + [Test] + public void MergeWarnings_WhenPriorWarningAndPerFrameNotice_AppendsNoticeAfterPrior() + { + string prior = "Pause point was enabled before PlayMode while Domain Reload is enabled. Entering PlayMode may clear this marker; keep Domain Reload disabled for this workflow or enable the marker after PlayMode starts."; + string notice = PausePointEnableWarnings.BuildPerFrameTraceWarningOrEmpty( + UloopPausePointCaptureMode.Trace, + "Player.Update", + 8); + + string merged = PausePointEnableWarnings.MergeWarnings(prior, notice); + + Assert.That( + merged, + Is.EqualTo( + "Pause point was enabled before PlayMode while Domain Reload is enabled. Entering PlayMode may clear this marker; keep Domain Reload disabled for this workflow or enable the marker after PlayMode starts. 'Player.Update' is a per-frame Unity message; with capture mode 'trace' the history (max 8) can roll over within moments. Prefer a conditional line or a larger --max-history.")); + } + + /// + /// What: enable with file:line trace on Update appends the per-frame notice using the effective max-history. + /// + [Test] + public void Enable_WhenTraceMarkerIsOnUpdate_AppendsPerFrameNotice() + { + string absolutePath = Path.Combine(UnityCliLoopPathResolver.GetProjectRoot(), FixtureFilePath); + string diskSource = File.ReadAllText(absolutePath); + int requestedLine = FindLineNumberContaining(diskSource, "per-frame-trace-notice" + "-probe-unique") + 1; + Assert.That(requestedLine, Is.GreaterThan(1)); + + PausePointResponse response = new PausePointUseCase().Enable(new EnablePausePointSchema + { + File = FixtureFilePath, + Line = requestedLine, + TimeoutSeconds = 30, + Mode = UloopPausePointCaptureMode.Trace, + MaxHistory = 8 + }); + + Assert.That( + response.Success, + Is.True, + response.ErrorCode + " / " + response.Message + " / " + response.RecommendedNextAction); + string notice = + "'PerFrameTraceNoticeFixture.Update' is a per-frame Unity message; with capture mode 'trace' the history (max 8) can roll over within moments. Prefer a conditional line or a larger --max-history."; + string expectedWarning = PausePointEnableWarnings.MergeWarnings( + PausePointEnableWarnings.MergeWarnings( + PausePointEnableWarnings.CreateEnableWarning(), + SourcePausePointConstants.SmallMethodInliningRiskWarning), + notice); + Assert.That(response.Warning, Is.EqualTo(expectedWarning)); + } + + private static int FindLineNumberContaining(string source, string fragment) + { + string[] lines = source.Replace("\r\n", "\n").Split('\n'); + for (int index = 0; index < lines.Length; index++) + { + if (lines[index].Contains(fragment)) + { + return index + 1; + } + } + + return -1; + } + + private sealed class FakePausePointPauseController : IUloopPausePointPauseController + { + public int PauseCount { get; private set; } + public bool IsPlaying => true; + public bool IsPaused => PauseCount > 0; + + public void Pause() + { + PauseCount++; + } + + public void Resume() + { + PauseCount = 0; + } + } + } +} diff --git a/Assets/Tests/Editor/PausePointPerFrameTraceNoticeTests.cs.meta b/Assets/Tests/Editor/PausePointPerFrameTraceNoticeTests.cs.meta new file mode 100644 index 0000000000..524bd7c4d7 --- /dev/null +++ b/Assets/Tests/Editor/PausePointPerFrameTraceNoticeTests.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: 62b5760e71a0749369cde866c6585466 +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: diff --git a/Packages/src/Editor/FirstPartyTools/PausePoint/PausePointEnableWarnings.cs b/Packages/src/Editor/FirstPartyTools/PausePoint/PausePointEnableWarnings.cs index 6c858ab08a..51b5d24308 100644 --- a/Packages/src/Editor/FirstPartyTools/PausePoint/PausePointEnableWarnings.cs +++ b/Packages/src/Editor/FirstPartyTools/PausePoint/PausePointEnableWarnings.cs @@ -65,6 +65,80 @@ internal static string MergeWarnings(string first, string second) return first + " " + second; } + // Why Type.Method in the notice: enable's resolved-method string is Cecil FullName, but + // the warning should name the Unity message the same way agents already read caller frames. + internal static string BuildPerFrameTraceWarningOrEmpty( + string captureMode, + string resolvedMethod, + int maxHistory) + { + if (captureMode != UloopPausePointCaptureMode.Trace || string.IsNullOrEmpty(resolvedMethod)) + { + return string.Empty; + } + + string simpleName = ExtractSimpleMethodName(resolvedMethod); + if (!IsPerFrameUnityMessageSimpleName(simpleName)) + { + return string.Empty; + } + + return string.Format( + SourcePausePointConstants.PerFrameTraceNoticeFormat, + FormatTypeMethodDisplay(resolvedMethod, simpleName), + maxHistory); + } + + private static bool IsPerFrameUnityMessageSimpleName(string simpleName) + { + return simpleName == "Update" + || simpleName == "FixedUpdate" + || simpleName == "LateUpdate" + || simpleName == "OnGUI"; + } + + private static string ExtractSimpleMethodName(string resolvedMethod) + { + int colon = resolvedMethod.IndexOf("::", StringComparison.Ordinal); + if (colon >= 0) + { + int start = colon + 2; + int paren = resolvedMethod.IndexOf('(', start); + if (paren >= 0) + { + return resolvedMethod.Substring(start, paren - start); + } + + return resolvedMethod.Substring(start); + } + + int lastDot = resolvedMethod.LastIndexOf('.'); + string tail = lastDot >= 0 ? resolvedMethod.Substring(lastDot + 1) : resolvedMethod; + int tailParen = tail.IndexOf('('); + if (tailParen >= 0) + { + return tail.Substring(0, tailParen); + } + + return tail; + } + + private static string FormatTypeMethodDisplay(string resolvedMethod, string simpleName) + { + int colon = resolvedMethod.IndexOf("::", StringComparison.Ordinal); + if (colon < 0) + { + return resolvedMethod; + } + + string beforeColon = resolvedMethod.Substring(0, colon); + int space = beforeColon.LastIndexOf(' '); + string typeFullName = space >= 0 ? beforeColon.Substring(space + 1) : beforeColon; + int typeSep = Math.Max(typeFullName.LastIndexOf('.'), typeFullName.LastIndexOf('/')); + string typeName = typeSep >= 0 ? typeFullName.Substring(typeSep + 1) : typeFullName; + return typeName + "." + simpleName; + } + // Why success-only: resolve failure leaves ResolvedMethod and ResolvedLineText empty, // so this wording would point at fields that are not on the response. // Why same resolvedLine on both sides: the resolver rounds empty/comment lines forward, diff --git a/Packages/src/Editor/FirstPartyTools/PausePoint/PausePointUseCase.cs b/Packages/src/Editor/FirstPartyTools/PausePoint/PausePointUseCase.cs index d84cab8686..f795bae18c 100644 --- a/Packages/src/Editor/FirstPartyTools/PausePoint/PausePointUseCase.cs +++ b/Packages/src/Editor/FirstPartyTools/PausePoint/PausePointUseCase.cs @@ -415,6 +415,12 @@ private static PausePointResponse FinishEnableBySourceLocation( response.Warning = PausePointEnableWarnings.MergeWarnings( response.Warning, PausePointEnableWarnings.BuildAddedFieldsNotCapturedWarningOrEmpty(patchResult.DeclaringType)); + response.Warning = PausePointEnableWarnings.MergeWarnings( + response.Warning, + PausePointEnableWarnings.BuildPerFrameTraceWarningOrEmpty( + parameters.Mode, + resolvedMethod, + snapshot.MaxHistory)); LogEnable(response.Id, response.ResolvedMethod, $"{parameters.File}:{response.ResolvedLine}", response.Mode, response.Warning); if (patchResult.HasPhysicsCallbackWarning) diff --git a/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointConstants.cs b/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointConstants.cs index 8db0234110..d4b5be20c4 100644 --- a/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointConstants.cs +++ b/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointConstants.cs @@ -140,6 +140,12 @@ internal static class SourcePausePointConstants "The target method body is very small and may be inlined by Mono's JIT into its callers; " + "if HitCount stays 0 while the line demonstrably runs, move the pause point into the calling method."; + // Format: Type.Method display name, effective max-history. + // Why not a completed-tense overflow: trace history on a per-frame message can roll + // over quickly, but that is not guaranteed (low hit rate, a larger --max-history). + public const string PerFrameTraceNoticeFormat = + "'{0}' is a per-frame Unity message; with capture mode 'trace' the history (max {1}) can roll over within moments. Prefer a conditional line or a larger --max-history."; + // Callers have observed captured values that look like they belong to the line after // ResolvedLine; this makes the pre-line snapshot timing explicit in the response itself // instead of leaving it documented only in the skill. From e70b8fba783cd673880b8973c8d21759336cbfbe Mon Sep 17 00:00:00 2001 From: hatayama Date: Thu, 20 Aug 2026 12:16:21 +0900 Subject: [PATCH 2/2] fix: condition the per-frame trace notice so a name match is not claimed as a Unity message The notice asserted the method is a per-frame Unity message, which is false for a plain C# Update driven by a MonoBehaviour. Name-based matching stays so that delegation pattern is not a false negative. Co-authored-by: Cursor --- .../PausePointPerFrameTraceNoticeTests.cs | 18 +++++++++--------- .../PausePoint/SourcePausePointConstants.cs | 8 +++++--- 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/Assets/Tests/Editor/PausePointPerFrameTraceNoticeTests.cs b/Assets/Tests/Editor/PausePointPerFrameTraceNoticeTests.cs index 82cebac45d..2a726a9e7c 100644 --- a/Assets/Tests/Editor/PausePointPerFrameTraceNoticeTests.cs +++ b/Assets/Tests/Editor/PausePointPerFrameTraceNoticeTests.cs @@ -10,7 +10,7 @@ namespace io.github.hatayama.UnityCliLoop.Tests.Editor { /// - /// Verifies the enable-time notice when a trace marker sits in a per-frame Unity message. + /// Verifies the enable-time notice when a trace marker's method name matches a per-frame Unity message. /// [TestFixture] public sealed class PausePointPerFrameTraceNoticeTests @@ -18,7 +18,7 @@ public sealed class PausePointPerFrameTraceNoticeTests private const string FixtureFilePath = "Assets/Tests/Editor/PausePointPerFrameTraceNoticeFixture.cs"; private const string PlayerUpdateNotice = - "'Player.Update' is a per-frame Unity message; with capture mode 'trace' the history (max 8) can roll over within moments. Prefer a conditional line or a larger --max-history."; + "'Player.Update' matches a per-frame Unity message name; if this line runs every frame, capture mode 'trace' can roll the history (max 8) over within moments. Prefer a conditional line or a larger --max-history."; [SetUp] public void SetUp() @@ -38,16 +38,16 @@ public void TearDown() /// [TestCase( "Player.Update", - "'Player.Update' is a per-frame Unity message; with capture mode 'trace' the history (max 8) can roll over within moments. Prefer a conditional line or a larger --max-history.")] + "'Player.Update' matches a per-frame Unity message name; if this line runs every frame, capture mode 'trace' can roll the history (max 8) over within moments. Prefer a conditional line or a larger --max-history.")] [TestCase( "Player.FixedUpdate", - "'Player.FixedUpdate' is a per-frame Unity message; with capture mode 'trace' the history (max 8) can roll over within moments. Prefer a conditional line or a larger --max-history.")] + "'Player.FixedUpdate' matches a per-frame Unity message name; if this line runs every frame, capture mode 'trace' can roll the history (max 8) over within moments. Prefer a conditional line or a larger --max-history.")] [TestCase( "Player.LateUpdate", - "'Player.LateUpdate' is a per-frame Unity message; with capture mode 'trace' the history (max 8) can roll over within moments. Prefer a conditional line or a larger --max-history.")] + "'Player.LateUpdate' matches a per-frame Unity message name; if this line runs every frame, capture mode 'trace' can roll the history (max 8) over within moments. Prefer a conditional line or a larger --max-history.")] [TestCase( "Player.OnGUI", - "'Player.OnGUI' is a per-frame Unity message; with capture mode 'trace' the history (max 8) can roll over within moments. Prefer a conditional line or a larger --max-history.")] + "'Player.OnGUI' matches a per-frame Unity message name; if this line runs every frame, capture mode 'trace' can roll the history (max 8) over within moments. Prefer a conditional line or a larger --max-history.")] public void BuildPerFrameTraceWarningOrEmpty_WhenTraceAndPerFrameMessage_ReturnsNotice( string resolvedMethod, string expectedNotice) @@ -75,7 +75,7 @@ public void BuildPerFrameTraceWarningOrEmpty_WhenResolvedMethodIsCecilFullName_F } /// - /// What: non-trace capture modes stay silent even when the method is a per-frame Unity message. + /// What: non-trace capture modes stay silent even when the method name matches a per-frame Unity message. /// [TestCase(UloopPausePointCaptureMode.SingleShot)] [TestCase(UloopPausePointCaptureMode.Continuous)] @@ -122,7 +122,7 @@ public void MergeWarnings_WhenPriorWarningAndPerFrameNotice_AppendsNoticeAfterPr Assert.That( merged, Is.EqualTo( - "Pause point was enabled before PlayMode while Domain Reload is enabled. Entering PlayMode may clear this marker; keep Domain Reload disabled for this workflow or enable the marker after PlayMode starts. 'Player.Update' is a per-frame Unity message; with capture mode 'trace' the history (max 8) can roll over within moments. Prefer a conditional line or a larger --max-history.")); + "Pause point was enabled before PlayMode while Domain Reload is enabled. Entering PlayMode may clear this marker; keep Domain Reload disabled for this workflow or enable the marker after PlayMode starts. 'Player.Update' matches a per-frame Unity message name; if this line runs every frame, capture mode 'trace' can roll the history (max 8) over within moments. Prefer a conditional line or a larger --max-history.")); } /// @@ -150,7 +150,7 @@ public void Enable_WhenTraceMarkerIsOnUpdate_AppendsPerFrameNotice() Is.True, response.ErrorCode + " / " + response.Message + " / " + response.RecommendedNextAction); string notice = - "'PerFrameTraceNoticeFixture.Update' is a per-frame Unity message; with capture mode 'trace' the history (max 8) can roll over within moments. Prefer a conditional line or a larger --max-history."; + "'PerFrameTraceNoticeFixture.Update' matches a per-frame Unity message name; if this line runs every frame, capture mode 'trace' can roll the history (max 8) over within moments. Prefer a conditional line or a larger --max-history."; string expectedWarning = PausePointEnableWarnings.MergeWarnings( PausePointEnableWarnings.MergeWarnings( PausePointEnableWarnings.CreateEnableWarning(), diff --git a/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointConstants.cs b/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointConstants.cs index d4b5be20c4..1086c77dc0 100644 --- a/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointConstants.cs +++ b/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointConstants.cs @@ -141,10 +141,12 @@ internal static class SourcePausePointConstants + "if HitCount stays 0 while the line demonstrably runs, move the pause point into the calling method."; // Format: Type.Method display name, effective max-history. - // Why not a completed-tense overflow: trace history on a per-frame message can roll - // over quickly, but that is not guaranteed (low hit rate, a larger --max-history). + // Why name-based, not a MonoBehaviour type check: a plain C# Update is often driven + // every frame by a MonoBehaviour delegate, and a type check would miss that case. + // Why conditional wording: the name match is not proof this is a Unity message, so + // "is a per-frame Unity message" would be false for non-MonoBehaviour types. public const string PerFrameTraceNoticeFormat = - "'{0}' is a per-frame Unity message; with capture mode 'trace' the history (max {1}) can roll over within moments. Prefer a conditional line or a larger --max-history."; + "'{0}' matches a per-frame Unity message name; if this line runs every frame, capture mode 'trace' can roll the history (max {1}) over within moments. Prefer a conditional line or a larger --max-history."; // Callers have observed captured values that look like they belong to the line after // ResolvedLine; this makes the pre-line snapshot timing explicit in the response itself