Fix 3D Viewer zooming problem #2379
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
elyasbny
changed the title
servantftechnicolor
requested changes
Apr 22, 2024
| 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.
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.
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.
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.
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.