From 3b5b9cfbd793e1fabcdbd02f298e43c6170a78e5 Mon Sep 17 00:00:00 2001 From: hatayama Date: Tue, 21 Jul 2026 22:45:51 +0900 Subject: [PATCH 1/7] Document existing-instance physics-callback pause-point gap and workaround Pause points armed on OnCollisionEnter2D/OnTriggerEnter2D miss GameObjects that already existed before the pause point was enabled, because Unity caches physics message dispatch per component type at first registration. Confirmed the enabled-toggle workaround (disable/enable the component once) re-resolves dispatch for the whole component type for the rest of the Editor session, fixing every instance, not just the toggled one -- verified via a controlled two-instance experiment (one toggled, one never touched, both received the fix). - Update the physics-callback Warning constants with the lightest-known workaround, its type-wide/session-wide scope, and its OnDisable/OnEnable side effect - Add a permanent regression-harness scene and driver script covering both OnCollisionEnter2D and OnTriggerEnter2D - The harness's baseline-miss check is informational only (not asserted): a best-effort domain reload before arming did not reliably reproduce a fresh miss on every run, so only the post-workaround hit is a strict assertion --- .../PhysicsCallbackExistingInstance.meta | 8 + .../PhysicsCallbackExistingInstance.unity | 406 ++++++++++++++++++ ...PhysicsCallbackExistingInstance.unity.meta | 7 + .../PhysicsCallbackFloor.cs | 19 + .../PhysicsCallbackFloor.cs.meta | 11 + .../PhysicsCallbackTriggerZone.cs | 18 + .../PhysicsCallbackTriggerZone.cs.meta | 11 + .../PausePoint/SourcePausePointConstants.cs | 22 +- docs/regression-harness.md | 1 + ...ness-physics-callback-existing-instance.sh | 181 ++++++++ 10 files changed, 678 insertions(+), 6 deletions(-) create mode 100644 Assets/RegressionHarness/PhysicsCallbackExistingInstance.meta create mode 100644 Assets/RegressionHarness/PhysicsCallbackExistingInstance/PhysicsCallbackExistingInstance.unity create mode 100644 Assets/RegressionHarness/PhysicsCallbackExistingInstance/PhysicsCallbackExistingInstance.unity.meta create mode 100644 Assets/RegressionHarness/PhysicsCallbackExistingInstance/PhysicsCallbackFloor.cs create mode 100644 Assets/RegressionHarness/PhysicsCallbackExistingInstance/PhysicsCallbackFloor.cs.meta create mode 100644 Assets/RegressionHarness/PhysicsCallbackExistingInstance/PhysicsCallbackTriggerZone.cs create mode 100644 Assets/RegressionHarness/PhysicsCallbackExistingInstance/PhysicsCallbackTriggerZone.cs.meta create mode 100755 scripts/regression-harness-physics-callback-existing-instance.sh diff --git a/Assets/RegressionHarness/PhysicsCallbackExistingInstance.meta b/Assets/RegressionHarness/PhysicsCallbackExistingInstance.meta new file mode 100644 index 0000000000..0064309ef8 --- /dev/null +++ b/Assets/RegressionHarness/PhysicsCallbackExistingInstance.meta @@ -0,0 +1,8 @@ +fileFormatVersion: 2 +guid: b497e99ba25c444729e07c73b22e0e26 +folderAsset: yes +DefaultImporter: + externalObjects: {} + userData: + assetBundleName: + assetBundleVariant: diff --git a/Assets/RegressionHarness/PhysicsCallbackExistingInstance/PhysicsCallbackExistingInstance.unity b/Assets/RegressionHarness/PhysicsCallbackExistingInstance/PhysicsCallbackExistingInstance.unity new file mode 100644 index 0000000000..7eb359ab64 --- /dev/null +++ b/Assets/RegressionHarness/PhysicsCallbackExistingInstance/PhysicsCallbackExistingInstance.unity @@ -0,0 +1,406 @@ +%YAML 1.1 +%TAG !u! tag:unity3d.com,2011: +--- !u!29 &1 +OcclusionCullingSettings: + m_ObjectHideFlags: 0 + serializedVersion: 2 + m_OcclusionBakeSettings: + smallestOccluder: 5 + smallestHole: 0.25 + backfaceThreshold: 100 + m_SceneGUID: 00000000000000000000000000000000 + m_OcclusionCullingData: {fileID: 0} +--- !u!104 &2 +RenderSettings: + m_ObjectHideFlags: 0 + serializedVersion: 9 + m_Fog: 0 + m_FogColor: {r: 0.5, g: 0.5, b: 0.5, a: 1} + m_FogMode: 3 + m_FogDensity: 0.01 + m_LinearFogStart: 0 + m_LinearFogEnd: 300 + m_AmbientSkyColor: {r: 0.212, g: 0.227, b: 0.259, a: 1} + m_AmbientEquatorColor: {r: 0.114, g: 0.125, b: 0.133, a: 1} + m_AmbientGroundColor: {r: 0.047, g: 0.043, b: 0.035, a: 1} + m_AmbientIntensity: 1 + m_AmbientMode: 0 + m_SubtractiveShadowColor: {r: 0.42, g: 0.478, b: 0.627, a: 1} + m_SkyboxMaterial: {fileID: 10304, guid: 0000000000000000f000000000000000, type: 0} + m_HaloStrength: 0.5 + m_FlareStrength: 1 + m_FlareFadeSpeed: 3 + m_HaloTexture: {fileID: 0} + m_SpotCookie: {fileID: 10001, guid: 0000000000000000e000000000000000, type: 0} + m_DefaultReflectionMode: 0 + m_DefaultReflectionResolution: 128 + m_ReflectionBounces: 1 + m_ReflectionIntensity: 1 + m_CustomReflection: {fileID: 0} + m_Sun: {fileID: 0} + m_UseRadianceAmbientProbe: 0 +--- !u!157 &3 +LightmapSettings: + m_ObjectHideFlags: 0 + serializedVersion: 12 + m_GIWorkflowMode: 1 + m_GISettings: + serializedVersion: 2 + m_BounceScale: 1 + m_IndirectOutputScale: 1 + m_AlbedoBoost: 1 + m_EnvironmentLightingMode: 0 + m_EnableBakedLightmaps: 1 + m_EnableRealtimeLightmaps: 0 + m_LightmapEditorSettings: + serializedVersion: 12 + m_Resolution: 2 + m_BakeResolution: 40 + m_AtlasSize: 1024 + m_AO: 0 + m_AOMaxDistance: 1 + m_CompAOExponent: 1 + m_CompAOExponentDirect: 0 + m_ExtractAmbientOcclusion: 0 + m_Padding: 2 + m_LightmapParameters: {fileID: 0} + m_LightmapsBakeMode: 1 + m_TextureCompression: 1 + m_FinalGather: 0 + m_FinalGatherFiltering: 1 + m_FinalGatherRayCount: 256 + m_ReflectionCompression: 2 + m_MixedBakeMode: 2 + m_BakeBackend: 1 + m_PVRSampling: 1 + m_PVRDirectSampleCount: 32 + m_PVRSampleCount: 512 + m_PVRBounces: 2 + m_PVREnvironmentSampleCount: 256 + m_PVREnvironmentReferencePointCount: 2048 + m_PVRFilteringMode: 1 + m_PVRDenoiserTypeDirect: 1 + m_PVRDenoiserTypeIndirect: 1 + m_PVRDenoiserTypeAO: 1 + m_PVRFilterTypeDirect: 0 + m_PVRFilterTypeIndirect: 0 + m_PVRFilterTypeAO: 0 + m_PVREnvironmentMIS: 1 + m_PVRCulling: 1 + m_PVRFilteringGaussRadiusDirect: 1 + m_PVRFilteringGaussRadiusIndirect: 5 + m_PVRFilteringGaussRadiusAO: 2 + m_PVRFilteringAtrousPositionSigmaDirect: 0.5 + m_PVRFilteringAtrousPositionSigmaIndirect: 2 + m_PVRFilteringAtrousPositionSigmaAO: 1 + m_ExportTrainingData: 0 + m_TrainingDataDestination: TrainingData + m_LightProbeSampleCountMultiplier: 4 + m_LightingDataAsset: {fileID: 0} + m_LightingSettings: {fileID: 0} +--- !u!196 &4 +NavMeshSettings: + serializedVersion: 2 + m_ObjectHideFlags: 0 + m_BuildSettings: + serializedVersion: 3 + agentTypeID: 0 + agentRadius: 0.5 + agentHeight: 2 + agentSlope: 45 + agentClimb: 0.4 + ledgeDropHeight: 0 + maxJumpAcrossDistance: 0 + minRegionArea: 2 + manualCellSize: 0 + cellSize: 0.16666667 + manualTileSize: 0 + tileSize: 256 + buildHeightMesh: 0 + maxJobWorkers: 0 + preserveTilesOutsideBounds: 0 + debug: + m_Flags: 0 + m_NavMeshData: {fileID: 0} +--- !u!1 &1055636263 +GameObject: + m_ObjectHideFlags: 0 + m_CorrespondingSourceObject: {fileID: 0} + m_PrefabInstance: {fileID: 0} + m_PrefabAsset: {fileID: 0} + serializedVersion: 6 + m_Component: + - component: {fileID: 1055636266} + - component: {fileID: 1055636265} + - component: {fileID: 1055636264} + m_Layer: 0 + m_Name: Floor + m_TagString: Untagged + m_Icon: {fileID: 0} + m_NavMeshLayer: 0 + m_StaticEditorFlags: 0 + m_IsActive: 1 +--- !u!114 &1055636264 +MonoBehaviour: + m_ObjectHideFlags: 0 + m_CorrespondingSourceObject: {fileID: 0} + m_PrefabInstance: {fileID: 0} + m_PrefabAsset: {fileID: 0} + m_GameObject: {fileID: 1055636263} + m_Enabled: 1 + m_EditorHideFlags: 0 + m_Script: {fileID: 11500000, guid: c46b879d8c5d74011b5c593078c84386, type: 3} + m_Name: + m_EditorClassIdentifier: +--- !u!61 &1055636265 +BoxCollider2D: + m_ObjectHideFlags: 0 + m_CorrespondingSourceObject: {fileID: 0} + m_PrefabInstance: {fileID: 0} + m_PrefabAsset: {fileID: 0} + m_GameObject: {fileID: 1055636263} + m_Enabled: 1 + m_Density: 1 + m_Material: {fileID: 0} + m_IncludeLayers: + serializedVersion: 2 + m_Bits: 0 + m_ExcludeLayers: + serializedVersion: 2 + m_Bits: 0 + m_LayerOverridePriority: 0 + m_ForceSendLayers: + serializedVersion: 2 + m_Bits: 4294967295 + m_ForceReceiveLayers: + serializedVersion: 2 + m_Bits: 4294967295 + m_ContactCaptureLayers: + serializedVersion: 2 + m_Bits: 4294967295 + m_CallbackLayers: + serializedVersion: 2 + m_Bits: 4294967295 + m_IsTrigger: 0 + m_UsedByEffector: 0 + m_UsedByComposite: 0 + m_Offset: {x: 0, y: 0} + m_SpriteTilingProperty: + border: {x: 0, y: 0, z: 0, w: 0} + pivot: {x: 0, y: 0} + oldSize: {x: 0, y: 0} + newSize: {x: 0, y: 0} + adaptiveTilingThreshold: 0 + drawMode: 0 + adaptiveTiling: 0 + m_AutoTiling: 0 + serializedVersion: 2 + m_Size: {x: 10, y: 1} + m_EdgeRadius: 0 +--- !u!4 &1055636266 +Transform: + m_ObjectHideFlags: 0 + m_CorrespondingSourceObject: {fileID: 0} + m_PrefabInstance: {fileID: 0} + m_PrefabAsset: {fileID: 0} + m_GameObject: {fileID: 1055636263} + serializedVersion: 2 + m_LocalRotation: {x: 0, y: 0, z: 0, w: 1} + m_LocalPosition: {x: 0, y: 0, z: 0} + m_LocalScale: {x: 1, y: 1, z: 1} + m_ConstrainProportionsScale: 0 + m_Children: [] + m_Father: {fileID: 0} + m_LocalEulerAnglesHint: {x: 0, y: 0, z: 0} +--- !u!1 &1293603021 +GameObject: + m_ObjectHideFlags: 0 + m_CorrespondingSourceObject: {fileID: 0} + m_PrefabInstance: {fileID: 0} + m_PrefabAsset: {fileID: 0} + serializedVersion: 6 + m_Component: + - component: {fileID: 1293603024} + - component: {fileID: 1293603023} + - component: {fileID: 1293603022} + m_Layer: 0 + m_Name: TriggerZone + m_TagString: Untagged + m_Icon: {fileID: 0} + m_NavMeshLayer: 0 + m_StaticEditorFlags: 0 + m_IsActive: 1 +--- !u!114 &1293603022 +MonoBehaviour: + m_ObjectHideFlags: 0 + m_CorrespondingSourceObject: {fileID: 0} + m_PrefabInstance: {fileID: 0} + m_PrefabAsset: {fileID: 0} + m_GameObject: {fileID: 1293603021} + m_Enabled: 1 + m_EditorHideFlags: 0 + m_Script: {fileID: 11500000, guid: e08ed59dddfc4426188835abe5e75e1c, type: 3} + m_Name: + m_EditorClassIdentifier: +--- !u!61 &1293603023 +BoxCollider2D: + m_ObjectHideFlags: 0 + m_CorrespondingSourceObject: {fileID: 0} + m_PrefabInstance: {fileID: 0} + m_PrefabAsset: {fileID: 0} + m_GameObject: {fileID: 1293603021} + m_Enabled: 1 + m_Density: 1 + m_Material: {fileID: 0} + m_IncludeLayers: + serializedVersion: 2 + m_Bits: 0 + m_ExcludeLayers: + serializedVersion: 2 + m_Bits: 0 + m_LayerOverridePriority: 0 + m_ForceSendLayers: + serializedVersion: 2 + m_Bits: 4294967295 + m_ForceReceiveLayers: + serializedVersion: 2 + m_Bits: 4294967295 + m_ContactCaptureLayers: + serializedVersion: 2 + m_Bits: 4294967295 + m_CallbackLayers: + serializedVersion: 2 + m_Bits: 4294967295 + m_IsTrigger: 1 + m_UsedByEffector: 0 + m_UsedByComposite: 0 + m_Offset: {x: 0, y: 0} + m_SpriteTilingProperty: + border: {x: 0, y: 0, z: 0, w: 0} + pivot: {x: 0, y: 0} + oldSize: {x: 0, y: 0} + newSize: {x: 0, y: 0} + adaptiveTilingThreshold: 0 + drawMode: 0 + adaptiveTiling: 0 + m_AutoTiling: 0 + serializedVersion: 2 + m_Size: {x: 10, y: 0.5} + m_EdgeRadius: 0 +--- !u!4 &1293603024 +Transform: + m_ObjectHideFlags: 0 + m_CorrespondingSourceObject: {fileID: 0} + m_PrefabInstance: {fileID: 0} + m_PrefabAsset: {fileID: 0} + m_GameObject: {fileID: 1293603021} + serializedVersion: 2 + m_LocalRotation: {x: 0, y: 0, z: 0, w: 1} + m_LocalPosition: {x: 0, y: 1.5, z: 0} + m_LocalScale: {x: 1, y: 1, z: 1} + m_ConstrainProportionsScale: 0 + m_Children: [] + m_Father: {fileID: 0} + m_LocalEulerAnglesHint: {x: 0, y: 0, z: 0} +--- !u!1 &1984951461 +GameObject: + m_ObjectHideFlags: 0 + m_CorrespondingSourceObject: {fileID: 0} + m_PrefabInstance: {fileID: 0} + m_PrefabAsset: {fileID: 0} + serializedVersion: 6 + m_Component: + - component: {fileID: 1984951464} + - component: {fileID: 1984951463} + - component: {fileID: 1984951462} + m_Layer: 0 + m_Name: Ball + m_TagString: Untagged + m_Icon: {fileID: 0} + m_NavMeshLayer: 0 + m_StaticEditorFlags: 0 + m_IsActive: 1 +--- !u!50 &1984951462 +Rigidbody2D: + serializedVersion: 4 + m_ObjectHideFlags: 0 + m_CorrespondingSourceObject: {fileID: 0} + m_PrefabInstance: {fileID: 0} + m_PrefabAsset: {fileID: 0} + m_GameObject: {fileID: 1984951461} + m_BodyType: 0 + m_Simulated: 1 + m_UseFullKinematicContacts: 0 + m_UseAutoMass: 0 + m_Mass: 1 + m_LinearDrag: 0 + m_AngularDrag: 0.05 + m_GravityScale: 3 + m_Material: {fileID: 0} + m_IncludeLayers: + serializedVersion: 2 + m_Bits: 0 + m_ExcludeLayers: + serializedVersion: 2 + m_Bits: 0 + m_Interpolate: 0 + m_SleepingMode: 1 + m_CollisionDetection: 0 + m_Constraints: 0 +--- !u!58 &1984951463 +CircleCollider2D: + m_ObjectHideFlags: 0 + m_CorrespondingSourceObject: {fileID: 0} + m_PrefabInstance: {fileID: 0} + m_PrefabAsset: {fileID: 0} + m_GameObject: {fileID: 1984951461} + m_Enabled: 1 + m_Density: 1 + m_Material: {fileID: 0} + m_IncludeLayers: + serializedVersion: 2 + m_Bits: 0 + m_ExcludeLayers: + serializedVersion: 2 + m_Bits: 0 + m_LayerOverridePriority: 0 + m_ForceSendLayers: + serializedVersion: 2 + m_Bits: 4294967295 + m_ForceReceiveLayers: + serializedVersion: 2 + m_Bits: 4294967295 + m_ContactCaptureLayers: + serializedVersion: 2 + m_Bits: 4294967295 + m_CallbackLayers: + serializedVersion: 2 + m_Bits: 4294967295 + m_IsTrigger: 0 + m_UsedByEffector: 0 + m_UsedByComposite: 0 + m_Offset: {x: 0, y: 0} + serializedVersion: 2 + m_Radius: 0.5 +--- !u!4 &1984951464 +Transform: + m_ObjectHideFlags: 0 + m_CorrespondingSourceObject: {fileID: 0} + m_PrefabInstance: {fileID: 0} + m_PrefabAsset: {fileID: 0} + m_GameObject: {fileID: 1984951461} + serializedVersion: 2 + m_LocalRotation: {x: 0, y: 0, z: 0, w: 1} + m_LocalPosition: {x: 0, y: 3, z: 0} + m_LocalScale: {x: 1, y: 1, z: 1} + m_ConstrainProportionsScale: 0 + m_Children: [] + m_Father: {fileID: 0} + m_LocalEulerAnglesHint: {x: 0, y: 0, z: 0} +--- !u!1660057539 &9223372036854775807 +SceneRoots: + m_ObjectHideFlags: 0 + m_Roots: + - {fileID: 1055636266} + - {fileID: 1984951464} + - {fileID: 1293603024} diff --git a/Assets/RegressionHarness/PhysicsCallbackExistingInstance/PhysicsCallbackExistingInstance.unity.meta b/Assets/RegressionHarness/PhysicsCallbackExistingInstance/PhysicsCallbackExistingInstance.unity.meta new file mode 100644 index 0000000000..9abcda04f8 --- /dev/null +++ b/Assets/RegressionHarness/PhysicsCallbackExistingInstance/PhysicsCallbackExistingInstance.unity.meta @@ -0,0 +1,7 @@ +fileFormatVersion: 2 +guid: 509e273f98e504b0abd91aec682f0cce +DefaultImporter: + externalObjects: {} + userData: + assetBundleName: + assetBundleVariant: diff --git a/Assets/RegressionHarness/PhysicsCallbackExistingInstance/PhysicsCallbackFloor.cs b/Assets/RegressionHarness/PhysicsCallbackExistingInstance/PhysicsCallbackFloor.cs new file mode 100644 index 0000000000..d6cb45a0ee --- /dev/null +++ b/Assets/RegressionHarness/PhysicsCallbackExistingInstance/PhysicsCallbackFloor.cs @@ -0,0 +1,19 @@ +using UnityEngine; + +namespace io.github.hatayama.UnityCliLoop.RegressionHarness +{ + // Minimal MonoBehaviour for the PhysicsCallbackExistingInstance regression harness. + // OnCollisionEnter2D is the pause-point marker line: a Harmony patch applied to this method + // after the GameObject already exists in the scene does not intercept Unity's cached physics + // message dispatch for that instance (see docs/regression-harness.md and + // SourcePausePointConstants.PhysicalCallbackMayMissExistingInstanceWarning). + public sealed class PhysicsCallbackFloor : MonoBehaviour + { + public int HitCount { get; private set; } + + private void OnCollisionEnter2D(Collision2D collision) + { + HitCount++; + } + } +} diff --git a/Assets/RegressionHarness/PhysicsCallbackExistingInstance/PhysicsCallbackFloor.cs.meta b/Assets/RegressionHarness/PhysicsCallbackExistingInstance/PhysicsCallbackFloor.cs.meta new file mode 100644 index 0000000000..1858086f05 --- /dev/null +++ b/Assets/RegressionHarness/PhysicsCallbackExistingInstance/PhysicsCallbackFloor.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: c46b879d8c5d74011b5c593078c84386 +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: diff --git a/Assets/RegressionHarness/PhysicsCallbackExistingInstance/PhysicsCallbackTriggerZone.cs b/Assets/RegressionHarness/PhysicsCallbackExistingInstance/PhysicsCallbackTriggerZone.cs new file mode 100644 index 0000000000..16cda5d30d --- /dev/null +++ b/Assets/RegressionHarness/PhysicsCallbackExistingInstance/PhysicsCallbackTriggerZone.cs @@ -0,0 +1,18 @@ +using UnityEngine; + +namespace io.github.hatayama.UnityCliLoop.RegressionHarness +{ + // Trigger-collider counterpart of PhysicsCallbackFloor, added because the reported feedback + // hit this same "existing instance" gap on both an OnCollision* and an OnTrigger* callback -- + // the two dispatch through separate Unity code paths, so the collision-only repro does not + // prove the trigger path shares the same fix. + public sealed class PhysicsCallbackTriggerZone : MonoBehaviour + { + public int HitCount { get; private set; } + + private void OnTriggerEnter2D(Collider2D other) + { + HitCount++; + } + } +} diff --git a/Assets/RegressionHarness/PhysicsCallbackExistingInstance/PhysicsCallbackTriggerZone.cs.meta b/Assets/RegressionHarness/PhysicsCallbackExistingInstance/PhysicsCallbackTriggerZone.cs.meta new file mode 100644 index 0000000000..da96c4aecc --- /dev/null +++ b/Assets/RegressionHarness/PhysicsCallbackExistingInstance/PhysicsCallbackTriggerZone.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: e08ed59dddfc4426188835abe5e75e1c +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 2b36e885b7..4c1a3e8a69 100644 --- a/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointConstants.cs +++ b/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointConstants.cs @@ -68,9 +68,14 @@ internal static class SourcePausePointConstants "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."; + + "hit even though the method body runs. Lightest workaround: after this pause point is " + + "enabled (the patch must already be applied), toggle any one instance's enabled off and " + + "on once. This re-resolves Unity's message dispatch for the whole component type for the " + + "rest of this Editor session (until the next domain reload), fixing every instance, not " + + "just the toggled one. Note the toggle fires OnDisable/OnEnable, so watch for side effects " + + "on components with game logic there. Heavier 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."; // The same cached-dispatch risk as PhysicalCallbackMayMissExistingInstanceWarning, but for a // method that is not itself named after a physics message method and is instead called (one @@ -81,9 +86,14 @@ internal static class SourcePausePointConstants "This method is called from a Unity physics message method (OnCollision*/OnTrigger*/OnParticleCollision) " + "elsewhere in the same compiled assembly. 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."; + + "point may never hit even though the method body runs. Lightest workaround: after this pause point " + + "is enabled (the patch must already be applied), toggle any one instance's enabled off and on once. " + + "This re-resolves Unity's message dispatch for the whole component type for the rest of this Editor " + + "session (until the next domain reload), fixing every instance, not just the toggled one. Note the " + + "toggle fires OnDisable/OnEnable, so watch for side effects on components with game logic there. " + + "Heavier 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."; // Surfaces the same JIT-inlining risk documented under Requirements & Safety in the skill, // but at enable time instead of only after a confusing HitCount=0 timeout. diff --git a/docs/regression-harness.md b/docs/regression-harness.md index 9c14b8d984..4476ead2fc 100644 --- a/docs/regression-harness.md +++ b/docs/regression-harness.md @@ -31,3 +31,4 @@ scenario covers. |------|-------|--------| | Key state divergence after a pause-point interruption | `Assets/RegressionHarness/KeyStateAfterPauseInterruption/` | `scripts/regression-harness-key-state-after-pause-interruption.sh` | | `await-pause-point --trigger` hits within the marker timeout instead of waiting out the triggered command's full duration | `Assets/RegressionHarness/KeyStateAfterPauseInterruption/` (reused) | `scripts/regression-harness-pause-point-trigger.sh` | +| A pause point armed on an OnCollisionEnter2D/OnTriggerEnter2D method misses a GameObject that already existed before arming, and toggling the component's `enabled` off/on resolves it | `Assets/RegressionHarness/PhysicsCallbackExistingInstance/` | `scripts/regression-harness-physics-callback-existing-instance.sh` | diff --git a/scripts/regression-harness-physics-callback-existing-instance.sh b/scripts/regression-harness-physics-callback-existing-instance.sh new file mode 100755 index 0000000000..54a2e850ee --- /dev/null +++ b/scripts/regression-harness-physics-callback-existing-instance.sh @@ -0,0 +1,181 @@ +#!/bin/sh +set -e +# Regression harness for the "existing instance" physics-callback pause-point gap: +# Unity's physics message dispatch (OnCollision*/OnTrigger*) resolves its call path once when a +# GameObject registers with the physics engine, so a Harmony patch applied to an +# OnCollisionEnter2D/OnTriggerEnter2D method after that GameObject already existed does not route +# through the patch. This harness proves the workaround half of that gap: +# toggling one instance's `enabled` off then on (after the pause point is enabled) is enough to +# make Unity re-resolve dispatch and route the next contact on that instance through the patch. +# +# The harness attempts a best-effort fresh domain reload via EditorUtility.RequestScriptReload() +# before arming, since the enabled-toggle workaround re-resolves dispatch for the whole component +# type for the rest of the Editor session -- without a reload, a type "fixed" by an earlier local +# run of this harness (or by any other pause point on the same type) would no longer reproduce the +# baseline miss. In practice this reload's timing relative to Unity's IPC recovery is not fully +# reliable, so the baseline-miss check is informational only (always logged, never asserted); the +# only strict assertion is that the workaround produces a hit. +# See docs/regression-harness.md and +# SourcePausePointConstants.PhysicalCallbackMayMissExistingInstanceWarning. +# +# Usage: sh scripts/regression-harness-physics-callback-existing-instance.sh [--project-path ] +# +# Prerequisites: +# - Assets/RegressionHarness/PhysicsCallbackExistingInstance/PhysicsCallbackExistingInstance.unity +# must be open in a running Unity Editor +# - uloop CLI must be installed +# - jq must be installed + +PROJECT_PATH="" +if [ "$1" = "--project-path" ] && [ -n "$2" ]; then + PROJECT_PATH="$2" +fi + +FLOOR_FILE="Assets/RegressionHarness/PhysicsCallbackExistingInstance/PhysicsCallbackFloor.cs" +FLOOR_LINE="16" +TRIGGER_FILE="Assets/RegressionHarness/PhysicsCallbackExistingInstance/PhysicsCallbackTriggerZone.cs" +TRIGGER_LINE="15" +RESULT_FILE="$(mktemp)" +DYNAMIC_CODE_FILE="$(mktemp)" +RELOAD_RECOVERY_RETRIES=15 +RELOAD_RECOVERY_INTERVAL_SECONDS=2 + +run_uloop() { + if [ -n "$PROJECT_PATH" ]; then + uloop "$@" --project-path "$PROJECT_PATH" + else + uloop "$@" + fi +} + +log() { + printf "\033[36m[physics-callback-existing-instance]\033[0m %s\n" "$1" +} + +cleanup() { + rm -f "$RESULT_FILE" "$DYNAMIC_CODE_FILE" + run_uloop clear-pause-point --all > /dev/null 2>&1 || true + run_uloop control-play-mode --action Stop > /dev/null 2>&1 || true +} +trap cleanup EXIT + +# Forces a fresh domain reload without touching any file, so the component type carries no +# leftover "fixed by a previous toggle" state into this run's baseline-miss check. Unity +# disconnects the IPC bridge for the duration of the reload -- that failure is expected and +# swallowed here; recovery is confirmed by polling a lightweight command with a hard retry cap. +force_clean_domain() { + cat > "$DYNAMIC_CODE_FILE" <<'EOF' +using UnityEditor; + +EditorUtility.RequestScriptReload(); +return "Requested script reload"; +EOF + run_uloop execute-dynamic-code --code-file "$DYNAMIC_CODE_FILE" > /dev/null 2>&1 || true + + log "Waiting for Unity to recover from the domain reload..." + ATTEMPT=1 + while [ "$ATTEMPT" -le "$RELOAD_RECOVERY_RETRIES" ]; do + if run_uloop list > /dev/null 2>&1; then + log "Unity recovered after ${ATTEMPT} attempt(s)." + return 0 + fi + ATTEMPT=$((ATTEMPT + 1)) + sleep "$RELOAD_RECOVERY_INTERVAL_SECONDS" + done + + log "FAIL: Unity did not recover within $((RELOAD_RECOVERY_RETRIES * RELOAD_RECOVERY_INTERVAL_SECONDS))s of requesting a script reload." + exit 1 +} + +reset_ball() { + cat > "$DYNAMIC_CODE_FILE" <<'EOF' +using UnityEngine; + +GameObject ball = GameObject.Find("Ball"); +Rigidbody2D ballBody = ball.GetComponent(); +ballBody.velocity = Vector2.zero; +ball.transform.position = new Vector3(0f, 3f, 0f); +return "ball reset"; +EOF + run_uloop execute-dynamic-code --code-file "$DYNAMIC_CODE_FILE" > /dev/null +} + +toggle_enabled() { + OBJECT_NAME="$1" + COMPONENT_TYPE="$2" + cat > "$DYNAMIC_CODE_FILE" <(); +component.enabled = false; +component.enabled = true; +return "toggled"; +EOF + run_uloop execute-dynamic-code --code-file "$DYNAMIC_CODE_FILE" > /dev/null +} + +# Verifies a pre-existing physics-callback instance (collision or trigger) misses a pause point +# armed after it already exists, then confirms the enabled-toggle workaround makes the next +# contact on that same instance route through the patch. +verify_existing_instance_gap_and_workaround() { + LABEL="$1" + MARKER_FILE="$2" + MARKER_LINE="$3" + OBJECT_NAME="$4" + COMPONENT_TYPE="$5" + MARKER_ID="${MARKER_FILE}:${MARKER_LINE}" + + log "[$LABEL] Arming pause-point on the pre-existing instance..." + run_uloop enable-pause-point --file "$MARKER_FILE" --line "$MARKER_LINE" --timeout-seconds 30 > /dev/null + + sleep 3 + run_uloop pause-point-status --id "$MARKER_ID" > "$RESULT_FILE" + BASELINE_IS_HIT="$(jq -r '.IsHit' "$RESULT_FILE")" + + # Informational only: the enabled-toggle workaround re-resolves dispatch for the whole + # component type for the rest of the Editor session, so whether the pre-existing instance + # still misses here depends on whether an earlier run (or any other armed pause point) already + # fixed this type since the last real domain reload. Not asserted -- see the file header. + if [ "$BASELINE_IS_HIT" = "false" ]; then + log "[$LABEL] Baseline miss reproduced (IsHit=false) as documented." + else + log "[$LABEL] INFO: baseline miss did not reproduce this run (the component type was likely already re-resolved earlier in this Editor session; this is expected after a prior run without a domain reload) -- proceeding to workaround assertion." + fi + + log "[$LABEL] Toggling enabled off/on and re-triggering contact..." + toggle_enabled "$OBJECT_NAME" "$COMPONENT_TYPE" + reset_ball + + sleep 3 + run_uloop pause-point-status --id "$MARKER_ID" > "$RESULT_FILE" + WORKAROUND_IS_HIT="$(jq -r '.IsHit' "$RESULT_FILE")" + + if [ "$WORKAROUND_IS_HIT" != "true" ]; then + log "[$LABEL] FAIL: expected the enabled-toggle workaround to make the pause point hit (IsHit=true), got IsHit=$WORKAROUND_IS_HIT" + cat "$RESULT_FILE" + exit 1 + fi + log "[$LABEL] PASS: enabled-toggle workaround resolved the existing-instance gap." + + run_uloop clear-pause-point --all > /dev/null +} + +log "Stopping any existing Play Mode session..." +run_uloop control-play-mode --action Stop > /dev/null + +log "Clearing any existing pause-point markers..." +run_uloop clear-pause-point --all > /dev/null + +force_clean_domain + +log "Starting Play Mode (Floor/TriggerZone/Ball already exist before any pause point is armed)..." +run_uloop control-play-mode --action Play > /dev/null + +verify_existing_instance_gap_and_workaround "OnCollisionEnter2D" "$FLOOR_FILE" "$FLOOR_LINE" "Floor" "PhysicsCallbackFloor" +reset_ball +verify_existing_instance_gap_and_workaround "OnTriggerEnter2D" "$TRIGGER_FILE" "$TRIGGER_LINE" "TriggerZone" "PhysicsCallbackTriggerZone" + +log "PASS: existing-instance gap and enabled-toggle workaround confirmed for both OnCollisionEnter2D and OnTriggerEnter2D." +exit 0 From 7b3988875fcd5305f8aabffb4f9c2d0e920db024 Mon Sep 17 00:00:00 2001 From: hatayama Date: Tue, 21 Jul 2026 22:52:54 +0900 Subject: [PATCH 2/7] Detect domain reload completion via a static marker instead of IPC polling EditorUtility.RequestScriptReload() queues the reload for a later editor update rather than running it inline, so polling for bare IPC responsiveness is not proof the reload actually happened -- both a still-loading domain and the still-alive pre-reload domain can answer a lightweight command successfully. Add HarnessDomainMarker, a static readonly value that is only re-initialized when the AppDomain actually reloads, and have the harness poll for that value changing instead. This still does not make the harness's baseline-miss check reproducible (see the investigation in PR #1922) -- left as-is pending further investigation. --- .../HarnessDomainMarker.cs | 12 ++++ .../HarnessDomainMarker.cs.meta | 11 ++++ ...ness-physics-callback-existing-instance.sh | 60 ++++++++++++------- 3 files changed, 62 insertions(+), 21 deletions(-) create mode 100644 Assets/RegressionHarness/PhysicsCallbackExistingInstance/HarnessDomainMarker.cs create mode 100644 Assets/RegressionHarness/PhysicsCallbackExistingInstance/HarnessDomainMarker.cs.meta diff --git a/Assets/RegressionHarness/PhysicsCallbackExistingInstance/HarnessDomainMarker.cs b/Assets/RegressionHarness/PhysicsCallbackExistingInstance/HarnessDomainMarker.cs new file mode 100644 index 0000000000..50fb4ab69f --- /dev/null +++ b/Assets/RegressionHarness/PhysicsCallbackExistingInstance/HarnessDomainMarker.cs @@ -0,0 +1,12 @@ +namespace io.github.hatayama.UnityCliLoop.RegressionHarness +{ + // A static readonly value is re-initialized whenever the AppDomain reloads, so comparing this + // value before and after a EditorUtility.RequestScriptReload() call is a deterministic way to + // detect that the reload has actually completed -- unlike polling for IPC responsiveness alone, + // which can observe the pre-reload domain still answering before the reload has even started + // (RequestScriptReload queues the reload for a later editor update, it does not run inline). + public static class HarnessDomainMarker + { + public static readonly string Id = System.Guid.NewGuid().ToString(); + } +} diff --git a/Assets/RegressionHarness/PhysicsCallbackExistingInstance/HarnessDomainMarker.cs.meta b/Assets/RegressionHarness/PhysicsCallbackExistingInstance/HarnessDomainMarker.cs.meta new file mode 100644 index 0000000000..a433d0b50d --- /dev/null +++ b/Assets/RegressionHarness/PhysicsCallbackExistingInstance/HarnessDomainMarker.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: 489253cc4b1e54844a8dd6cfa77ff109 +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: diff --git a/scripts/regression-harness-physics-callback-existing-instance.sh b/scripts/regression-harness-physics-callback-existing-instance.sh index 54a2e850ee..49e9fdfe14 100755 --- a/scripts/regression-harness-physics-callback-existing-instance.sh +++ b/scripts/regression-harness-physics-callback-existing-instance.sh @@ -8,13 +8,15 @@ set -e # toggling one instance's `enabled` off then on (after the pause point is enabled) is enough to # make Unity re-resolve dispatch and route the next contact on that instance through the patch. # -# The harness attempts a best-effort fresh domain reload via EditorUtility.RequestScriptReload() -# before arming, since the enabled-toggle workaround re-resolves dispatch for the whole component -# type for the rest of the Editor session -- without a reload, a type "fixed" by an earlier local -# run of this harness (or by any other pause point on the same type) would no longer reproduce the -# baseline miss. In practice this reload's timing relative to Unity's IPC recovery is not fully -# reliable, so the baseline-miss check is informational only (always logged, never asserted); the -# only strict assertion is that the workaround produces a hit. +# The harness forces a fresh domain reload via EditorUtility.RequestScriptReload() before arming, +# since the enabled-toggle workaround re-resolves dispatch for the whole component type for the +# rest of the Editor session -- without a reload, a type "fixed" by an earlier local run of this +# harness (or by any other pause point on the same type) would not reproduce the baseline miss. +# RequestScriptReload queues the reload for a later editor update rather than running it inline, +# so polling for bare IPC responsiveness is not sufficient -- a still-loading-the-reload domain +# and the still-alive pre-reload domain both answer "list" successfully. Reload completion is +# instead detected via HarnessDomainMarker.Id, a static readonly value that is re-initialized only +# when the AppDomain actually reloads; the harness waits until this value changes. # See docs/regression-harness.md and # SourcePausePointConstants.PhysicalCallbackMayMissExistingInstanceWarning. # @@ -59,11 +61,29 @@ cleanup() { } trap cleanup EXIT +read_domain_marker() { + cat > "$DYNAMIC_CODE_FILE" <<'EOF' +using io.github.hatayama.UnityCliLoop.RegressionHarness; + +return HarnessDomainMarker.Id; +EOF + run_uloop execute-dynamic-code --code-file "$DYNAMIC_CODE_FILE" 2>/dev/null | jq -r '.Result // empty' +} + # Forces a fresh domain reload without touching any file, so the component type carries no # leftover "fixed by a previous toggle" state into this run's baseline-miss check. Unity # disconnects the IPC bridge for the duration of the reload -- that failure is expected and -# swallowed here; recovery is confirmed by polling a lightweight command with a hard retry cap. +# swallowed here. RequestScriptReload queues the reload for a later editor update rather than +# running it inline, so bare IPC responsiveness is not proof the reload happened -- completion is +# instead detected by HarnessDomainMarker.Id (a static readonly value) changing from its +# pre-reload reading, with a hard retry cap. force_clean_domain() { + BEFORE_MARKER="$(read_domain_marker)" + if [ -z "$BEFORE_MARKER" ]; then + log "FAIL: could not read HarnessDomainMarker.Id before requesting a script reload." + exit 1 + fi + cat > "$DYNAMIC_CODE_FILE" <<'EOF' using UnityEditor; @@ -72,18 +92,19 @@ return "Requested script reload"; EOF run_uloop execute-dynamic-code --code-file "$DYNAMIC_CODE_FILE" > /dev/null 2>&1 || true - log "Waiting for Unity to recover from the domain reload..." + log "Waiting for the domain marker to change (proves the reload actually completed)..." ATTEMPT=1 while [ "$ATTEMPT" -le "$RELOAD_RECOVERY_RETRIES" ]; do - if run_uloop list > /dev/null 2>&1; then - log "Unity recovered after ${ATTEMPT} attempt(s)." + sleep "$RELOAD_RECOVERY_INTERVAL_SECONDS" + AFTER_MARKER="$(read_domain_marker)" + if [ -n "$AFTER_MARKER" ] && [ "$AFTER_MARKER" != "$BEFORE_MARKER" ]; then + log "Domain reload confirmed after ${ATTEMPT} attempt(s) (marker changed)." return 0 fi ATTEMPT=$((ATTEMPT + 1)) - sleep "$RELOAD_RECOVERY_INTERVAL_SECONDS" done - log "FAIL: Unity did not recover within $((RELOAD_RECOVERY_RETRIES * RELOAD_RECOVERY_INTERVAL_SECONDS))s of requesting a script reload." + log "FAIL: HarnessDomainMarker.Id did not change within $((RELOAD_RECOVERY_RETRIES * RELOAD_RECOVERY_INTERVAL_SECONDS))s of requesting a script reload." exit 1 } @@ -134,15 +155,12 @@ verify_existing_instance_gap_and_workaround() { run_uloop pause-point-status --id "$MARKER_ID" > "$RESULT_FILE" BASELINE_IS_HIT="$(jq -r '.IsHit' "$RESULT_FILE")" - # Informational only: the enabled-toggle workaround re-resolves dispatch for the whole - # component type for the rest of the Editor session, so whether the pre-existing instance - # still misses here depends on whether an earlier run (or any other armed pause point) already - # fixed this type since the last real domain reload. Not asserted -- see the file header. - if [ "$BASELINE_IS_HIT" = "false" ]; then - log "[$LABEL] Baseline miss reproduced (IsHit=false) as documented." - else - log "[$LABEL] INFO: baseline miss did not reproduce this run (the component type was likely already re-resolved earlier in this Editor session; this is expected after a prior run without a domain reload) -- proceeding to workaround assertion." + if [ "$BASELINE_IS_HIT" != "false" ]; then + log "[$LABEL] FAIL: expected the pre-existing instance to miss the pause point (IsHit=false), got IsHit=$BASELINE_IS_HIT" + cat "$RESULT_FILE" + exit 1 fi + log "[$LABEL] Baseline miss confirmed (IsHit=false) as documented." log "[$LABEL] Toggling enabled off/on and re-triggering contact..." toggle_enabled "$OBJECT_NAME" "$COMPONENT_TYPE" From aadc24dded78c49e21fcf419fb06285c6994770b Mon Sep 17 00:00:00 2001 From: hatayama Date: Tue, 21 Jul 2026 23:29:29 +0900 Subject: [PATCH 3/7] Make the physics-callback harness's baseline check conditional and add an indirect-callee scenario A confirmed-fresh domain reload (verified via a static marker that only changes when the AppDomain actually reloads) did not reliably reproduce the pre-existing-instance dispatch miss on every run -- reproduction is environment-dependent. The harness now treats the baseline-miss check as conditional: if a miss is observed, the enabled-toggle workaround is strictly asserted to fix it; if not, that is logged and the run passes without exercising the workaround assertion. Also add an indirect-callee scenario (a one-hop method called from OnCollisionEnter2D, primed with one prior contact before arming), matching the dominant real-world pattern where the pause point marker sits in a helper method rather than the physics callback itself. --- .../PhysicsCallbackFloor.cs | 20 ++++- docs/regression-harness.md | 19 ++++- ...ness-physics-callback-existing-instance.sh | 73 +++++++++++-------- 3 files changed, 78 insertions(+), 34 deletions(-) diff --git a/Assets/RegressionHarness/PhysicsCallbackExistingInstance/PhysicsCallbackFloor.cs b/Assets/RegressionHarness/PhysicsCallbackExistingInstance/PhysicsCallbackFloor.cs index d6cb45a0ee..28506b64db 100644 --- a/Assets/RegressionHarness/PhysicsCallbackExistingInstance/PhysicsCallbackFloor.cs +++ b/Assets/RegressionHarness/PhysicsCallbackExistingInstance/PhysicsCallbackFloor.cs @@ -3,17 +3,29 @@ namespace io.github.hatayama.UnityCliLoop.RegressionHarness { // Minimal MonoBehaviour for the PhysicsCallbackExistingInstance regression harness. - // OnCollisionEnter2D is the pause-point marker line: a Harmony patch applied to this method - // after the GameObject already exists in the scene does not intercept Unity's cached physics - // message dispatch for that instance (see docs/regression-harness.md and - // SourcePausePointConstants.PhysicalCallbackMayMissExistingInstanceWarning). + // OnCollisionEnter2D holds the direct pause-point marker line: a Harmony patch applied to this + // method after the GameObject already exists in the scene has been observed in real projects + // to miss Unity's physics message dispatch for that instance. The trigger condition is + // environment-dependent and does not reproduce deterministically (see docs/regression-harness.md + // and SourcePausePointConstants.PhysicalCallbackMayMissExistingInstanceWarning). public sealed class PhysicsCallbackFloor : MonoBehaviour { public int HitCount { get; private set; } + public int IndirectHitCount { get; private set; } + private void OnCollisionEnter2D(Collision2D collision) { HitCount++; + RecordIndirectHit(); + } + + // One-hop indirect callee: the dominant miss pattern reported from real games places the + // pause-point marker in a small method called from the physics callback rather than in the + // callback itself, so the harness probes this shape as a separate scenario. + private void RecordIndirectHit() + { + IndirectHitCount++; } } } diff --git a/docs/regression-harness.md b/docs/regression-harness.md index 4476ead2fc..a0790b0b1f 100644 --- a/docs/regression-harness.md +++ b/docs/regression-harness.md @@ -31,4 +31,21 @@ scenario covers. |------|-------|--------| | Key state divergence after a pause-point interruption | `Assets/RegressionHarness/KeyStateAfterPauseInterruption/` | `scripts/regression-harness-key-state-after-pause-interruption.sh` | | `await-pause-point --trigger` hits within the marker timeout instead of waiting out the triggered command's full duration | `Assets/RegressionHarness/KeyStateAfterPauseInterruption/` (reused) | `scripts/regression-harness-pause-point-trigger.sh` | -| A pause point armed on an OnCollisionEnter2D/OnTriggerEnter2D method misses a GameObject that already existed before arming, and toggling the component's `enabled` off/on resolves it | `Assets/RegressionHarness/PhysicsCallbackExistingInstance/` | `scripts/regression-harness-physics-callback-existing-instance.sh` | +| A pause point armed on a physics message method (or a method called one hop from one) misses a GameObject that already existed before arming; toggling the component's `enabled` off/on resolves it. The miss itself is environment-dependent and does not reproduce deterministically (see below) -- the harness runs three scenarios (direct/OnCollisionEnter2D, indirect callee with priming, and OnTriggerEnter2D) and asserts the workaround only when a miss is actually observed | `Assets/RegressionHarness/PhysicsCallbackExistingInstance/` | `scripts/regression-harness-physics-callback-existing-instance.sh` | + +### Physics-callback existing-instance miss: environment-dependent, not deterministic + +The following were ruled out as the deterministic trigger condition through controlled +experiments (each patched correctly, i.e. did not reproduce the miss): a fresh domain reload +immediately before arming, priming the JIT before arming, arming right after a script compile, +arming in a freshly launched Editor process, arming in a freshly launched process right after a +compile, arming on a runtime-`AddComponent`-created instance, and arming a one-hop indirect +callee with the instance primed by one prior contact. The miss is real (it has been observed and +the enabled-toggle workaround has been confirmed to fix it, including a controlled two-instance +experiment proving the fix applies to the whole component type, not just the toggled instance), +but no deterministic reproduction recipe is known. `PausePointTools` logs a +`pause_point_physics_dispatch_diagnostics` VibeLogger entry whenever a physics-flagged pause +point is enabled (and a `pause_point_expired_without_hit_physics` entry if it later expires +without ever hitting), capturing Play Mode state, seconds since the last domain reload, the +declaring type, and its current instance count -- if the miss recurs, this is the primary +evidence to work from. diff --git a/scripts/regression-harness-physics-callback-existing-instance.sh b/scripts/regression-harness-physics-callback-existing-instance.sh index 49e9fdfe14..44326212ac 100755 --- a/scripts/regression-harness-physics-callback-existing-instance.sh +++ b/scripts/regression-harness-physics-callback-existing-instance.sh @@ -1,22 +1,31 @@ #!/bin/sh set -e # Regression harness for the "existing instance" physics-callback pause-point gap: -# Unity's physics message dispatch (OnCollision*/OnTrigger*) resolves its call path once when a -# GameObject registers with the physics engine, so a Harmony patch applied to an -# OnCollisionEnter2D/OnTriggerEnter2D method after that GameObject already existed does not route -# through the patch. This harness proves the workaround half of that gap: -# toggling one instance's `enabled` off then on (after the pause point is enabled) is enough to -# make Unity re-resolve dispatch and route the next contact on that instance through the patch. +# Unity's physics message dispatch (OnCollision*/OnTrigger*) has been observed in real projects to +# bypass a Harmony patch applied while the GameObject already existed, so the pause point can +# silently miss even though the method body runs. The trigger condition is environment-dependent +# and has NOT been reproduced deterministically here (see docs/regression-harness.md for the list +# of ruled-out hypotheses) -- this harness therefore treats the baseline-miss check as conditional: +# - if the pre-existing instance misses the pause point (the miss reproduces in this run), the +# enabled-toggle workaround is strictly asserted to fix it +# - if it does not miss (the common case in this environment), that is logged and this run passes +# without exercising the workaround assertion +# It covers three call shapes: +# 1. direct -- pause point on the physics message method itself (OnCollisionEnter2D) +# 2. indirect -- pause point on a method called one hop deep from the physics message method, +# with the instance primed (one prior contact before arming) to match the dominant pattern +# reported from real games, where the marker sits in a helper method rather than the callback +# 3. trigger -- pause point on OnTriggerEnter2D, a separate Unity dispatch path from OnCollision* # # The harness forces a fresh domain reload via EditorUtility.RequestScriptReload() before arming, -# since the enabled-toggle workaround re-resolves dispatch for the whole component type for the -# rest of the Editor session -- without a reload, a type "fixed" by an earlier local run of this -# harness (or by any other pause point on the same type) would not reproduce the baseline miss. -# RequestScriptReload queues the reload for a later editor update rather than running it inline, -# so polling for bare IPC responsiveness is not sufficient -- a still-loading-the-reload domain -# and the still-alive pre-reload domain both answer "list" successfully. Reload completion is -# instead detected via HarnessDomainMarker.Id, a static readonly value that is re-initialized only -# when the AppDomain actually reloads; the harness waits until this value changes. +# since the enabled-toggle workaround (when it applies) re-resolves dispatch for the whole +# component type -- without a reload, a type already "fixed" by an earlier local run of this +# harness would carry that fix into this run. RequestScriptReload queues the reload for a later +# editor update rather than running it inline, so polling for bare IPC responsiveness is not +# sufficient -- a still-loading-the-reload domain and the still-alive pre-reload domain both answer +# "list" successfully. Reload completion is instead detected via HarnessDomainMarker.Id, a static +# readonly value that is re-initialized only when the AppDomain actually reloads; the harness waits +# until this value changes. # See docs/regression-harness.md and # SourcePausePointConstants.PhysicalCallbackMayMissExistingInstanceWarning. # @@ -34,7 +43,8 @@ if [ "$1" = "--project-path" ] && [ -n "$2" ]; then fi FLOOR_FILE="Assets/RegressionHarness/PhysicsCallbackExistingInstance/PhysicsCallbackFloor.cs" -FLOOR_LINE="16" +FLOOR_DIRECT_LINE="19" +FLOOR_INDIRECT_LINE="28" TRIGGER_FILE="Assets/RegressionHarness/PhysicsCallbackExistingInstance/PhysicsCallbackTriggerZone.cs" TRIGGER_LINE="15" RESULT_FILE="$(mktemp)" @@ -137,10 +147,11 @@ EOF run_uloop execute-dynamic-code --code-file "$DYNAMIC_CODE_FILE" > /dev/null } -# Verifies a pre-existing physics-callback instance (collision or trigger) misses a pause point -# armed after it already exists, then confirms the enabled-toggle workaround makes the next -# contact on that same instance route through the patch. -verify_existing_instance_gap_and_workaround() { +# Waits for the ball to make contact and settle, then arms a pause point and reports whether the +# pre-existing instance missed it. If it missed, strictly asserts the enabled-toggle workaround +# fixes it. If it did not miss, logs that this run did not reproduce the trap (expected in an +# environment where the trigger condition does not apply) and returns without asserting further. +verify_conditional_gap_and_workaround() { LABEL="$1" MARKER_FILE="$2" MARKER_LINE="$3" @@ -155,12 +166,12 @@ verify_existing_instance_gap_and_workaround() { run_uloop pause-point-status --id "$MARKER_ID" > "$RESULT_FILE" BASELINE_IS_HIT="$(jq -r '.IsHit' "$RESULT_FILE")" - if [ "$BASELINE_IS_HIT" != "false" ]; then - log "[$LABEL] FAIL: expected the pre-existing instance to miss the pause point (IsHit=false), got IsHit=$BASELINE_IS_HIT" - cat "$RESULT_FILE" - exit 1 + if [ "$BASELINE_IS_HIT" = "true" ]; then + log "[$LABEL] PASS (trap did not reproduce in this environment; environment-dependent, see PR #1922): the pre-existing instance was hit on arming without needing the enabled-toggle workaround." + run_uloop clear-pause-point --all > /dev/null + return 0 fi - log "[$LABEL] Baseline miss confirmed (IsHit=false) as documented." + log "[$LABEL] Baseline miss reproduced (IsHit=false) -- asserting the enabled-toggle workaround strictly." log "[$LABEL] Toggling enabled off/on and re-triggering contact..." toggle_enabled "$OBJECT_NAME" "$COMPONENT_TYPE" @@ -171,11 +182,11 @@ verify_existing_instance_gap_and_workaround() { WORKAROUND_IS_HIT="$(jq -r '.IsHit' "$RESULT_FILE")" if [ "$WORKAROUND_IS_HIT" != "true" ]; then - log "[$LABEL] FAIL: expected the enabled-toggle workaround to make the pause point hit (IsHit=true), got IsHit=$WORKAROUND_IS_HIT" + log "[$LABEL] FAIL: reproduced the baseline miss but the enabled-toggle workaround did not make the pause point hit (IsHit=$WORKAROUND_IS_HIT)" cat "$RESULT_FILE" exit 1 fi - log "[$LABEL] PASS: enabled-toggle workaround resolved the existing-instance gap." + log "[$LABEL] PASS: reproduced the baseline miss and the enabled-toggle workaround resolved it." run_uloop clear-pause-point --all > /dev/null } @@ -191,9 +202,13 @@ force_clean_domain log "Starting Play Mode (Floor/TriggerZone/Ball already exist before any pause point is armed)..." run_uloop control-play-mode --action Play > /dev/null -verify_existing_instance_gap_and_workaround "OnCollisionEnter2D" "$FLOOR_FILE" "$FLOOR_LINE" "Floor" "PhysicsCallbackFloor" +log "[indirect] Priming Floor with one contact before the indirect callee is armed..." +sleep 3 + +verify_conditional_gap_and_workaround "direct" "$FLOOR_FILE" "$FLOOR_DIRECT_LINE" "Floor" "PhysicsCallbackFloor" +verify_conditional_gap_and_workaround "indirect" "$FLOOR_FILE" "$FLOOR_INDIRECT_LINE" "Floor" "PhysicsCallbackFloor" reset_ball -verify_existing_instance_gap_and_workaround "OnTriggerEnter2D" "$TRIGGER_FILE" "$TRIGGER_LINE" "TriggerZone" "PhysicsCallbackTriggerZone" +verify_conditional_gap_and_workaround "trigger" "$TRIGGER_FILE" "$TRIGGER_LINE" "TriggerZone" "PhysicsCallbackTriggerZone" -log "PASS: existing-instance gap and enabled-toggle workaround confirmed for both OnCollisionEnter2D and OnTriggerEnter2D." +log "PASS: direct, indirect, and trigger physics-callback scenarios all handled (reproduced-and-fixed, or did-not-reproduce)." exit 0 From 9c802d87828120d3581e19e9956fb3e5fdd5478e Mon Sep 17 00:00:00 2001 From: hatayama Date: Tue, 21 Jul 2026 23:30:00 +0900 Subject: [PATCH 4/7] Add physics-callback dispatch diagnostics for a future recurrence The existing-instance physics-callback dispatch miss investigated in this PR turned out to be environment-dependent with no deterministic reproduction found (see docs/regression-harness.md for the ruled-out hypotheses). Since the miss is real but rare, add diagnostics so a future recurrence carries evidence instead of just a symptom: - SourcePausePointPatcher now reports whether a patch carries a physics- callback warning, plus the declaring type, alongside the existing warning text (SourcePausePointPatchResult.HasPhysicsCallbackWarning/DeclaringType) - PausePointDomainReloadTracker captures this AppDomain's load time via [InitializeOnLoad], so diagnostics can report how long the domain has been alive without a reload - PausePointTools logs a pause_point_physics_dispatch_diagnostics VibeLogger entry whenever a physics-flagged pause point is enabled (Play Mode state, seconds since domain reload, declaring type, and -- for MonoBehaviour-derived types -- current scene instance count), and a pause_point_expired_without_hit_physics entry if it later expires without ever hitting - Update the Warning constants: the enabled-toggle workaround's effect was previously described as lasting "until the next domain reload" based on a reload-timing theory that further investigation ruled out; it now says the miss has not been observed to recur once re-resolved, without claiming a specific expiry condition - PausePointPhysicsDispatchDiagnostics.ResolveInstanceCount is unit tested as the one pure piece of this diagnostics logic (whether counting applies to a non-MonoBehaviour declaring type) --- ...usePointPhysicsDispatchDiagnosticsTests.cs | 35 ++++++++++++++ ...intPhysicsDispatchDiagnosticsTests.cs.meta | 11 +++++ .../PausePointDomainReloadTracker.cs | 21 ++++++++ .../PausePointDomainReloadTracker.cs.meta | 11 +++++ .../PausePointPhysicsDispatchDiagnostics.cs | 17 +++++++ ...usePointPhysicsDispatchDiagnostics.cs.meta | 11 +++++ .../PausePoint/PausePointTools.cs | 48 +++++++++++++++++++ .../PausePoint/SourcePausePointConstants.cs | 22 +++++---- .../PausePoint/SourcePausePointPatchResult.cs | 19 ++++++-- .../PausePoint/SourcePausePointPatcher.cs | 11 +++-- .../Api/PausePointStatusBridgeCommand.cs | 5 +- 11 files changed, 192 insertions(+), 19 deletions(-) create mode 100644 Assets/Tests/Editor/SourcePausePointPatcher/PausePointPhysicsDispatchDiagnosticsTests.cs create mode 100644 Assets/Tests/Editor/SourcePausePointPatcher/PausePointPhysicsDispatchDiagnosticsTests.cs.meta create mode 100644 Packages/src/Editor/FirstPartyTools/PausePoint/PausePointDomainReloadTracker.cs create mode 100644 Packages/src/Editor/FirstPartyTools/PausePoint/PausePointDomainReloadTracker.cs.meta create mode 100644 Packages/src/Editor/FirstPartyTools/PausePoint/PausePointPhysicsDispatchDiagnostics.cs create mode 100644 Packages/src/Editor/FirstPartyTools/PausePoint/PausePointPhysicsDispatchDiagnostics.cs.meta diff --git a/Assets/Tests/Editor/SourcePausePointPatcher/PausePointPhysicsDispatchDiagnosticsTests.cs b/Assets/Tests/Editor/SourcePausePointPatcher/PausePointPhysicsDispatchDiagnosticsTests.cs new file mode 100644 index 0000000000..27fc6deabb --- /dev/null +++ b/Assets/Tests/Editor/SourcePausePointPatcher/PausePointPhysicsDispatchDiagnosticsTests.cs @@ -0,0 +1,35 @@ +using NUnit.Framework; + +using io.github.hatayama.UnityCliLoop.FirstPartyTools; + +namespace io.github.hatayama.UnityCliLoop.Tests.Editor +{ + /// + /// Verifies the pure instance-count resolution rule used by physics-callback dispatch + /// diagnostics: counting only applies when the declaring type is a MonoBehaviour. + /// + [TestFixture] + public sealed class PausePointPhysicsDispatchDiagnosticsTests + { + // What: a MonoBehaviour-derived declaring type reports the actual scene instance count. + [Test] + public void ResolveInstanceCount_WithMonoBehaviourDerivedType_ReturnsActualCount() + { + int result = PausePointPhysicsDispatchDiagnostics.ResolveInstanceCount( + isMonoBehaviourDerived: true, monoBehaviourInstanceCount: 3); + + Assert.AreEqual(3, result); + } + + // What: a non-MonoBehaviour declaring type reports -1, since instance counting has no + // meaning for it (the diagnostics exist for MonoBehaviour physics message dispatch only). + [Test] + public void ResolveInstanceCount_WithNonMonoBehaviourType_ReturnsNegativeOne() + { + int result = PausePointPhysicsDispatchDiagnostics.ResolveInstanceCount( + isMonoBehaviourDerived: false, monoBehaviourInstanceCount: 0); + + Assert.AreEqual(-1, result); + } + } +} diff --git a/Assets/Tests/Editor/SourcePausePointPatcher/PausePointPhysicsDispatchDiagnosticsTests.cs.meta b/Assets/Tests/Editor/SourcePausePointPatcher/PausePointPhysicsDispatchDiagnosticsTests.cs.meta new file mode 100644 index 0000000000..5f72822fd1 --- /dev/null +++ b/Assets/Tests/Editor/SourcePausePointPatcher/PausePointPhysicsDispatchDiagnosticsTests.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: 9c842ed1c5896499482af8bac4e9fa19 +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: diff --git a/Packages/src/Editor/FirstPartyTools/PausePoint/PausePointDomainReloadTracker.cs b/Packages/src/Editor/FirstPartyTools/PausePoint/PausePointDomainReloadTracker.cs new file mode 100644 index 0000000000..264d99fce5 --- /dev/null +++ b/Packages/src/Editor/FirstPartyTools/PausePoint/PausePointDomainReloadTracker.cs @@ -0,0 +1,21 @@ +using System; + +using UnityEditor; + +namespace io.github.hatayama.UnityCliLoop.FirstPartyTools +{ + // [InitializeOnLoad] runs this type's static field initializers once per AppDomain load, so + // LoadedAtUtc marks this domain's birth. Used by physics-callback dispatch diagnostics to + // report how long the current domain has been alive without a reload -- a suspected factor in + // the existing-instance physics-dispatch miss (see docs/regression-harness.md). + [InitializeOnLoad] + internal static class PausePointDomainReloadTracker + { + public static readonly DateTime LoadedAtUtc = DateTime.UtcNow; + + public static double SecondsSinceLoad() + { + return (DateTime.UtcNow - LoadedAtUtc).TotalSeconds; + } + } +} diff --git a/Packages/src/Editor/FirstPartyTools/PausePoint/PausePointDomainReloadTracker.cs.meta b/Packages/src/Editor/FirstPartyTools/PausePoint/PausePointDomainReloadTracker.cs.meta new file mode 100644 index 0000000000..ffaba88aee --- /dev/null +++ b/Packages/src/Editor/FirstPartyTools/PausePoint/PausePointDomainReloadTracker.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: f2a87356467aa4b4c9776c8627463baf +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: diff --git a/Packages/src/Editor/FirstPartyTools/PausePoint/PausePointPhysicsDispatchDiagnostics.cs b/Packages/src/Editor/FirstPartyTools/PausePoint/PausePointPhysicsDispatchDiagnostics.cs new file mode 100644 index 0000000000..7ab3ec75ca --- /dev/null +++ b/Packages/src/Editor/FirstPartyTools/PausePoint/PausePointPhysicsDispatchDiagnostics.cs @@ -0,0 +1,17 @@ +namespace io.github.hatayama.UnityCliLoop.FirstPartyTools +{ + /// + /// Pure part of the physics-callback dispatch diagnostics snapshot: whether an instance count + /// is meaningful for the declaring type at all. + /// + internal static class PausePointPhysicsDispatchDiagnostics + { + // -1 signals "not applicable": counting instances only means something when the declaring + // type is a MonoBehaviour (the physics dispatch miss this diagnostic exists for is scoped + // to MonoBehaviour physics message methods). + public static int ResolveInstanceCount(bool isMonoBehaviourDerived, int monoBehaviourInstanceCount) + { + return isMonoBehaviourDerived ? monoBehaviourInstanceCount : -1; + } + } +} diff --git a/Packages/src/Editor/FirstPartyTools/PausePoint/PausePointPhysicsDispatchDiagnostics.cs.meta b/Packages/src/Editor/FirstPartyTools/PausePoint/PausePointPhysicsDispatchDiagnostics.cs.meta new file mode 100644 index 0000000000..0045255cf6 --- /dev/null +++ b/Packages/src/Editor/FirstPartyTools/PausePoint/PausePointPhysicsDispatchDiagnostics.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: 4f795930c12b141529a9321c1fe203cd +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: diff --git a/Packages/src/Editor/FirstPartyTools/PausePoint/PausePointTools.cs b/Packages/src/Editor/FirstPartyTools/PausePoint/PausePointTools.cs index 97da069b81..3d6ef88c75 100644 --- a/Packages/src/Editor/FirstPartyTools/PausePoint/PausePointTools.cs +++ b/Packages/src/Editor/FirstPartyTools/PausePoint/PausePointTools.cs @@ -6,6 +6,7 @@ using System.Threading.Tasks; using UnityEditor; using UnityEditor.Compilation; +using UnityEngine; using io.github.hatayama.UnityCliLoop.Runtime; using io.github.hatayama.UnityCliLoop.ToolContracts; @@ -271,6 +272,13 @@ protected override Task ExecuteAsync(ClearPausePointSchema p /// internal sealed class PausePointUseCase { + // Tracks which currently-armed source pause point ids carry a physics-callback warning, + // and their declaring type, so a later expiry (LogExpired) can attribute the same + // diagnostics snapshot to a miss that was never hit. Volatile by design: a domain reload + // clears the Harmony patches this tracks anyway, so this dictionary does not need to + // survive one, and entries are removed as soon as their pause point is cleared. + private static readonly Dictionary PhysicsFlaggedDeclaringTypesById = new(); + public PausePointResponse Enable(EnablePausePointSchema parameters) { string captureSettingsError = ValidateCaptureSettings(parameters); @@ -315,6 +323,7 @@ public PausePointResponse Clear(ClearPausePointSchema parameters) // Patcher directly. UloopPausePointClearAllResult clearAllResult = UloopPausePointRegistry.ClearAll(); LogCleared("all", string.Empty); + PhysicsFlaggedDeclaringTypesById.Clear(); return PausePointResponse.FromClearAll(clearAllResult); } @@ -329,7 +338,12 @@ public PausePointResponse Clear(ClearPausePointSchema parameters) if (snapshot.StatusBeforeClear == UloopPausePointStatus.Expired) { LogExpired(snapshot.Id, snapshot.ElapsedSinceEnabledMilliseconds); + if (PhysicsFlaggedDeclaringTypesById.TryGetValue(snapshot.Id, out Type declaringType)) + { + LogPhysicsDispatchDiagnostics("pause_point_expired_without_hit_physics", snapshot.Id, declaringType); + } } + PhysicsFlaggedDeclaringTypesById.Remove(snapshot.Id); PausePointResponse response = PausePointResponse.FromSnapshot(snapshot); if (resumedFromPause) @@ -401,6 +415,13 @@ private static PausePointResponse EnableBySourceLocation(EnablePausePointSchema response.SnapshotTiming = SourcePausePointConstants.PreLineSnapshotTimingNote; response.Warning = MergeWarnings(CreateEnableWarning(), patchResult.Warning); LogEnable(response.Id, response.ResolvedMethod, $"{parameters.File}:{response.ResolvedLine}", response.Mode, response.Warning); + + if (patchResult.HasPhysicsCallbackWarning) + { + PhysicsFlaggedDeclaringTypesById[id] = patchResult.DeclaringType; + LogPhysicsDispatchDiagnostics("pause_point_physics_dispatch_diagnostics", id, patchResult.DeclaringType); + } + return response; } @@ -412,6 +433,33 @@ private static void LogEnable(string id, string resolvedMethod, string fileLine, new { Id = id, ResolvedMethod = resolvedMethod, FileLine = fileLine, Mode = mode, HasWarning = !string.IsNullOrEmpty(warning) }); } + // Captures the state needed to diagnose a physics-callback dispatch miss if one recurs: + // whether Play Mode is running, how long the current domain has been alive without a + // reload (a suspected factor -- see docs/regression-harness.md), the declaring type, and + // (for MonoBehaviour-derived types only) how many instances currently exist in the loaded + // scenes. + private static void LogPhysicsDispatchDiagnostics(string operation, string id, Type declaringType) + { + bool isMonoBehaviourDerived = typeof(MonoBehaviour).IsAssignableFrom(declaringType); + int monoBehaviourInstanceCount = isMonoBehaviourDerived + ? UnityEngine.Object.FindObjectsByType(declaringType, FindObjectsInactive.Include, FindObjectsSortMode.None).Length + : 0; + int instanceCount = PausePointPhysicsDispatchDiagnostics.ResolveInstanceCount(isMonoBehaviourDerived, monoBehaviourInstanceCount); + + VibeLogger.LogInfo( + operation, + $"Physics-callback pause point dispatch diagnostics: {id}", + new + { + Id = id, + IsPlaying = EditorApplication.isPlaying, + IsPaused = EditorApplication.isPaused, + SecondsSinceLastDomainReload = PausePointDomainReloadTracker.SecondsSinceLoad(), + DeclaringType = declaringType.FullName, + InstanceCount = instanceCount + }); + } + // The resolved line can be rounded forward from the requested line (the Resolver picks // the closest sequence point on or after it), so returning the actual source text lets // the caller notice a mismatch immediately instead of assuming the requested line hit. diff --git a/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointConstants.cs b/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointConstants.cs index 4c1a3e8a69..43c54a0f6d 100644 --- a/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointConstants.cs +++ b/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointConstants.cs @@ -59,20 +59,22 @@ 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. + // Unity's physics message dispatch (OnCollision*/OnTrigger*/OnParticleCollision) has been + // observed in real projects to bypass a Harmony patch applied while the GameObject already + // existed, so the pause point can silently miss even though the method body runs. The + // trigger condition is environment-dependent and has not been reproduced deterministically + // (fresh sessions, fresh Editor processes, primed JIT, runtime-created instances, and + // one-hop indirect callees all patched correctly in controlled experiments; see + // docs/regression-harness.md). This is informational only. 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. Lightest workaround: after this pause point is " + "enabled (the patch must already be applied), toggle any one instance's enabled off and " - + "on once. This re-resolves Unity's message dispatch for the whole component type for the " - + "rest of this Editor session (until the next domain reload), fixing every instance, not " - + "just the toggled one. Note the toggle fires OnDisable/OnEnable, so watch for side effects " + + "on once. This re-resolves Unity's message dispatch for the whole component type, fixing " + + "every instance, not just the toggled one; once re-resolved, the miss has not been " + + "observed to recur. Note the toggle fires OnDisable/OnEnable, so watch for side effects " + "on components with game logic there. Heavier 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."; @@ -88,8 +90,8 @@ internal static class SourcePausePointConstants + "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. Lightest workaround: after this pause point " + "is enabled (the patch must already be applied), toggle any one instance's enabled off and on once. " - + "This re-resolves Unity's message dispatch for the whole component type for the rest of this Editor " - + "session (until the next domain reload), fixing every instance, not just the toggled one. Note the " + + "This re-resolves Unity's message dispatch for the whole component type, fixing every instance, not " + + "just the toggled one; once re-resolved, the miss has not been observed to recur. Note the " + "toggle fires OnDisable/OnEnable, so watch for side effects on components with game logic there. " + "Heavier 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 " diff --git a/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointPatchResult.cs b/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointPatchResult.cs index d1472faac6..4e9f76d6f4 100644 --- a/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointPatchResult.cs +++ b/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointPatchResult.cs @@ -1,3 +1,5 @@ +using System; + namespace io.github.hatayama.UnityCliLoop.FirstPartyTools { /// @@ -10,25 +12,34 @@ internal sealed class SourcePausePointPatchResult public string ErrorMessage { get; } public string Hint { get; } public string Warning { get; } + public Type DeclaringType { get; } + public bool HasPhysicsCallbackWarning { get; } private SourcePausePointPatchResult( - bool success, SourcePausePointPatchFailureReason failureReason, string errorMessage, string hint, string warning) + bool success, SourcePausePointPatchFailureReason failureReason, string errorMessage, string hint, + string warning, Type declaringType, bool hasPhysicsCallbackWarning) { Success = success; FailureReason = failureReason; ErrorMessage = errorMessage; Hint = hint; Warning = warning; + DeclaringType = declaringType; + HasPhysicsCallbackWarning = hasPhysicsCallbackWarning; } - public static SourcePausePointPatchResult SuccessResult(string warning = "") + public static SourcePausePointPatchResult SuccessResult( + string warning = "", Type declaringType = null, bool hasPhysicsCallbackWarning = false) { - return new SourcePausePointPatchResult(true, SourcePausePointPatchFailureReason.None, string.Empty, string.Empty, warning); + return new SourcePausePointPatchResult( + true, SourcePausePointPatchFailureReason.None, string.Empty, string.Empty, + warning, declaringType, hasPhysicsCallbackWarning); } public static SourcePausePointPatchResult Failure(SourcePausePointPatchFailureReason reason, string errorMessage, string hint) { - return new SourcePausePointPatchResult(false, reason, errorMessage, hint, string.Empty); + return new SourcePausePointPatchResult( + false, reason, errorMessage, hint, string.Empty, declaringType: null, hasPhysicsCallbackWarning: false); } } } diff --git a/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointPatcher.cs b/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointPatcher.cs index 8d3e158814..5c61174f45 100644 --- a/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointPatcher.cs +++ b/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointPatcher.cs @@ -118,8 +118,8 @@ public static SourcePausePointPatchResult Patch(string id, SourcePausePointResol } } - string warning = BuildPatchWarning(method); - return SourcePausePointPatchResult.SuccessResult(warning); + (string warning, bool hasPhysicsCallbackWarning) = BuildPatchWarning(method); + return SourcePausePointPatchResult.SuccessResult(warning, method.DeclaringType, hasPhysicsCallbackWarning); } public static void Unpatch(string id) @@ -251,9 +251,10 @@ private static bool HasBurstCompileAttribute(MemberInfo member) return false; } - private static string BuildPatchWarning(MethodBase method) + private static (string Warning, bool HasPhysicsCallbackWarning) BuildPatchWarning(MethodBase method) { List warnings = new(); + bool hasPhysicsCallbackWarning = false; if (!method.IsStatic && IsByRefLikeType(method.DeclaringType)) { @@ -264,10 +265,12 @@ private static string BuildPatchWarning(MethodBase method) typeof(MonoBehaviour).IsAssignableFrom(method.DeclaringType)) { warnings.Add(SourcePausePointConstants.PhysicalCallbackMayMissExistingInstanceWarning); + hasPhysicsCallbackWarning = true; } else if (SourcePausePointPhysicalCallbackCallSiteScanner.IsCalledFromPhysicalMessageMethod(method)) { warnings.Add(SourcePausePointConstants.PhysicalCallbackIndirectCallMayMissExistingInstanceWarning); + hasPhysicsCallbackWarning = true; } if (IsLikelyJitInlined(method)) @@ -275,7 +278,7 @@ private static string BuildPatchWarning(MethodBase method) warnings.Add(SourcePausePointConstants.SmallMethodInliningRiskWarning); } - return string.Join(" ", warnings); + return (string.Join(" ", warnings), hasPhysicsCallbackWarning); } private static bool IsByRefLikeType(Type type) diff --git a/Packages/src/Editor/Infrastructure/Api/PausePointStatusBridgeCommand.cs b/Packages/src/Editor/Infrastructure/Api/PausePointStatusBridgeCommand.cs index f3a5dd7942..c90470f74a 100644 --- a/Packages/src/Editor/Infrastructure/Api/PausePointStatusBridgeCommand.cs +++ b/Packages/src/Editor/Infrastructure/Api/PausePointStatusBridgeCommand.cs @@ -65,7 +65,10 @@ private static void LogCleared(string target, string statusBeforeClear) // Why: PausePointTools.LogExpired duplicates this instead of sharing it, since this // bridge must not reference that Editor-only tool assembly. Keep both in sync if the - // log shape or wording changes. + // log shape or wording changes. The physics-callback dispatch diagnostics + // (pause_point_physics_dispatch_diagnostics / pause_point_expired_without_hit_physics) + // are NOT duplicated here -- they are tool-side only, since this bridge has no access to + // the declaring-type/patch state PausePointUseCase tracks for that purpose. private static void LogExpired(string id, long elapsedSinceEnabledMilliseconds) { VibeLogger.LogInfo( From 5e83593d17b3d967394dfe2b9a70ab0291c22056 Mon Sep 17 00:00:00 2001 From: hatayama Date: Tue, 21 Jul 2026 23:48:03 +0900 Subject: [PATCH 5/7] Retract the enabled-toggle workaround for the physics-callback existing-instance miss The harness's baseline-miss check never triggered a fresh contact after arming a pause point, so every "miss" it reported was a false positive: IsHit=false at baseline only meant no new collision occurred in the check window (the ball had already settled from a fall before arming), not that dispatch was missed. The apparent "toggle fixes it" result was simply the first-ever fresh collision since arming. - Revert the Warning text to only recommend destroy-and-recreate or a manual UloopPausePoint.Pause marker; document why the toggle workaround was rejected. - Rewrite the harness to require a component hit-counter increment alongside IsHit=false before calling something a genuine miss, and to distinguish that from a harness self-failure (no fresh contact produced at all). An enabled-toggle probe still runs informationally on a genuine miss but is no longer asserted. - Update docs/regression-harness.md to match. --- .../PausePoint/SourcePausePointConstants.cs | 25 ++-- docs/regression-harness.md | 29 ++-- ...ness-physics-callback-existing-instance.sh | 134 ++++++++++++------ 3 files changed, 122 insertions(+), 66 deletions(-) diff --git a/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointConstants.cs b/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointConstants.cs index 43c54a0f6d..a53ef02e6d 100644 --- a/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointConstants.cs +++ b/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointConstants.cs @@ -65,19 +65,18 @@ internal static class SourcePausePointConstants // trigger condition is environment-dependent and has not been reproduced deterministically // (fresh sessions, fresh Editor processes, primed JIT, runtime-created instances, and // one-hop indirect callees all patched correctly in controlled experiments; see - // docs/regression-harness.md). This is informational only. + // docs/regression-harness.md). A lighter enabled-toggle workaround was investigated and + // rejected: every local "miss" that seemed to support it was a false positive where no new + // callback ran during the check window, so only the mechanism-sound workarounds (recreate + // the GameObject, or a manual marker) are recommended. This is informational only. 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. Lightest workaround: after this pause point is " - + "enabled (the patch must already be applied), toggle any one instance's enabled off and " - + "on once. This re-resolves Unity's message dispatch for the whole component type, fixing " - + "every instance, not just the toggled one; once re-resolved, the miss has not been " - + "observed to recur. Note the toggle fires OnDisable/OnEnable, so watch for side effects " - + "on components with game logic there. Heavier 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."; + + "hit even though the method body runs. If that happens, work around it by destroying and " + + "recreating 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."; // The same cached-dispatch risk as PhysicalCallbackMayMissExistingInstanceWarning, but for a // method that is not itself named after a physics message method and is instead called (one @@ -88,12 +87,8 @@ internal static class SourcePausePointConstants "This method is called from a Unity physics message method (OnCollision*/OnTrigger*/OnParticleCollision) " + "elsewhere in the same compiled assembly. 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. Lightest workaround: after this pause point " - + "is enabled (the patch must already be applied), toggle any one instance's enabled off and on once. " - + "This re-resolves Unity's message dispatch for the whole component type, fixing every instance, not " - + "just the toggled one; once re-resolved, the miss has not been observed to recur. Note the " - + "toggle fires OnDisable/OnEnable, so watch for side effects on components with game logic there. " - + "Heavier workarounds: destroy and recreate the GameObject after enabling this pause point, or embed " + + "point may never hit even though the method body runs. If that happens, work around it by destroying " + + "and recreating 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."; diff --git a/docs/regression-harness.md b/docs/regression-harness.md index a0790b0b1f..93791f9c42 100644 --- a/docs/regression-harness.md +++ b/docs/regression-harness.md @@ -31,21 +31,30 @@ scenario covers. |------|-------|--------| | Key state divergence after a pause-point interruption | `Assets/RegressionHarness/KeyStateAfterPauseInterruption/` | `scripts/regression-harness-key-state-after-pause-interruption.sh` | | `await-pause-point --trigger` hits within the marker timeout instead of waiting out the triggered command's full duration | `Assets/RegressionHarness/KeyStateAfterPauseInterruption/` (reused) | `scripts/regression-harness-pause-point-trigger.sh` | -| A pause point armed on a physics message method (or a method called one hop from one) misses a GameObject that already existed before arming; toggling the component's `enabled` off/on resolves it. The miss itself is environment-dependent and does not reproduce deterministically (see below) -- the harness runs three scenarios (direct/OnCollisionEnter2D, indirect callee with priming, and OnTriggerEnter2D) and asserts the workaround only when a miss is actually observed | `Assets/RegressionHarness/PhysicsCallbackExistingInstance/` | `scripts/regression-harness-physics-callback-existing-instance.sh` | +| A pause point armed on a physics message method (or a method called one hop from one) can miss a GameObject that already existed before arming. The miss itself is environment-dependent and does not reproduce deterministically (see below) -- the harness runs three scenarios (direct/OnCollisionEnter2D, indirect callee with priming, and OnTriggerEnter2D), each triggering a fresh contact after arming and classifying the result from the component's own hit counter plus `IsHit` | `Assets/RegressionHarness/PhysicsCallbackExistingInstance/` | `scripts/regression-harness-physics-callback-existing-instance.sh` | ### Physics-callback existing-instance miss: environment-dependent, not deterministic +A "miss" is only valid evidence when a fresh contact is actually triggered after arming and the +component's own hit counter proves the method body ran; `IsHit=false` on its own only means no new +collision occurred in the check window, not that dispatch was missed. An earlier version of this +harness checked baseline `IsHit` without ever triggering a fresh contact after arming, which made +every "miss" it reported a false positive (the ball had already settled from a fall that happened +before arming) -- including the "enabled-toggle workaround fixes it" conclusion that briefly lived +in this doc and in `SourcePausePointConstants`. That conclusion has been retracted; the harness now +requires a counter increment alongside `IsHit=false` before it will call something a genuine miss. + The following were ruled out as the deterministic trigger condition through controlled experiments (each patched correctly, i.e. did not reproduce the miss): a fresh domain reload immediately before arming, priming the JIT before arming, arming right after a script compile, arming in a freshly launched Editor process, arming in a freshly launched process right after a compile, arming on a runtime-`AddComponent`-created instance, and arming a one-hop indirect -callee with the instance primed by one prior contact. The miss is real (it has been observed and -the enabled-toggle workaround has been confirmed to fix it, including a controlled two-instance -experiment proving the fix applies to the whole component type, not just the toggled instance), -but no deterministic reproduction recipe is known. `PausePointTools` logs a -`pause_point_physics_dispatch_diagnostics` VibeLogger entry whenever a physics-flagged pause -point is enabled (and a `pause_point_expired_without_hit_physics` entry if it later expires -without ever hitting), capturing Play Mode state, seconds since the last domain reload, the -declaring type, and its current instance count -- if the miss recurs, this is the primary -evidence to work from. +callee with the instance primed by one prior contact. The miss is real (it has been observed in +real projects), but no deterministic reproduction recipe is known, and no lighter workaround than +destroying and recreating the GameObject (or a manual `UloopPausePoint.Pause` marker) has been +validated -- see `SourcePausePointConstants.PhysicalCallbackMayMissExistingInstanceWarning`. +`PausePointTools` logs a `pause_point_physics_dispatch_diagnostics` VibeLogger entry whenever a +physics-flagged pause point is enabled (and a `pause_point_expired_without_hit_physics` entry if +it later expires without ever hitting), capturing Play Mode state, seconds since the last domain +reload, the declaring type, and its current instance count -- if the miss recurs, this is the +primary evidence to work from. diff --git a/scripts/regression-harness-physics-callback-existing-instance.sh b/scripts/regression-harness-physics-callback-existing-instance.sh index 44326212ac..a3f7087f7f 100755 --- a/scripts/regression-harness-physics-callback-existing-instance.sh +++ b/scripts/regression-harness-physics-callback-existing-instance.sh @@ -5,11 +5,21 @@ set -e # bypass a Harmony patch applied while the GameObject already existed, so the pause point can # silently miss even though the method body runs. The trigger condition is environment-dependent # and has NOT been reproduced deterministically here (see docs/regression-harness.md for the list -# of ruled-out hypotheses) -- this harness therefore treats the baseline-miss check as conditional: -# - if the pre-existing instance misses the pause point (the miss reproduces in this run), the -# enabled-toggle workaround is strictly asserted to fix it -# - if it does not miss (the common case in this environment), that is logged and this run passes -# without exercising the workaround assertion +# of ruled-out hypotheses). +# +# A "miss" is only valid evidence if a fresh contact is actually triggered after arming and the +# component's own hit counter proves the method body ran. Each scenario therefore: arms the pause +# point, reads the counter, triggers a fresh contact via reset_ball, reads the counter again, and +# reads IsHit. Three outcomes: +# - IsHit=true -> PASS, did not reproduce in this environment +# - counter incremented AND IsHit=false -> genuine miss (the method body ran but the +# pause point never hit); this is a rare capture event worth surfacing loudly, so the harness +# logs diagnostics and exits 1. An enabled-toggle probe then runs informationally (logged, not +# asserted) since it is not a validated fix -- see +# SourcePausePointConstants.PhysicalCallbackMayMissExistingInstanceWarning. +# - counter did NOT increment AND IsHit=false -> harness self-failure: reset_ball did not +# produce a fresh contact, so this run never exercised the scenario at all +# # It covers three call shapes: # 1. direct -- pause point on the physics message method itself (OnCollisionEnter2D) # 2. indirect -- pause point on a method called one hop deep from the physics message method, @@ -18,14 +28,13 @@ set -e # 3. trigger -- pause point on OnTriggerEnter2D, a separate Unity dispatch path from OnCollision* # # The harness forces a fresh domain reload via EditorUtility.RequestScriptReload() before arming, -# since the enabled-toggle workaround (when it applies) re-resolves dispatch for the whole -# component type -- without a reload, a type already "fixed" by an earlier local run of this -# harness would carry that fix into this run. RequestScriptReload queues the reload for a later -# editor update rather than running it inline, so polling for bare IPC responsiveness is not -# sufficient -- a still-loading-the-reload domain and the still-alive pre-reload domain both answer -# "list" successfully. Reload completion is instead detected via HarnessDomainMarker.Id, a static -# readonly value that is re-initialized only when the AppDomain actually reloads; the harness waits -# until this value changes. +# so a leftover "resolved by a previous run's informational toggle probe" state does not carry into +# this run's counter/IsHit checks. RequestScriptReload queues the reload for a later editor update +# rather than running it inline, so polling for bare IPC responsiveness is not sufficient -- a +# still-loading-the-reload domain and the still-alive pre-reload domain both answer "list" +# successfully. Reload completion is instead detected via HarnessDomainMarker.Id, a static readonly +# value that is re-initialized only when the AppDomain actually reloads; the harness waits until +# this value changes. # See docs/regression-harness.md and # SourcePausePointConstants.PhysicalCallbackMayMissExistingInstanceWarning. # @@ -80,8 +89,26 @@ EOF run_uloop execute-dynamic-code --code-file "$DYNAMIC_CODE_FILE" 2>/dev/null | jq -r '.Result // empty' } -# Forces a fresh domain reload without touching any file, so the component type carries no -# leftover "fixed by a previous toggle" state into this run's baseline-miss check. Unity +# Reads one of the harness component's own hit counters (not the pause-point's own hit state) -- +# this is the proof that a physics callback's method body actually ran, independent of whether the +# pause point captured it. +read_counter() { + OBJECT_NAME="$1" + COMPONENT_TYPE="$2" + FIELD_NAME="$3" + cat > "$DYNAMIC_CODE_FILE" <(); +return component.$FIELD_NAME.ToString(); +EOF + run_uloop execute-dynamic-code --code-file "$DYNAMIC_CODE_FILE" 2>/dev/null | jq -r '.Result // empty' +} + +# Forces a fresh domain reload without touching any file, so this run's counter/IsHit checks are +# not contaminated by a "resolved by a previous run's informational toggle probe" state. Unity # disconnects the IPC bridge for the duration of the reload -- that failure is expected and # swallowed here. RequestScriptReload queues the reload for a later editor update rather than # running it inline, so bare IPC responsiveness is not proof the reload happened -- completion is @@ -131,6 +158,11 @@ EOF run_uloop execute-dynamic-code --code-file "$DYNAMIC_CODE_FILE" > /dev/null } +# Informational-only probe: this workaround was investigated and found to not be validated (every +# local "it fixed the miss" observation turned out to be a harness false positive, see +# SourcePausePointConstants.PhysicalCallbackMayMissExistingInstanceWarning). It is still run and +# logged on a genuine miss so a real recurrence carries this data point, but its result is never +# asserted. toggle_enabled() { OBJECT_NAME="$1" COMPONENT_TYPE="$2" @@ -147,48 +179,58 @@ EOF run_uloop execute-dynamic-code --code-file "$DYNAMIC_CODE_FILE" > /dev/null } -# Waits for the ball to make contact and settle, then arms a pause point and reports whether the -# pre-existing instance missed it. If it missed, strictly asserts the enabled-toggle workaround -# fixes it. If it did not miss, logs that this run did not reproduce the trap (expected in an -# environment where the trigger condition does not apply) and returns without asserting further. -verify_conditional_gap_and_workaround() { +# Arms a pause point, triggers a fresh contact (reset_ball) after arming, and classifies the +# result from the component's own hit counter plus IsHit. See the file header for the three +# possible outcomes; only "harness self-failure" and "genuine miss" exit non-zero. +verify_gap() { LABEL="$1" MARKER_FILE="$2" MARKER_LINE="$3" OBJECT_NAME="$4" COMPONENT_TYPE="$5" + COUNTER_FIELD="$6" MARKER_ID="${MARKER_FILE}:${MARKER_LINE}" log "[$LABEL] Arming pause-point on the pre-existing instance..." run_uloop enable-pause-point --file "$MARKER_FILE" --line "$MARKER_LINE" --timeout-seconds 30 > /dev/null + COUNTER_BEFORE="$(read_counter "$OBJECT_NAME" "$COMPONENT_TYPE" "$COUNTER_FIELD")" + if [ -z "$COUNTER_BEFORE" ]; then + log "[$LABEL] FAIL: could not read $COMPONENT_TYPE.$COUNTER_FIELD on $OBJECT_NAME before triggering a fresh contact." + exit 1 + fi + + log "[$LABEL] Triggering a fresh contact (reset_ball) now that the pause point is armed..." + reset_ball sleep 3 + + COUNTER_AFTER="$(read_counter "$OBJECT_NAME" "$COMPONENT_TYPE" "$COUNTER_FIELD")" run_uloop pause-point-status --id "$MARKER_ID" > "$RESULT_FILE" - BASELINE_IS_HIT="$(jq -r '.IsHit' "$RESULT_FILE")" + IS_HIT="$(jq -r '.IsHit' "$RESULT_FILE")" - if [ "$BASELINE_IS_HIT" = "true" ]; then - log "[$LABEL] PASS (trap did not reproduce in this environment; environment-dependent, see PR #1922): the pre-existing instance was hit on arming without needing the enabled-toggle workaround." + if [ "$IS_HIT" = "true" ]; then + log "[$LABEL] PASS (did not reproduce in this environment; environment-dependent, see PR #1922): the fresh contact was captured by the pause point ($COUNTER_FIELD $COUNTER_BEFORE -> $COUNTER_AFTER)." run_uloop clear-pause-point --all > /dev/null return 0 fi - log "[$LABEL] Baseline miss reproduced (IsHit=false) -- asserting the enabled-toggle workaround strictly." - log "[$LABEL] Toggling enabled off/on and re-triggering contact..." - toggle_enabled "$OBJECT_NAME" "$COMPONENT_TYPE" - reset_ball - - sleep 3 - run_uloop pause-point-status --id "$MARKER_ID" > "$RESULT_FILE" - WORKAROUND_IS_HIT="$(jq -r '.IsHit' "$RESULT_FILE")" - - if [ "$WORKAROUND_IS_HIT" != "true" ]; then - log "[$LABEL] FAIL: reproduced the baseline miss but the enabled-toggle workaround did not make the pause point hit (IsHit=$WORKAROUND_IS_HIT)" + if [ -n "$COUNTER_AFTER" ] && [ "$COUNTER_AFTER" -gt "$COUNTER_BEFORE" ] 2>/dev/null; then + log "[$LABEL] FAIL: genuine miss captured -- the method body ran ($COUNTER_FIELD $COUNTER_BEFORE -> $COUNTER_AFTER) but the pause point never hit. Investigate; see PR #1922." cat "$RESULT_FILE" + log "[$LABEL] Running the enabled-toggle probe informationally (not asserted, not a validated fix)..." + toggle_enabled "$OBJECT_NAME" "$COMPONENT_TYPE" + reset_ball + sleep 3 + run_uloop pause-point-status --id "$MARKER_ID" > "$RESULT_FILE" + PROBE_IS_HIT="$(jq -r '.IsHit' "$RESULT_FILE")" + log "[$LABEL] Toggle probe result (informational only): IsHit=$PROBE_IS_HIT" + run_uloop clear-pause-point --all > /dev/null exit 1 fi - log "[$LABEL] PASS: reproduced the baseline miss and the enabled-toggle workaround resolved it." + log "[$LABEL] FAIL: harness self-failure -- reset_ball did not produce a fresh contact ($COUNTER_FIELD stayed at $COUNTER_BEFORE); this scenario never actually exercised the pause point." run_uloop clear-pause-point --all > /dev/null + exit 1 } log "Stopping any existing Play Mode session..." @@ -202,13 +244,23 @@ force_clean_domain log "Starting Play Mode (Floor/TriggerZone/Ball already exist before any pause point is armed)..." run_uloop control-play-mode --action Play > /dev/null -log "[indirect] Priming Floor with one contact before the indirect callee is armed..." +log "Waiting for the ball's initial fall to land on Floor (this priming contact seeds both HitCount and IndirectHitCount before any pause point is armed)..." sleep 3 +INITIAL_HIT_COUNT="$(read_counter "Floor" "PhysicsCallbackFloor" "HitCount")" +INITIAL_INDIRECT_COUNT="$(read_counter "Floor" "PhysicsCallbackFloor" "IndirectHitCount")" +if [ -z "$INITIAL_HIT_COUNT" ] || [ "$INITIAL_HIT_COUNT" -lt 1 ] 2>/dev/null; then + log "FAIL: the ball did not make initial contact with Floor before arming (HitCount=$INITIAL_HIT_COUNT)." + exit 1 +fi +if [ -z "$INITIAL_INDIRECT_COUNT" ] || [ "$INITIAL_INDIRECT_COUNT" -lt 1 ] 2>/dev/null; then + log "FAIL: the indirect callee was not primed before arming (IndirectHitCount=$INITIAL_INDIRECT_COUNT)." + exit 1 +fi +log "Priming confirmed (HitCount=$INITIAL_HIT_COUNT, IndirectHitCount=$INITIAL_INDIRECT_COUNT)." -verify_conditional_gap_and_workaround "direct" "$FLOOR_FILE" "$FLOOR_DIRECT_LINE" "Floor" "PhysicsCallbackFloor" -verify_conditional_gap_and_workaround "indirect" "$FLOOR_FILE" "$FLOOR_INDIRECT_LINE" "Floor" "PhysicsCallbackFloor" -reset_ball -verify_conditional_gap_and_workaround "trigger" "$TRIGGER_FILE" "$TRIGGER_LINE" "TriggerZone" "PhysicsCallbackTriggerZone" +verify_gap "direct" "$FLOOR_FILE" "$FLOOR_DIRECT_LINE" "Floor" "PhysicsCallbackFloor" "HitCount" +verify_gap "indirect" "$FLOOR_FILE" "$FLOOR_INDIRECT_LINE" "Floor" "PhysicsCallbackFloor" "IndirectHitCount" +verify_gap "trigger" "$TRIGGER_FILE" "$TRIGGER_LINE" "TriggerZone" "PhysicsCallbackTriggerZone" "HitCount" -log "PASS: direct, indirect, and trigger physics-callback scenarios all handled (reproduced-and-fixed, or did-not-reproduce)." +log "PASS: direct, indirect, and trigger physics-callback scenarios all did not reproduce a genuine miss." exit 0 From b55601b9c6787bc9c7e730afd88586bfac7b1d74 Mon Sep 17 00:00:00 2001 From: hatayama Date: Tue, 21 Jul 2026 23:54:26 +0900 Subject: [PATCH 6/7] Stop the physics-callback expired diagnostic from firing after a hit, and cover the clear --all path pause_point_expired_without_hit_physics was logged for any Expired marker, including one that hit before expiring, which pollutes the primary evidence a future recurrence would rely on. It also never fired via clear-pause-point --all, the dominant field path (an agent cleaning up after an await timeout), so the common case lost this diagnostic entirely. - Gate both the single-id Clear path and a new ClearAll path on HitCount == 0 in addition to Status == Expired. - Snapshot each physics-flagged tracked id via GetStatus before ClearAll resolves it away, so the ClearAll path also gets the diagnostic. - Inline PausePointPhysicsDispatchDiagnostics.ResolveInstanceCount into its only caller and delete its dedicated test; the ternary it wrapped needed no separate coverage. --- ...usePointPhysicsDispatchDiagnosticsTests.cs | 35 ------------------- ...intPhysicsDispatchDiagnosticsTests.cs.meta | 11 ------ .../PausePointPhysicsDispatchDiagnostics.cs | 17 --------- ...usePointPhysicsDispatchDiagnostics.cs.meta | 11 ------ .../PausePoint/PausePointTools.cs | 23 +++++++++--- 5 files changed, 19 insertions(+), 78 deletions(-) delete mode 100644 Assets/Tests/Editor/SourcePausePointPatcher/PausePointPhysicsDispatchDiagnosticsTests.cs delete mode 100644 Assets/Tests/Editor/SourcePausePointPatcher/PausePointPhysicsDispatchDiagnosticsTests.cs.meta delete mode 100644 Packages/src/Editor/FirstPartyTools/PausePoint/PausePointPhysicsDispatchDiagnostics.cs delete mode 100644 Packages/src/Editor/FirstPartyTools/PausePoint/PausePointPhysicsDispatchDiagnostics.cs.meta diff --git a/Assets/Tests/Editor/SourcePausePointPatcher/PausePointPhysicsDispatchDiagnosticsTests.cs b/Assets/Tests/Editor/SourcePausePointPatcher/PausePointPhysicsDispatchDiagnosticsTests.cs deleted file mode 100644 index 27fc6deabb..0000000000 --- a/Assets/Tests/Editor/SourcePausePointPatcher/PausePointPhysicsDispatchDiagnosticsTests.cs +++ /dev/null @@ -1,35 +0,0 @@ -using NUnit.Framework; - -using io.github.hatayama.UnityCliLoop.FirstPartyTools; - -namespace io.github.hatayama.UnityCliLoop.Tests.Editor -{ - /// - /// Verifies the pure instance-count resolution rule used by physics-callback dispatch - /// diagnostics: counting only applies when the declaring type is a MonoBehaviour. - /// - [TestFixture] - public sealed class PausePointPhysicsDispatchDiagnosticsTests - { - // What: a MonoBehaviour-derived declaring type reports the actual scene instance count. - [Test] - public void ResolveInstanceCount_WithMonoBehaviourDerivedType_ReturnsActualCount() - { - int result = PausePointPhysicsDispatchDiagnostics.ResolveInstanceCount( - isMonoBehaviourDerived: true, monoBehaviourInstanceCount: 3); - - Assert.AreEqual(3, result); - } - - // What: a non-MonoBehaviour declaring type reports -1, since instance counting has no - // meaning for it (the diagnostics exist for MonoBehaviour physics message dispatch only). - [Test] - public void ResolveInstanceCount_WithNonMonoBehaviourType_ReturnsNegativeOne() - { - int result = PausePointPhysicsDispatchDiagnostics.ResolveInstanceCount( - isMonoBehaviourDerived: false, monoBehaviourInstanceCount: 0); - - Assert.AreEqual(-1, result); - } - } -} diff --git a/Assets/Tests/Editor/SourcePausePointPatcher/PausePointPhysicsDispatchDiagnosticsTests.cs.meta b/Assets/Tests/Editor/SourcePausePointPatcher/PausePointPhysicsDispatchDiagnosticsTests.cs.meta deleted file mode 100644 index 5f72822fd1..0000000000 --- a/Assets/Tests/Editor/SourcePausePointPatcher/PausePointPhysicsDispatchDiagnosticsTests.cs.meta +++ /dev/null @@ -1,11 +0,0 @@ -fileFormatVersion: 2 -guid: 9c842ed1c5896499482af8bac4e9fa19 -MonoImporter: - externalObjects: {} - serializedVersion: 2 - defaultReferences: [] - executionOrder: 0 - icon: {instanceID: 0} - userData: - assetBundleName: - assetBundleVariant: diff --git a/Packages/src/Editor/FirstPartyTools/PausePoint/PausePointPhysicsDispatchDiagnostics.cs b/Packages/src/Editor/FirstPartyTools/PausePoint/PausePointPhysicsDispatchDiagnostics.cs deleted file mode 100644 index 7ab3ec75ca..0000000000 --- a/Packages/src/Editor/FirstPartyTools/PausePoint/PausePointPhysicsDispatchDiagnostics.cs +++ /dev/null @@ -1,17 +0,0 @@ -namespace io.github.hatayama.UnityCliLoop.FirstPartyTools -{ - /// - /// Pure part of the physics-callback dispatch diagnostics snapshot: whether an instance count - /// is meaningful for the declaring type at all. - /// - internal static class PausePointPhysicsDispatchDiagnostics - { - // -1 signals "not applicable": counting instances only means something when the declaring - // type is a MonoBehaviour (the physics dispatch miss this diagnostic exists for is scoped - // to MonoBehaviour physics message methods). - public static int ResolveInstanceCount(bool isMonoBehaviourDerived, int monoBehaviourInstanceCount) - { - return isMonoBehaviourDerived ? monoBehaviourInstanceCount : -1; - } - } -} diff --git a/Packages/src/Editor/FirstPartyTools/PausePoint/PausePointPhysicsDispatchDiagnostics.cs.meta b/Packages/src/Editor/FirstPartyTools/PausePoint/PausePointPhysicsDispatchDiagnostics.cs.meta deleted file mode 100644 index 0045255cf6..0000000000 --- a/Packages/src/Editor/FirstPartyTools/PausePoint/PausePointPhysicsDispatchDiagnostics.cs.meta +++ /dev/null @@ -1,11 +0,0 @@ -fileFormatVersion: 2 -guid: 4f795930c12b141529a9321c1fe203cd -MonoImporter: - externalObjects: {} - serializedVersion: 2 - defaultReferences: [] - executionOrder: 0 - icon: {instanceID: 0} - userData: - assetBundleName: - assetBundleVariant: diff --git a/Packages/src/Editor/FirstPartyTools/PausePoint/PausePointTools.cs b/Packages/src/Editor/FirstPartyTools/PausePoint/PausePointTools.cs index 3d6ef88c75..9b09922045 100644 --- a/Packages/src/Editor/FirstPartyTools/PausePoint/PausePointTools.cs +++ b/Packages/src/Editor/FirstPartyTools/PausePoint/PausePointTools.cs @@ -318,6 +318,19 @@ public PausePointResponse Clear(ClearPausePointSchema parameters) { if (parameters.All) { + // Snapshot each physics-flagged marker before ClearAll resolves it away, so a + // marker that expired without a hit still gets its diagnostics logged. This is + // the dominant field path (await timeout -> agent cleans up with --all), so + // skipping it here would lose the primary evidence in the common case. + foreach (KeyValuePair tracked in PhysicsFlaggedDeclaringTypesById) + { + UloopPausePointSnapshot trackedSnapshot = UloopPausePointRegistry.GetStatus(tracked.Key); + if (trackedSnapshot.Status == UloopPausePointStatus.Expired && trackedSnapshot.HitCount == 0) + { + LogPhysicsDispatchDiagnostics("pause_point_expired_without_hit_physics", tracked.Key, tracked.Value); + } + } + // Registry.ClearAll unpatches any source pause points via the hook // SourcePausePointPatcher wires into it; this use case never references the // Patcher directly. @@ -338,7 +351,7 @@ public PausePointResponse Clear(ClearPausePointSchema parameters) if (snapshot.StatusBeforeClear == UloopPausePointStatus.Expired) { LogExpired(snapshot.Id, snapshot.ElapsedSinceEnabledMilliseconds); - if (PhysicsFlaggedDeclaringTypesById.TryGetValue(snapshot.Id, out Type declaringType)) + if (snapshot.HitCount == 0 && PhysicsFlaggedDeclaringTypesById.TryGetValue(snapshot.Id, out Type declaringType)) { LogPhysicsDispatchDiagnostics("pause_point_expired_without_hit_physics", snapshot.Id, declaringType); } @@ -441,10 +454,12 @@ private static void LogEnable(string id, string resolvedMethod, string fileLine, private static void LogPhysicsDispatchDiagnostics(string operation, string id, Type declaringType) { bool isMonoBehaviourDerived = typeof(MonoBehaviour).IsAssignableFrom(declaringType); - int monoBehaviourInstanceCount = isMonoBehaviourDerived + // -1 signals "not applicable": counting instances only means something when the + // declaring type is a MonoBehaviour (the physics dispatch miss this diagnostic exists + // for is scoped to MonoBehaviour physics message methods). + int instanceCount = isMonoBehaviourDerived ? UnityEngine.Object.FindObjectsByType(declaringType, FindObjectsInactive.Include, FindObjectsSortMode.None).Length - : 0; - int instanceCount = PausePointPhysicsDispatchDiagnostics.ResolveInstanceCount(isMonoBehaviourDerived, monoBehaviourInstanceCount); + : -1; VibeLogger.LogInfo( operation, From 848aa50f5432a138ec0908efedc35aefb9d91215 Mon Sep 17 00:00:00 2001 From: hatayama Date: Tue, 21 Jul 2026 23:57:00 +0900 Subject: [PATCH 7/7] Add a precondition assert to LogPhysicsDispatchDiagnostics's declaringType CodeRabbit flagged declaringType.FullName as an NRE risk if the parameter were ever null. It is only reachable via PhysicsFlaggedDeclaringTypesById, populated solely from a successful patch's method.DeclaringType, which a C#-sourced method always has -- so a null-conditional would silently mask a contract violation instead of surfacing it. Document that contract with a Debug.Assert precondition instead, per this repo's Fail Fast / contract-programming exception policy. --- .../src/Editor/FirstPartyTools/PausePoint/PausePointTools.cs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Packages/src/Editor/FirstPartyTools/PausePoint/PausePointTools.cs b/Packages/src/Editor/FirstPartyTools/PausePoint/PausePointTools.cs index 9b09922045..133c534051 100644 --- a/Packages/src/Editor/FirstPartyTools/PausePoint/PausePointTools.cs +++ b/Packages/src/Editor/FirstPartyTools/PausePoint/PausePointTools.cs @@ -453,6 +453,10 @@ private static void LogEnable(string id, string resolvedMethod, string fileLine, // scenes. private static void LogPhysicsDispatchDiagnostics(string operation, string id, Type declaringType) { + // Only reachable via PhysicsFlaggedDeclaringTypesById, which is populated solely from + // a successful patch's method.DeclaringType -- a C#-sourced method always has one. + Debug.Assert(declaringType != null, "declaringType must not be null"); + bool isMonoBehaviourDerived = typeof(MonoBehaviour).IsAssignableFrom(declaringType); // -1 signals "not applicable": counting instances only means something when the // declaring type is a MonoBehaviour (the physics dispatch miss this diagnostic exists