feat(skeleton): viewport bone picking with gizmo priority - #906
Conversation
Allow mouse-picking skeleton-debug bones when the overlay is visible, preferring bones over meshes while keeping gizmos above bone picks and avoiding Edit Mode exit when re-selecting the active mesh. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Warning Review limit reached
Next review available in: 28 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: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughSkeleton debug visuals now retain their owning entity names. PropertiesPanelController exposes skeleton-debug activity queries, and TransformOperator prioritizes active skeleton bone picks across selection and transform mouse interactions. ChangesSkeleton bone picking
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MouseEvent
participant TransformOperator
participant SkeletonDebug
participant PropertiesPanelController
participant AnimationControlController
MouseEvent->>TransformOperator: provide screen click
TransformOperator->>SkeletonDebug: ray-pick tagged bone visual
TransformOperator->>PropertiesPanelController: verify skeleton debug activity
TransformOperator->>AnimationControlController: select owning entity and bone
TransformOperator-->>MouseEvent: bypass mesh selection or transform
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 34714890f0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/SkeletonDebug_test.cpp (1)
125-125: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the exact owning entity name.
A non-empty but incorrect tag still passes this test and would make picking resolve the wrong mesh. Compare
entityNameForMovable(obj)with the fixture entity’s actual Ogre name.As per coding guidelines, "
src/**/*_test.cpp: Add Google Test unit tests for new functionality."🤖 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 `@src/SkeletonDebug_test.cpp` at line 125, Update the test around SkeletonDebug::entityNameForMovable(obj) to assert equality with the fixture entity’s actual Ogre name instead of only checking that the result is non-empty. Preserve the existing movable object setup and use the fixture’s owning entity name as the expected value.Source: Coding guidelines
src/TransformOperator.cpp (1)
955-1025: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAdd focused tests for bone-picking priority and selection semantics.
Cover overlay-disabled misses, inactive-owner hits, gizmo overlap, mesh fallback,
ADD_SELECT/DEL_SELECT, and preserving Edit Mode when the owner is already selected. Tests should run under Xvfb without requiring a real display.As per coding guidelines, "
src/**/*_test.cpp: Add Google Test unit tests for new functionality. Tests should work under Xvfb and avoid assumptions about a real display."🤖 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 `@src/TransformOperator.cpp` around lines 955 - 1025, add focused Google Test coverage for TransformOperator::tryPickBoneAt, using Xvfb-compatible test setup without requiring a real display. Exercise overlay-disabled and inactive-owner misses, gizmo-overlap priority, mesh fallback, NEW_SELECT/ADD_SELECT/DEL_SELECT semantics, and the already-selected owner path preserving Edit Mode; use existing test fixtures and mocks/helpers for ray queries, properties, selection, and controller state rather than introducing unrelated refactors.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@src/SkeletonDebug_test.cpp`:
- Line 125: Update the test around SkeletonDebug::entityNameForMovable(obj) to
assert equality with the fixture entity’s actual Ogre name instead of only
checking that the result is non-empty. Preserve the existing movable object
setup and use the fixture’s owning entity name as the expected value.
In `@src/TransformOperator.cpp`:
- Around line 955-1025: add focused Google Test coverage for
TransformOperator::tryPickBoneAt, using Xvfb-compatible test setup without
requiring a real display. Exercise overlay-disabled and inactive-owner misses,
gizmo-overlap priority, mesh fallback, NEW_SELECT/ADD_SELECT/DEL_SELECT
semantics, and the already-selected owner path preserving Edit Mode; use
existing test fixtures and mocks/helpers for ray queries, properties, selection,
and controller state rather than introducing unrelated refactors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e8ab8776-16ad-44a5-b6c3-37d5c04e5786
📒 Files selected for processing (7)
src/PropertiesPanelController.cppsrc/PropertiesPanelController.hsrc/SkeletonDebug.cppsrc/SkeletonDebug.hsrc/SkeletonDebug_test.cppsrc/TransformOperator.cppsrc/TransformOperator.h
Hidden transform gizmos still carry GIZMO_QUERY_FLAGS in Select mode, so an unconditional gizmo gate stole bone hits. Gate only for W/E/R and cover the policy with a unit test; assert exact entity tags on overlays. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addressed review feedback:
|
|
1 participant
Summary
SelectionSet, so Edit Mode stays active when picking bones on the already-selected mesh.PropertiesPanelController::isSkeletonDebugActive/hasAnySkeletonDebugActive.Test plan
SkeletonDebug*/TransformOperator*Made with Cursor
Summary by CodeRabbit
New Features
Bug Fixes
Tests