Repository navigation
fix: Physics pause points that miss a pre-existing GameObject now log diagnostics for future investigation - #1922
Merged
hatayama merged 7 commits intoJul 21, 2026
Conversation
…round 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
Contributor
|
Warning Review limit reached
Next review available in: 13 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds physics callback fixtures, structured warning metadata, dispatch diagnostics, domain-reload tracking, and a regression harness covering existing-instance collision and trigger pause points. ChangesPhysics callback diagnostics
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Script as Regression script
participant Unity as Unity Editor
participant Physics as Physics callback components
participant Tools as Pause-point tools
participant Status as Pause-point status
Script->>Unity: Request domain reload and start Play Mode
Script->>Physics: Prime and reset contacts
Script->>Tools: Arm pause point on existing instance
Physics->>Tools: Invoke callback and update counter
Script->>Status: Read IsHit and counter delta
Tools->>Tools: Log dispatch diagnostics on warning or expiry
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
…lling 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.
…d 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.
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)
…ng-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.
hatayama
changed the title
… 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.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Packages/src/Editor/FirstPartyTools/PausePoint/PausePointTools.cs`:
- Line 458: Update the diagnostic payload construction near DeclaringType to
access declaringType.FullName safely when declaringType is null, using the
null-conditional access so global methods produce a null fallback instead of
throwing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e815be1c-bb5b-46e6-87ff-5f15d7cbde64
⛔ Files ignored due to path filters (4)
Assets/RegressionHarness/PhysicsCallbackExistingInstance/HarnessDomainMarker.cs.metais excluded by none and included by noneAssets/Tests/Editor/SourcePausePointPatcher/PausePointPhysicsDispatchDiagnosticsTests.cs.metais excluded by none and included by nonePackages/src/Editor/FirstPartyTools/PausePoint/PausePointDomainReloadTracker.cs.metais excluded by none and included by nonePackages/src/Editor/FirstPartyTools/PausePoint/PausePointPhysicsDispatchDiagnostics.cs.metais excluded by none and included by none
📒 Files selected for processing (12)
Assets/RegressionHarness/PhysicsCallbackExistingInstance/HarnessDomainMarker.csAssets/RegressionHarness/PhysicsCallbackExistingInstance/PhysicsCallbackFloor.csAssets/Tests/Editor/SourcePausePointPatcher/PausePointPhysicsDispatchDiagnosticsTests.csPackages/src/Editor/FirstPartyTools/PausePoint/PausePointDomainReloadTracker.csPackages/src/Editor/FirstPartyTools/PausePoint/PausePointPhysicsDispatchDiagnostics.csPackages/src/Editor/FirstPartyTools/PausePoint/PausePointTools.csPackages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointConstants.csPackages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointPatchResult.csPackages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointPatcher.csPackages/src/Editor/Infrastructure/Api/PausePointStatusBridgeCommand.csdocs/regression-harness.mdscripts/regression-harness-physics-callback-existing-instance.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- Assets/RegressionHarness/PhysicsCallbackExistingInstance/PhysicsCallbackFloor.cs
…gType 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.
hatayama
dismissed
coderabbitai[bot]’s stale review
July 21, 2026 14:57
Addressed: unreachable-null contract documented with a precondition assert (848aa50); CodeRabbit re-review unavailable due to rate limiting.
hatayama
merged commit Jul 21, 2026
de474f4
into
feature/round7-pause-point-improvements
2 checks passed
This was referenced Jul 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
OnCollisionEnter2D/OnTriggerEnter2D/etc.) that miss a GameObject which already existed before the pause point was enabled now log diagnostics to help pin down a future recurrence.UloopPausePoint.Pausemarker) was found to be valid; an enabled-toggle workaround investigated during this work was rejected as a false positive (see Investigation summary).User Impact
UloopPausePoint.Pausemarker), and there were no diagnostics to help investigate a recurrence.Changes
PausePointToolslogs apause_point_physics_dispatch_diagnosticsVibeLogger entry whenever a physics-flagged pause point is enabled, and apause_point_expired_without_hit_physicsentry if it later expires without ever hitting.Assets/RegressionHarness/PhysicsCallbackExistingInstance/,scripts/regression-harness-physics-callback-existing-instance.sh) covering three call shapes: direct (OnCollisionEnter2D), indirect (a one-hop callee, primed with a prior contact), andOnTriggerEnter2D. Each scenario arms the pause point, triggers a fresh contact, and classifies the result from the component's own hit counter plusIsHit-- a miss is only counted as genuine when the counter incremented (proof the method body ran) whileIsHitstayed false; a counter that did not increment means the harness itself failed to produce a contact.Investigation summary
An enabled-toggle workaround (toggle a physics-callback component's
enabledoff/on once, claimed to re-resolve Unity's cached message dispatch for the whole component type) was investigated as a lighter alternative to destroy-and-recreate. Every local observation that seemed to support it was a false positive: the harness's baseline check never triggered a fresh contact after arming, soIsHit=falseat baseline only meant "no new collision occurred in the check window" (the ball had already settled from a fall that happened before arming), not that dispatch was missed. The apparent "hit after toggle" was simply the first-ever fresh collision since arming, unrelated to the toggle. The same flaw explained an earlier single miss observation recorded in this environment's VibeLogger output. The Warning text (SourcePausePointConstants.PhysicalCallbackMayMissExistingInstanceWarning/PhysicalCallbackIndirectCallMayMissExistingInstanceWarning) anddocs/regression-harness.mdhave been corrected to remove the toggle-workaround claim, and the harness now requires a counter increment alongsideIsHit=falsebefore calling something a genuine miss.Verification
uloop compile-- 0 errors, 0 warnings.uloop run-tests --filter-type regex --filter-value "SourcePausePointPatcherTests|PausePointPhysicsDispatchDiagnosticsTests" --test-mode EditMode-- 29/29 passed.sh scripts/regression-harness-physics-callback-existing-instance.sh-- all three scenarios (direct/indirect/trigger) passed as "did not reproduce" (each fresh contact was captured by the pause point; no genuine miss and no harness self-failure)..uloop/outputs/VibeLogs/thatpause_point_physics_dispatch_diagnosticsentries are emitted with the expected fields (Play Mode state, declaring type, instance count, seconds since domain reload).