Refactor mesh selection into shared handler and improve camera focus selection behavior#604
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughA refactoring of ChangesSelection Logic Centralization
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Review rate limit: 0/1 reviews remaining, refill in 51 minutes and 26 seconds.Comment |
Deploying flockxr with
|
| Latest commit: |
b3c15cf
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f5ef8794.flockxr.pages.dev |
| Branch Preview URL: | https://codex-update-f-key-to-select.flockxr.pages.dev |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/gizmos.js`:
- Around line 429-432: The deselect path leaves a composite root's visibility at
0.001 because resetBoundingBoxVisibilityIfManuallyChanged isn't called; update
the click-to-deselect flow so after resetChildMeshesOfAttachedMesh() (and before
or after gizmoManager.attachToMesh(null)) you explicitly call
resetBoundingBoxVisibilityIfManuallyChanged for the previously attached mesh (or
invoke the same sequence turnOffAllGizmos uses) so the root mesh visibility is
restored; reference functions: applyMeshSelection,
resetChildMeshesOfAttachedMesh, resetBoundingBoxVisibilityIfManuallyChanged,
resetAttachedMesh, hideBoundingBox, gizmoManager.attachToMesh, and
turnOffAllGizmos to locate where to insert the call.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| if (gizmoManager.attachedMesh) { | ||
| resetChildMeshesOfAttachedMesh(); | ||
| gizmoManager.attachToMesh(null); | ||
| } |
There was a problem hiding this comment.
Composite mesh root visibility is not restored on deselect — visual regression
When a composite (e.g. glTF) mesh is selected, applyMeshSelection sets its root's visibility to 0.001 (line 409) so the bounding box renders. On deselect, the deselect path (lines 429-432) calls resetChildMeshesOfAttachedMesh() and gizmoManager.attachToMesh(null). The patched attachToMesh internally calls resetAttachedMesh → hideBoundingBox, but neither path calls resetBoundingBoxVisibilityIfManuallyChanged, so the root mesh's visibility stays at 0.001 — the model remains fractionally visible after being "deselected".
turnOffAllGizmos handles this correctly (line 1920), but it is not in the click-to-deselect flow.
🐛 Proposed fix
if (gizmoManager.attachedMesh) {
+ resetBoundingBoxVisibilityIfManuallyChanged(gizmoManager.attachedMesh);
resetChildMeshesOfAttachedMesh();
gizmoManager.attachToMesh(null);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/gizmos.js` around lines 429 - 432, The deselect path leaves a composite
root's visibility at 0.001 because resetBoundingBoxVisibilityIfManuallyChanged
isn't called; update the click-to-deselect flow so after
resetChildMeshesOfAttachedMesh() (and before or after
gizmoManager.attachToMesh(null)) you explicitly call
resetBoundingBoxVisibilityIfManuallyChanged for the previously attached mesh (or
invoke the same sequence turnOffAllGizmos uses) so the root mesh visibility is
restored; reference functions: applyMeshSelection,
resetChildMeshesOfAttachedMesh, resetBoundingBoxVisibilityIfManuallyChanged,
resetAttachedMesh, hideBoundingBox, gizmoManager.attachToMesh, and
turnOffAllGizmos to locate where to insert the call.
1 participant
Motivation
Description
applyMeshSelection(pickedMesh, pickedPoint)that centralizes selection behavior and ground click handling.focusCameraOnMesh()now callsapplyMeshSelectionto apply the same highlighting and gizmo attachment when focusing a mesh.handleSelectGizmo()was simplified to delegate toapplyMeshSelection, removing duplicated code for parent/root mesh resolution, visibility adjustments, bounding-box display, block highlighting, and ground readout.resetChildMeshesOfAttachedMesh()and detaches the gizmo viagizmoManager.attachToMesh(null)to restore child visibility consistently.Testing
Codex Task
Summary by CodeRabbit