allow disabling controls per axis#2706
Conversation
|
@romankoho is attempting to deploy a commit to the Poimandres Team on Vercel. A member of the Team first needs to authorize it. |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
|
Why did you close the last PR for this? It removes the review/discussion threads and with bots/security being issues for libs it raises hairs. |
|
oh I see the git history note... |
DennisSmolek
left a comment
There was a problem hiding this comment.
All in all this seems good.
It doesnt solve your ultimate problem though and requires users to straight disable them instead if the rotation to be off when the camera at the extreme angle.
These props are useful so I support merging it, however I made some semantic recommendations.
I would also be interested in an expanded version that takes the camera normal and potentially disables the rotation arcs which solves your actual problem.
The next thing is we are avoiding v10 merges unless fixing breaking/major things, so I'm not sure this would go in soon on v10, however, if you PR this against v11 with those minor changes I will merge it in immediately.
travisbreaks
left a comment
There was a problem hiding this comment.
Both issues from my review on #2701 are resolved:
-
Inverted ternary:
disableScalingFinalnow follows the sameArray.isArray(x) ? x : [x, x, x]pattern as the other three props. Good. -
Axis index mapping: Sliders and rotators now index by the
axisprop value (the normal axis) instead of render order.disableSlidersFinal[2]guardsaxis={2},[1]guardsaxis={1},[0]guardsaxis={0}. Consistent withdisableAxesanddisableScaling. This is exactly what I suggested.
Clean diff overall. The typo fix and storybook entry are nice touches. LGTM.
|
thanks @travisbreaks and @DennisSmolek for your review! @travisbreaks I'm also curios about your thorough review. Did you you write it yourself or use a tool? |
c542252 to
7361cb4
Compare
|
hey @DennisSmolek any more changes needed or is it good to merge? |
|
you guys can also check out #2708 |
Ill spend time this weekend on drei and likely merge this to v11. Sorry for the delay. Starting a new job ate up some time. Will be catching up though |
Why
When using the PivotControl with an orthographic camera certain elements of the control can collapse / project on the same plane.
In my screenshot below you see how the pink arch of the AxisRotator collapses with the arrow of the AxisArrow. The user tries to move the object along the axis but instead he/she interacts with the AxisRotator and rotates the element unintentionally.
In this case I would want to be able to control the visible elements per axis
With the solution implemented I would be able to hide the arch for the x and z axes if the camera forward vector is (almost) perfectly aligned with the world-up vector.
What
1. The disable props of the PivotControls now either accept a boolean to disable the element for all axes or an array of three booleans to turn on/off the element per axis.
the flag is then considered when rendering
2. fix typo in PivotControls index.tsx (noticable to noticeable)
Checklist
I had PR #2701 open but messed up the git history (a noob was doing noob stuff)