Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -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++;
}
}
}

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
@@ -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++;
}
}
}

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -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()
{
Expand Down
52 changes: 52 additions & 0 deletions Assets/Tests/Editor/SourcePausePointPhysicalMessageMethodsTests.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
using NUnit.Framework;

using io.github.hatayama.UnityCliLoop.FirstPartyTools;

namespace io.github.hatayama.UnityCliLoop.Tests.Editor
{
/// <summary>
/// Verifies which method names are treated as Unity physics message methods for the
/// physical-callback pause-point warning.
/// </summary>
[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);
}
}
}

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down Expand Up @@ -253,6 +251,24 @@ private static bool HasBurstCompileAttribute(MemberInfo member)
return false;
}

private static string BuildPatchWarning(MethodBase method)
{
List<string> 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))
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
using System;
using System.Collections.Generic;

namespace io.github.hatayama.UnityCliLoop.FirstPartyTools
{
/// <summary>
/// Names Unity's physics message methods, whose native dispatch caches its call path
/// independently of a later Harmony patch (see PhysicalCallbackMayMissExistingInstanceWarning).
/// </summary>
internal static class SourcePausePointPhysicalMessageMethods
{
private static readonly HashSet<string> 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);
}
}
}

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -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(\"<id>\") 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(\"<id>\") 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 ""
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(\"<id>\") 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(\"<id>\") 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.",
},
}

Expand Down