-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix 3D Viewer zooming problem #2379
Conversation
var d = (root.camera.viewCenter.minus(root.camera.position)).length() * 0.2 | ||
var tz = (wheel.angleDelta.y / 120) * d | ||
var d = root.camera.viewCenter.minus(root.camera.position).length() // Distance between camera position and center position | ||
var tz = (wheel.angleDelta.y / 120) * d * 0.2 // Translation to apply depending on user action (mouse wheel), bigger absolute value means we'll zoom/dezoom more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move out the constant values to named variables and explain their meaning. 120 ? 0.2 ? 0.001 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to do so, I don't know if it's okay now
} | ||
|
||
// We forbid too small zoom as it means we are getting very close to center position and next zoom may lead to similar problem as previous cases (no translation) | ||
if (tz > 0 && tz <= 0.001) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks to me we should test the distance d directly !! ("it means we are getting very close to center position") why using some function of d when d is ok ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested with d, but the bug still occurs (the condition tz > 0 cannot be replaced with d > 0), so I kept tz
… when zooming too much.
Description
When zooming in the 3D Viewer, we get stuck when we come the camera position and the viewpoint are too close.
This fix solves this problem by not allowing us those two points to get too close.
Solved with @almarouk and @Just-Kiel
Implementation remarks
The zoom and dezoom depends on the distance between camera and center position, if this distance is too low, the translation we apply to zoom/dezoom won't have any impact in the worst case as we are limited by float representation (and that's why we are stuck), or would be very low in the other case (and we'll need more user actions to have a visible dezoom).
There are two possibilities for the camera position to be too close to the center.
1/ We forbid too big zoom, as it means the distance between camera and center would be too low and we'll have no translation after (due to float representation)
2/ We forbid too small zoom as it means we are getting very close to center position and next zoom may lead to similar problem as previous case (no translation)
We could have done it in another way: if we compute the next camera position (without applying it), and test future dezooms with the new distance, we can notice if it will lead the 3D viewer to be stuck and forbid this camera position.
We didn't do it, as it is leading to a more complex code and a higher computational cost, and our code already solves this issue in a convenient way for the user.