diff --git a/Assets/Tests/Editor/SourcePausePointPatcher/Fixtures/PatcherPhysicalCallbackMethodFixture.cs b/Assets/Tests/Editor/SourcePausePointPatcher/Fixtures/PatcherPhysicalCallbackMethodFixture.cs new file mode 100644 index 0000000000..9b9c2b1375 --- /dev/null +++ b/Assets/Tests/Editor/SourcePausePointPatcher/Fixtures/PatcherPhysicalCallbackMethodFixture.cs @@ -0,0 +1,21 @@ +// FROZEN FIXTURE: content and line numbers are asserted by SourcePausePointPatcherTests. +// Do not reformat or edit this file; add a new fixture file instead. +using UnityEngine; + +namespace io.github.hatayama.UnityCliLoop.Tests.SourcePausePointPatcherFixtures +{ + internal sealed class PatcherPhysicalCallbackMethodFixture : MonoBehaviour + { + public int HitCount; + + private void OnCollisionEnter2D(Collision2D collision) + { + HitCount++; + } + + private void Update() + { + HitCount++; + } + } +} diff --git a/Assets/Tests/Editor/SourcePausePointPatcher/Fixtures/PatcherPhysicalCallbackMethodFixture.cs.meta b/Assets/Tests/Editor/SourcePausePointPatcher/Fixtures/PatcherPhysicalCallbackMethodFixture.cs.meta new file mode 100644 index 0000000000..d2ed0e79c1 --- /dev/null +++ b/Assets/Tests/Editor/SourcePausePointPatcher/Fixtures/PatcherPhysicalCallbackMethodFixture.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: 8923053082ea64e1c836935dcb251b1e +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: diff --git a/Assets/Tests/Editor/SourcePausePointPatcher/Fixtures/PatcherPhysicsNamedMethodOnPlainClassFixture.cs b/Assets/Tests/Editor/SourcePausePointPatcher/Fixtures/PatcherPhysicsNamedMethodOnPlainClassFixture.cs new file mode 100644 index 0000000000..32db94ae34 --- /dev/null +++ b/Assets/Tests/Editor/SourcePausePointPatcher/Fixtures/PatcherPhysicsNamedMethodOnPlainClassFixture.cs @@ -0,0 +1,14 @@ +// FROZEN FIXTURE: content and line numbers are asserted by SourcePausePointPatcherTests. +// Do not reformat or edit this file; add a new fixture file instead. +namespace io.github.hatayama.UnityCliLoop.Tests.SourcePausePointPatcherFixtures +{ + internal sealed class PatcherPhysicsNamedMethodOnPlainClassFixture + { + public int HitCount; + + public void OnTriggerEnter2D() + { + HitCount++; + } + } +} diff --git a/Assets/Tests/Editor/SourcePausePointPatcher/Fixtures/PatcherPhysicsNamedMethodOnPlainClassFixture.cs.meta b/Assets/Tests/Editor/SourcePausePointPatcher/Fixtures/PatcherPhysicsNamedMethodOnPlainClassFixture.cs.meta new file mode 100644 index 0000000000..baa4f1bc8e --- /dev/null +++ b/Assets/Tests/Editor/SourcePausePointPatcher/Fixtures/PatcherPhysicsNamedMethodOnPlainClassFixture.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: 71a3245e98f904601a3d3afaa334f78f +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: diff --git a/Assets/Tests/Editor/SourcePausePointPatcher/SourcePausePointPatcherTests.cs b/Assets/Tests/Editor/SourcePausePointPatcher/SourcePausePointPatcherTests.cs index 6c5ed50f4c..f9e56ed389 100644 --- a/Assets/Tests/Editor/SourcePausePointPatcher/SourcePausePointPatcherTests.cs +++ b/Assets/Tests/Editor/SourcePausePointPatcher/SourcePausePointPatcherTests.cs @@ -139,6 +139,60 @@ public void Patch_RefStructInstanceMethod_DegradesToNullInstanceAndCapturesLocal Assert.That(snapshot.CapturedVariables.Select(v => v.Name), Is.EquivalentTo(new[] { "doubled" })); } + [Test] + public void Patch_PhysicsMessageMethodOnMonoBehaviour_ReturnsCachedDispatchWarning() + { + // Verifies OnCollisionEnter2D on a MonoBehaviour-derived type reports the informational + // warning about Unity's physics message dispatch caching its call path independently of + // this patch (see To-Do 2 investigation: an already-existing GameObject's collision + // callback can miss the pause point even though the method body runs). + const string id = "patcher-physical-callback-method"; + SourcePausePointResolveResult resolveResult = SourcePausePointResolver.Resolve( + FixturesDirectory + "PatcherPhysicalCallbackMethodFixture.cs", 13); + Assert.That(resolveResult.Success, Is.True); + + UloopPausePointRegistry.Enable(id, 30); + SourcePausePointPatchResult patchResult = SourcePausePointPatcher.Patch(id, resolveResult.Resolution); + + Assert.That(patchResult.Success, Is.True); + Assert.That(patchResult.Warning, Is.EqualTo(SourcePausePointConstants.PhysicalCallbackMayMissExistingInstanceWarning)); + } + + [Test] + public void Patch_OrdinaryMessageMethodOnSameMonoBehaviour_DoesNotReturnCachedDispatchWarning() + { + // Verifies Update() on the same MonoBehaviour fixture does not trigger the + // physics-message warning, since only physics message methods carry the caching risk. + const string id = "patcher-ordinary-message-method"; + SourcePausePointResolveResult resolveResult = SourcePausePointResolver.Resolve( + FixturesDirectory + "PatcherPhysicalCallbackMethodFixture.cs", 18); + Assert.That(resolveResult.Success, Is.True); + + UloopPausePointRegistry.Enable(id, 30); + SourcePausePointPatchResult patchResult = SourcePausePointPatcher.Patch(id, resolveResult.Resolution); + + Assert.That(patchResult.Success, Is.True); + Assert.That(patchResult.Warning, Is.Empty); + } + + [Test] + public void Patch_PhysicsNamedMethodOnNonMonoBehaviourType_DoesNotReturnCachedDispatchWarning() + { + // Verifies the warning requires both a matching method name AND a MonoBehaviour-derived + // declaring type, so an unrelated plain class that happens to name a method + // "OnTriggerEnter2D" does not produce a false positive. + const string id = "patcher-physics-named-method-plain-class"; + SourcePausePointResolveResult resolveResult = SourcePausePointResolver.Resolve( + FixturesDirectory + "PatcherPhysicsNamedMethodOnPlainClassFixture.cs", 9); + Assert.That(resolveResult.Success, Is.True); + + UloopPausePointRegistry.Enable(id, 30); + SourcePausePointPatchResult patchResult = SourcePausePointPatcher.Patch(id, resolveResult.Resolution); + + Assert.That(patchResult.Success, Is.True); + Assert.That(patchResult.Warning, Is.Empty); + } + [Test] public void Patch_LoopMethod_PreservesBackEdgeBranchTargetAndCapturesFirstIterationState() { diff --git a/Assets/Tests/Editor/SourcePausePointPhysicalMessageMethodsTests.cs b/Assets/Tests/Editor/SourcePausePointPhysicalMessageMethodsTests.cs new file mode 100644 index 0000000000..672b9fbd33 --- /dev/null +++ b/Assets/Tests/Editor/SourcePausePointPhysicalMessageMethodsTests.cs @@ -0,0 +1,52 @@ +using NUnit.Framework; + +using io.github.hatayama.UnityCliLoop.FirstPartyTools; + +namespace io.github.hatayama.UnityCliLoop.Tests.Editor +{ + /// + /// Verifies which method names are treated as Unity physics message methods for the + /// physical-callback pause-point warning. + /// + [TestFixture] + public sealed class SourcePausePointPhysicalMessageMethodsTests + { + [TestCase("OnCollisionEnter")] + [TestCase("OnCollisionStay")] + [TestCase("OnCollisionExit")] + [TestCase("OnCollisionEnter2D")] + [TestCase("OnCollisionStay2D")] + [TestCase("OnCollisionExit2D")] + [TestCase("OnTriggerEnter")] + [TestCase("OnTriggerStay")] + [TestCase("OnTriggerExit")] + [TestCase("OnTriggerEnter2D")] + [TestCase("OnTriggerStay2D")] + [TestCase("OnTriggerExit2D")] + [TestCase("OnParticleCollision")] + public void IsPhysicalMessageMethod_WhenNameIsAPhysicsMessageMethod_ReturnsTrue(string methodName) + { + // Verifies every known Unity physics message method name is recognized. + bool result = SourcePausePointPhysicalMessageMethods.IsPhysicalMessageMethod(methodName); + + Assert.That(result, Is.True); + } + + [TestCase("Update")] + [TestCase("LateUpdate")] + [TestCase("FixedUpdate")] + [TestCase("Awake")] + [TestCase("OnEnable")] + [TestCase("Add")] + [TestCase("")] + [TestCase(null)] + public void IsPhysicalMessageMethod_WhenNameIsNotAPhysicsMessageMethod_ReturnsFalse(string methodName) + { + // Verifies ordinary lifecycle methods and non-message names do not match, so the + // warning never fires for methods like Update() that patch and hit reliably. + bool result = SourcePausePointPhysicalMessageMethods.IsPhysicalMessageMethod(methodName); + + Assert.That(result, Is.False); + } + } +} diff --git a/Assets/Tests/Editor/SourcePausePointPhysicalMessageMethodsTests.cs.meta b/Assets/Tests/Editor/SourcePausePointPhysicalMessageMethodsTests.cs.meta new file mode 100644 index 0000000000..db2301fd3b --- /dev/null +++ b/Assets/Tests/Editor/SourcePausePointPhysicalMessageMethodsTests.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: 125a07e740286444abccff0f6d6f8529 +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: diff --git a/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointConstants.cs b/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointConstants.cs index 34893b0765..4172bd1eab 100644 --- a/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointConstants.cs +++ b/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointConstants.cs @@ -47,6 +47,19 @@ internal static class SourcePausePointConstants "The declaring type is a ref struct; this-instance fields are not captured " + "(locals and parameters are still captured normally)."; + // Unity's physics message dispatch (OnCollision*/OnTrigger*/OnParticleCollision) resolves + // its call path once when the GameObject registers with the physics engine; a Harmony + // patch applied after that registration does not reach the cached path, so the pause + // point can silently miss a GameObject that already existed before this call. This is + // informational only: the same method on a newly created GameObject patches correctly. + public const string PhysicalCallbackMayMissExistingInstanceWarning = + "This resolves to a Unity physics message method (OnCollision*/OnTrigger*/OnParticleCollision). " + + "If the target GameObject already existed before this pause point was enabled, Unity's " + + "cached message dispatch may not route through the patch and the pause point may never " + + "hit even though the method body runs. Workarounds: destroy and recreate the GameObject " + + "after enabling this pause point, or embed UloopPausePoint.Pause(\"id\") directly in the " + + "method body and arm it with enable-pause-point --id instead."; + // 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/FirstPartyTools/PausePoint/SourcePausePointPatcher.cs b/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointPatcher.cs index ecb07e0847..1c525ff0fe 100644 --- a/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointPatcher.cs +++ b/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointPatcher.cs @@ -118,9 +118,7 @@ public static SourcePausePointPatchResult Patch(string id, SourcePausePointResol } } - string warning = !method.IsStatic && IsByRefLikeType(method.DeclaringType) - ? SourcePausePointConstants.RefStructInstanceNotCapturedWarning - : string.Empty; + string warning = BuildPatchWarning(method); return SourcePausePointPatchResult.SuccessResult(warning); } @@ -253,6 +251,24 @@ private static bool HasBurstCompileAttribute(MemberInfo member) return false; } + private static string BuildPatchWarning(MethodBase method) + { + List warnings = new(); + + if (!method.IsStatic && IsByRefLikeType(method.DeclaringType)) + { + warnings.Add(SourcePausePointConstants.RefStructInstanceNotCapturedWarning); + } + + if (SourcePausePointPhysicalMessageMethods.IsPhysicalMessageMethod(method.Name) && + typeof(MonoBehaviour).IsAssignableFrom(method.DeclaringType)) + { + warnings.Add(SourcePausePointConstants.PhysicalCallbackMayMissExistingInstanceWarning); + } + + return string.Join(" ", warnings); + } + private static bool IsByRefLikeType(Type type) { foreach (object attribute in type.GetCustomAttributes(inherit: false)) diff --git a/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointPhysicalMessageMethods.cs b/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointPhysicalMessageMethods.cs new file mode 100644 index 0000000000..594a7447fe --- /dev/null +++ b/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointPhysicalMessageMethods.cs @@ -0,0 +1,34 @@ +using System; +using System.Collections.Generic; + +namespace io.github.hatayama.UnityCliLoop.FirstPartyTools +{ + /// + /// Names Unity's physics message methods, whose native dispatch caches its call path + /// independently of a later Harmony patch (see PhysicalCallbackMayMissExistingInstanceWarning). + /// + internal static class SourcePausePointPhysicalMessageMethods + { + private static readonly HashSet Names = new(StringComparer.Ordinal) + { + "OnCollisionEnter", + "OnCollisionStay", + "OnCollisionExit", + "OnCollisionEnter2D", + "OnCollisionStay2D", + "OnCollisionExit2D", + "OnTriggerEnter", + "OnTriggerStay", + "OnTriggerExit", + "OnTriggerEnter2D", + "OnTriggerStay2D", + "OnTriggerExit2D", + "OnParticleCollision", + }; + + public static bool IsPhysicalMessageMethod(string methodName) + { + return !string.IsNullOrEmpty(methodName) && Names.Contains(methodName); + } + } +} diff --git a/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointPhysicalMessageMethods.cs.meta b/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointPhysicalMessageMethods.cs.meta new file mode 100644 index 0000000000..b75f15cea2 --- /dev/null +++ b/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointPhysicalMessageMethods.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: dd8bcf632710d4c4cbccf0e5c44f502c +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: diff --git a/cli/project-runner/internal/projectrunner/pause_point_errors.go b/cli/project-runner/internal/projectrunner/pause_point_errors.go index 65eed92d76..06942097f0 100644 --- a/cli/project-runner/internal/projectrunner/pause_point_errors.go +++ b/cli/project-runner/internal/projectrunner/pause_point_errors.go @@ -78,7 +78,8 @@ func pausePointTimeoutHint(response pausePointStatusResponse) string { return pausePointHintEditorAlreadyPaused } if response.HitCount == 0 && response.Status == pausePointStatusEnabled { - return "Marker was enabled but never hit. Confirm the id matches UloopPausePoint.Pause(\"\") and that the code path was executed. In fast-progressing games the state may have already moved past the marker (for example back to Ready or GameOver), so re-trigger the code path and wait again." + return "Marker was enabled but never hit. Confirm the id matches UloopPausePoint.Pause(\"\") and that the code path was executed. In fast-progressing games the state may have already moved past the marker (for example back to Ready or GameOver), so re-trigger the code path and wait again. " + + "If the marker targets a Unity message method such as OnCollisionEnter2D/OnTriggerEnter2D, check whether `enable-pause-point`'s response carried a Warning about cached message dispatch: Unity can resolve a GameObject's message dispatch before the marker patch is installed, so a GameObject that already existed at enable time may never reach the marker even though the method body runs. Recreating the GameObject after enabling, or embedding UloopPausePoint.Pause(\"id\") directly in the method body, avoids this." } return "" } diff --git a/cli/project-runner/internal/projectrunner/pause_point_wait_test.go b/cli/project-runner/internal/projectrunner/pause_point_wait_test.go index f3fd6d13e1..8a9c70a14a 100644 --- a/cli/project-runner/internal/projectrunner/pause_point_wait_test.go +++ b/cli/project-runner/internal/projectrunner/pause_point_wait_test.go @@ -713,7 +713,8 @@ func TestPausePointTimeoutErrorIncludesDiagnosisHint(t *testing.T) { EditorState: pausePointEditorState{IsPlaying: true, CapturedAt: "Current"}, HitCount: 0, }, - wantHint: "Marker was enabled but never hit. Confirm the id matches UloopPausePoint.Pause(\"\") and that the code path was executed. In fast-progressing games the state may have already moved past the marker (for example back to Ready or GameOver), so re-trigger the code path and wait again.", + wantHint: "Marker was enabled but never hit. Confirm the id matches UloopPausePoint.Pause(\"\") and that the code path was executed. In fast-progressing games the state may have already moved past the marker (for example back to Ready or GameOver), so re-trigger the code path and wait again. " + + "If the marker targets a Unity message method such as OnCollisionEnter2D/OnTriggerEnter2D, check whether `enable-pause-point`'s response carried a Warning about cached message dispatch: Unity can resolve a GameObject's message dispatch before the marker patch is installed, so a GameObject that already existed at enable time may never reach the marker even though the method body runs. Recreating the GameObject after enabling, or embedding UloopPausePoint.Pause(\"id\") directly in the method body, avoids this.", }, }