fix: add renderOrder to AxisRotator #2504
Conversation
|
Someone 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. |
romankoho
changed the title
|
I’d suggest making it a prop rather than hard coding the renderOrder. Also to preserve backwards compatibility, making the prop optional would be good |
|
@FarazzShaikh I agree on that. But in this case I also replaced the hard coded value from ScalingSphere and AxisArrow with the property. |
| @@ -115,6 +116,7 @@ export const PivotControls: ForwardRefComponent<PivotControlsProps, THREE.Group> | |||
| rotationLimits, | |||
| scaleLimits, | |||
| depthTest = true, | |||
| renderOrder = 500, | |||
There was a problem hiding this comment.
Let’s default it to undefined please. As that was the original behavior (backwards compatibility). Unless 500 is absolutely necessary for it to work?
There was a problem hiding this comment.
you are right in regards to the AxisRotator. But if you look at the current code of AxisArrow and ScalingSphere you can see that there the value is hard coded to 500.
So either backwards-compatibility for AxisArrow andScalingSphere is broken or for AxisRotator. I plead for setting the default to 500 as this rather fixes AxisRotator than breaking it. Setting default to undefined could potentially break AxisArrows and ScalingSpheres which were working until now.
There was a problem hiding this comment.
@FarazzShaikh if this is a show stopper I'm fine with setting it to undefined.
There was a problem hiding this comment.
@FarazzShaikh any opinion on my reply? I'd love to see this being merged.
There was a problem hiding this comment.
I’ll merge it tonight. Just need to verify it works as expected with the 500 render order
There was a problem hiding this comment.
amazing, thank you!
Why
this PR closes ticket #2503
What
Checklist
I need this urgently ;) would be nice if it can be merged soon