Repository navigation
fix: Raycast grid annotation no longer misses rotated or split colliders - #1666
Conversation
--annotate-raycast-grid にはマスク無し時の5x5粗いグリッド予覧(RaycastGridPoints)と マスク指定時の40x40密クラスタ化(輪郭付きPhysicsCollider)という2つの排他モードがあり、 5x5サンプリングが回転/分割コライダーを取りこぼして「当たり判定が壊れている」と誤解される 事態が発生した。実際には同じ40x40密データをRaycastLayerSummaries用に既に取得していたため、 粗い可視化を返す理由がなかった。 隣ペインのFable 5にセカンドオピニオンを求め、常時クラスタ化に統一する方針で合意。 マスク無し時はPhysics.DefaultRaycastLayers(Ignore Raycastのみ除外)でクラスタ化し、 RaycastLayerSummariesはマスクの有無を問わず常に返す。レスポンスJSONには実際にクラスタ化 対象になったレイヤー名一覧(RaycastLayerNamesChecked = 使用マスク & Camera.main.cullingMask) を新規に追加した。 - RaycastGridPointInfo(5x5専用DTO)をToolContractsから削除し、RaycastGridAnnotator内部の RaycastLayerHitSample(Hit/HitLayerIndex/HitLayer/HitGameObjectPathの4フィールドのみ)に置換 - CollectRaycastGridPoints/CalculateGridInputPosition/CreateOverlayElementsの5x5専用デッドコードを削除 - RaycastLayerMaskResolver.CreateLayerNamesFromMaskを追加し、ビットマスクからレイヤー名一覧を生成
annotated-elements.mdからRaycastGridPointsセクションを削除し、 RaycastLayerSummariesが常時返る仕様とRaycastLayerNamesChecked 新フィールドの説明を追加。SimulateMouseInputのSKILL.mdと default-tools.json、READMEからも5x5グリッド専用の言及を除去し、 AnnotatedElements[].SimX/SimYへの案内に統一した。 .claude/.agentsのミラーはuloop skills installで再生成し、 canonicalファイルとの差分が無いことを確認済み。
|
Warning Review limit reached
Next review available in: 34 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)
📝 WalkthroughWalkthroughRefactors raycast grid annotation from ChangesRaycast Layer Summary Refactor
Estimated code review effort: 3 (Moderate) | ~25 minutes 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Packages/src/Editor/FirstPartyTools/Screenshot/RaycastAnnotation/RaycastGridAnnotator.cs (2)
125-142: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider renaming
CreatePointInfoto reflect the newRaycastLayerHitSamplereturn type.The method now creates
RaycastLayerHitSample, not the removedRaycastGridPointInfo. The nameCreatePointInfois stale and could mislead future readers. ConsiderCreateLayerHitSampleorCreateHitSample.♻️ Proposed rename
-private static RaycastLayerHitSample CreatePointInfo(GameViewRaycastResult raycastResult) +private static RaycastLayerHitSample CreateLayerHitSample(GameViewRaycastResult raycastResult)🤖 Prompt for 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. In `@Packages/src/Editor/FirstPartyTools/Screenshot/RaycastAnnotation/RaycastGridAnnotator.cs` around lines 125 - 142, The method name CreatePointInfo is stale now that it returns RaycastLayerHitSample, so rename it to match the actual purpose and return type. Update the identifier used in RaycastGridAnnotator to something like CreateLayerHitSample or CreateHitSample, and make sure any call sites or references are updated to the new name for consistency.
31-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
CollectRaycastGridPointsForGridduplicates the grid-iteration and raycast logic ofCollectClusterSamples.Both methods iterate the same 40×40 grid, call
CalculateGridInputPositionForGrid, and invokeRaycastFromInputPosition. They differ only in mask (hardcoded vs parameter) and how results are stored (all samples vs hits-only). Extracting a shared dense-raycast helper that returns rawGameViewRaycastResult[]would eliminate the duplication and ensure both passes stay in sync if grid dimensions or raycast parameters change.Additionally, the method name
CollectRaycastGridPointsForGridis redundant ("Grid…ForGrid") and references the removedRaycastGridPointInfoconcept. ConsiderCollectDenseRaycastSamplesorCollectLayerHitSamples.♻️ Proposed refactor: extract shared grid-raycast helper
private static List<RaycastLayerHitSample> CollectRaycastGridPointsForGrid( Vector2 renderingImageSize, int imageToInputOffsetY, int rowCount, int columnCount, int layerMask) { - List<RaycastLayerHitSample> samples = new List<RaycastLayerHitSample>(); - Physics.SyncTransforms(); - - for (int row = 1; row <= rowCount; row++) - { - for (int column = 1; column <= columnCount; column++) - { - Vector2 inputPosition = CalculateGridInputPositionForGrid( - renderingImageSize, - imageToInputOffsetY, - rowCount, - columnCount, - row, - column); - GameViewRaycastResult raycastResult = GameViewRaycastUtility.RaycastFromInputPosition( - inputPosition, - UnityCliLoopConstants.RAYCAST_DEFAULT_MAX_DISTANCE, - layerMask, - false); - - samples.Add(CreatePointInfo(raycastResult)); - } - } - - return samples; + List<GameViewRaycastResult> results = RaycastDenseGrid( + renderingImageSize, imageToInputOffsetY, rowCount, columnCount, layerMask); + return results.ConvertAll(r => CreatePointInfo(r)); } +private static List<GameViewRaycastResult> RaycastDenseGrid( + Vector2 renderingImageSize, + int imageToInputOffsetY, + int rowCount, + int columnCount, + int layerMask) +{ + List<GameViewRaycastResult> results = new List<GameViewRaycastResult>(rowCount * columnCount); + Physics.SyncTransforms(); + for (int row = 1; row <= rowCount; row++) + { + for (int column = 1; column <= columnCount; column++) + { + Vector2 inputPosition = CalculateGridInputPositionForGrid( + renderingImageSize, imageToInputOffsetY, rowCount, columnCount, row, column); + results.Add(GameViewRaycastUtility.RaycastFromInputPosition( + inputPosition, UnityCliLoopConstants.RAYCAST_DEFAULT_MAX_DISTANCE, layerMask, false)); + } + } + return results; +}🤖 Prompt for 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. In `@Packages/src/Editor/FirstPartyTools/Screenshot/RaycastAnnotation/RaycastGridAnnotator.cs` around lines 31 - 63, The grid raycast logic in CollectRaycastGridPointsForGrid is duplicated from CollectClusterSamples, so extract a shared helper that performs the grid iteration and RaycastFromInputPosition calls and returns the raw GameViewRaycastResult values for both callers. Keep CalculateGridInputPositionForGrid as the single source for grid coordinates, and let each method handle its own filtering/storage (all samples vs hits-only) after the shared raycast pass. Also rename CollectRaycastGridPointsForGrid to a clearer, non-redundant name such as CollectDenseRaycastSamples or CollectLayerHitSamples to match the updated behavior and avoid the old RaycastGridPointInfo terminology.
🤖 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/Screenshot/ScreenshotUseCase.cs`:
- Around line 95-113: `ScreenshotUseCase` is building layer summaries without
respecting the resolved raycast mask, so the summary output can disagree with
`RaycastLayerNamesChecked`. Update the `CollectRaycastLayerSummaries` call in
`ScreenshotUseCase` to pass `effectiveLayerMask`, and thread that mask through
`RaycastGridAnnotator.CollectRaycastGridPointsForGrid` instead of hardcoding
`Physics.DefaultRaycastLayers`. Make the matching signature changes in
`RaycastGridAnnotator` and any helpers so all raycast-related results are
filtered by the same mask.
---
Nitpick comments:
In
`@Packages/src/Editor/FirstPartyTools/Screenshot/RaycastAnnotation/RaycastGridAnnotator.cs`:
- Around line 125-142: The method name CreatePointInfo is stale now that it
returns RaycastLayerHitSample, so rename it to match the actual purpose and
return type. Update the identifier used in RaycastGridAnnotator to something
like CreateLayerHitSample or CreateHitSample, and make sure any call sites or
references are updated to the new name for consistency.
- Around line 31-63: The grid raycast logic in CollectRaycastGridPointsForGrid
is duplicated from CollectClusterSamples, so extract a shared helper that
performs the grid iteration and RaycastFromInputPosition calls and returns the
raw GameViewRaycastResult values for both callers. Keep
CalculateGridInputPositionForGrid as the single source for grid coordinates, and
let each method handle its own filtering/storage (all samples vs hits-only)
after the shared raycast pass. Also rename CollectRaycastGridPointsForGrid to a
clearer, non-redundant name such as CollectDenseRaycastSamples or
CollectLayerHitSamples to match the updated behavior and avoid the old
RaycastGridPointInfo terminology.
🪄 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: db0b8af5-6cdc-4a68-ba40-cab2cf7c97fe
⛔ Files ignored due to path filters (1)
Packages/src/Editor/ToolContracts/RaycastGridPointInfo.cs.metais excluded by none and included by none
📒 Files selected for processing (14)
.agents/skills/uloop-screenshot/references/annotated-elements.md.agents/skills/uloop-simulate-mouse-input/SKILL.md.claude/skills/uloop-screenshot/references/annotated-elements.md.claude/skills/uloop-simulate-mouse-input/SKILL.mdAssets/Tests/Editor/RaycastGridAnnotatorTests.csPackages/src/Editor/FirstPartyTools/Screenshot/RaycastAnnotation/RaycastGridAnnotator.csPackages/src/Editor/FirstPartyTools/Screenshot/RaycastAnnotation/RaycastLayerMaskResolver.csPackages/src/Editor/FirstPartyTools/Screenshot/ScreenshotUseCase.csPackages/src/Editor/FirstPartyTools/Screenshot/Skill/references/annotated-elements.mdPackages/src/Editor/FirstPartyTools/SimulateMouseInput/Skill/SKILL.mdPackages/src/Editor/ToolContracts/RaycastGridPointInfo.csPackages/src/Editor/ToolContracts/ScreenshotResponse.csREADME.mdcli/common/tools/default-tools.json
💤 Files with no reviewable changes (1)
- Packages/src/Editor/ToolContracts/RaycastGridPointInfo.cs
戻り値の型がRaycastGridPointInfoからRaycastLayerHitSampleに変わった際、 メソッド名が古い型名を指したまま残っていた。CodeRabbitのレビュー指摘を 受け、実際の戻り値の型に合わせて改名した。
粗い5x5モードが廃止されたことで"GridPointsForGrid"という名前は 過去DTO(RaycastGridPointInfo)への冗長な参照になっていた。 実際の役割は「密グリッド全点でレイヤーヒットを標本化する」ヘルパー なのでCollectLayerHitSamplesにリネームし、レビュアーからの 可読性指摘に対応。呼び出し側はCollectRaycastLayerSummaries1箇所のみ。
Disposition of CodeRabbit findingsReviewed all 3 findings against current code, spec, and design intent. Two accepted (both applied), one rejected with rationale. 1. [Major] 2. [Nitpick] Rename 3. [Nitpick] Extract shared helper + rename
Verification after changes: |
|
Addressed CodeRabbit's review:
|
1 participant
Summary
screenshot --annotate-raycast-grid(without--raycast-layer-mask) now always uses the same dense clustered raycast pass as the masked mode, instead of a coarse 5x5 preview that could miss rotated or disconnected collider shapes entirely.RaycastLayerNamesChecked, the physics layer names actually eligible to produce hits in this response, to help diagnose why an expected layer produced no results.User Impact
--annotate-raycast-gridwithout a layer mask sampled only 25 points on a coarse grid. A rotated or split collider could fall between those sample points and appear completely undetected, even though the same object was hittable viaraycast --x --ydirectly — misleadingly suggesting the collider's hit detection itself was broken.--annotate-raycast-gridcall uses the same dense 40x40 sampling and clustering used by the masked mode, so collider shapes are outlined accurately regardless of orientation or whether the mask is specified.RaycastLayerSummaries(which layers exist and how many hits they have) is now always populated, not only when no mask is given.RaycastLayerNamesCheckedfield tells the caller exactly which layers were checked against the active camera, making it possible to tell "this layer produced no PhysicsCollider entries because the camera can't see it" from "because nothing is there."Changes
ScreenshotUseCase.CaptureRenderingAsynconto a single clustering code path; layer mask defaults toPhysics.DefaultRaycastLayerswhen--raycast-layer-maskis not given.RaycastLayerMaskResolver.CreateLayerNamesFromMaskto convert a layer bitmask into layer names, reused for the newRaycastLayerNamesCheckedfield.RaycastGridPointsfield,RaycastGridPointInfoDTO, and related dead code inRaycastGridAnnotator), replaced with the internalRaycastLayerHitSampletype used only for layer-summary aggregation.annotated-elements.md,SimulateMouseInput/SKILL.mdand their generated mirrors),README.md, anddefault-tools.jsontool descriptions to match.Verification
dist/darwin-arm64/uloop compile— 0 errors, 0 warningsuloop run-testsforRaycastGridAnnotatorTests(32/32),ScreenshotUseCaseTests(4/4),DefaultToolsCatalogDriftTests(1/1) — all greenRaycastAnnotationPerspectiveDemoScene: confirmed the previously-missed rotated/split collider is now correctly outlined,RaycastLayerNamesCheckedreflects the expected layer set with and without--raycast-layer-mask, andRaycastLayerSummariesremains populated in both cases