Keep zoom during rotation using the buttons of the 3D-viewport #4750
Conversation
fm3
changed the title
daniel-wer
changed the title
There was a problem hiding this comment.
Nice work digging into this and simplifying the code! 👍 I think there's still a little bit of room to make this work even better :)
One exception: The rotation of the buttons might look a bit off due to not using always the shortest path because we do not use quaternions for interpolation.
This distracts me more than I would've thought. The "crazy" rotations that happen sometimes make it hard to keep the correct spatial picture in my head. I tested the buttons using the old code, and there the rotations seem to always use the shortest path - What's the difference to the new code, why does it not work that way any longer?
On a more general note, please proofread the comments you're writing to make sure they're concise, understandable to future readers and without spelling mistakes. :)
| b: squareCenterY - height / 2, | ||
| }; | ||
| // Factor to reduce the clipping offset of the z coordinate to get a better angle on the dataset. | ||
| const zReductionFactor = 0.7; |
There was a problem hiding this comment.
Is this needed because of the different z-scale in relation to x and y? If so, shouldn't it be computed from the dataset scale automatically as dataset scales differ widely?
There was a problem hiding this comment.
I thought of this as a way to simply change the view angle at the dataset. Here are two examples:
With zReductionFactor = 1:
With zReductionFactor = 0.7:
As you can see: The angle changed.
But if you compare these I think the first version is fine too and thus the factor can be omitted.
To be honest, I had trouble understanding how the previous code worked. That's why I cannot say what actually the difference is between before and this version. But maybe I should really investigate some more time into fine-tuning the rotation to not do something "crazy" |
There was a problem hiding this comment.
This distracts me more than I would've thought. The "crazy" rotations that happen sometimes make it hard to keep the correct spatial picture in my head. I tested the buttons using the old code, and there the rotations seem to always use the shortest path - What's the difference to the new code, why does it not work that way any longer?
To be honest, I had trouble understanding how the previous code worked. That's why I cannot say what actually the difference is between before and this version. But maybe I should really investigate some more time into fine-tuning the rotation to not do something "crazy"
Definitely kudos to you for digging through the old (and not easy-to-read) code and adapting it 👍 👍 👍
Regarding the rotations, maybe it's easiest to just use quaternions (which threejs supports actively) instead of trying to understand what changed in this PR (since the animation wasn't perfect before, either). Here's an example in threejs (code is linked, too):
https://threejs.org/examples/#webgl_math_orientation_transform
So, maybe this is worth a try to integrate quaternions? If this is too much of a hassle, we could re-consider the importance. But maybe it's "quite simple"? :)
Co-authored-by: Daniel <[email protected]>
…ableminds/webknossos into keep-zoom-during-td-button-rotation
|
@daniel-wer I tried out using quaternions to smooth the rotation, but somehow the camera does not update after applying the desired quaternion. I pushed the code I used to test rotating a not used camera to branch -> Thus I would not invest more time into this unless a customer or so is really irritated by the weird rotation. Previously, the rotation was also a bit off in some cases. |
|
@daniel-wer I had another chat with @philippotto about the weird rotation and we found out that the "3D" preset had "wrong" values and corrected them to work fine with the other presets. Thus the rotation between all presets does not look weird anymore. Nevertheless, the problem that the rotation in some cases is looks weird still exists. For that, we need to interpolate the position on a sphere. But this should be done in another pr 🙂 |
|
@philippotto As daniel is currently unavailable, do you want to take over reviewing this pr? |
Sure thing! I just gave it another test and this PR vastly improves the rotation 👍 Very pleasant to use. However, I noticed one issue concerning the aspect ratio / scale. If the 3D viewport is not square, the rendered plane is squished somehow: Compare this to the current master: In both cases, I clicked the XY button. Probably |
|
@philippotto Thanks for finding this bug. I accidentally forgot to remove a reassignment of width and height. The bug should be fixed now |
|
Just wanted to say, the rotation is much better now! Really cool work 🚀 |
This PR reworks the functionality of the rotation buttons of the 3D-Viewport. The zoom is now kept during rotation and the calculation is now shorted, commented and easier to understand.
URL of deployed dev instance (used for testing):
Steps to test:
Issues:
[ ] Updated (unreleased) migration guide if applicable[ ] Updated documentation if applicable[ ] Adapted wk-connect if datastore API changes[ ] Needs datastore update after deployment