Restrict maximum zoom in Graph Editor

[vitvakatu]

Pull Request Description

Task link.

The maximum zoom factor of Graph Editor is limited to 1.0x. It is not possible to zoom in from the default camera position.
Debug Mode (activated with ctrl-shift-d shortcut) allows to zoom up to 100.0x (the previous behavior of Graph Editor).

If you enable Debug Mode, then zoom in and disable Debug Mode - you won't see the immediate change of zoom factor back to 1.0x. But it will "jump" (with animation) back once you make a zoom in/out event with your controls.

Video(updated):

2022-02-15.12-49-03.mp4

I was also confused by variable names in Navigator implementation, so the code was significantly refactored.

• I replaced direction with translation, because direction was used for both the normalized direction vector and translation vector of the camera.
• I started to use MIN_ZOOM and MAX_ZOOM constants instead of old max_zoom and min_zoom respectively (yes, they are reversed). This is because we operate on zoom factors now, not on camera distance values.
• So the min/max camera distances are calculated from zoom factors now. It may cause some confusion because minimum distance corresponds to the maximum zoom factor, and vice versa. I left a comment to make this clear.

Important Notes

• New values for maximum and minimum zoom factors roughly correspond to previous values. This is because the initial camera position is about 1014 units, while I was using a nicer value of 1000 in my calculations. It will not affect user experience though - the difference is too small to be spotted.
• I was experiencing rare glitches on the minimum zoom factor (the nodes were disappearing because the camera's far clipping plane was just as far as the maximum possible distance in Navigator). This is fixed in this PR, as we explicitly avoid moving beyond the far clipping plane.

Checklist

Please include the following checklist in your PR:

• The documentation has been updated if necessary.
• All code conforms to the Scala, Java, and Rust style guides.
• All documentation and configuration conforms to the markdown and YAML style guides.
• All code has been tested where possible.

[wdanilo]

I have 4 main issues with the code:

1. Poorly named variables (see the comment in the code). Always take extra care about naming of variables, please. It helps so much with code maintainability.
2. Not explained equations. There are some parts of the equation that I'm especially worried about, like the 1/something part. Such things tend to blow up with extreme values, sometimes cause by fast mouse/fingers movement or OS-specific value interpretation. Maybe it's correct, I don't know – I was not able to easily understand the code as it is not refactored to nicely named vars.
3. The video has several issues:
   1. It's in really low-quality, its hard to see how things behave there. For example, I'm not sure whether the camera "jumps" when its hitting its zoom limits.
   2. It does not show all possible corner cases, including:
      1. I would like to see how the scene behaves with slow zoom to the limit or very fast zoom to the limit. Also, not only zooming in the middle of the screen, but also more to a corner of a screen.
      2. What we should check is also how the zooming animation behaves when we are zooming to the limit while constantly zooming with our fingers (e.g. on touchpad) and how it behaves when we started zooming really fast, stopped touching touchpad, and the zooming increases because of inertia operator. It is very important that in both cases, when the zoom hits its limits, the scene is not accidentally paned.
      3. Also, how zooming behaves when its done just after the scene was panned (when the panning animation is still happening)?
4. On the video, the zoom "jumps" between second 10 and 11 (it zooms in, then zooms out). It's really hard to see if its an error or video artefact, because the video is so pixelated, but I feel its kind of artifact. It may be caused by the settings of our inertia animation, but in order to understand if it behaves correctly, the video should present all kind of corner cases, including slow and fast zooms. See:

Screen.Recording.2022-02-15.at.00.04.19.mov

[wdanilo]

I see several issues with this code:

1. The comment about min/max prefixes is added to a fovy_slope var, which is a little bit confusing.
2. The code is more repetitive than it was before this change. There are lines which are just copy-pasted from a code few lines above these lines.
3. Some variables are poorly named, like let max_zoom = max_zoom_limit.get().unwrap_or(max_zoom);. So max_zoom is either max_zoom_limit or max_zoom? If you already have max_zoom, why are you checking the limit? I don't understand the logic of the code, probably because of var names (not only in this place).
4. There are other poorly named variables, like limiting_factor - why limiting_factor multiplied by translation gives us position? In the old version of the code that was zoom_factor multiplied by direction. Wile I agree that translation is a way better name than the old direction (which was logically incorrect), the limiting_factor is not understandable.
5. Another name to be reconsidered is distance_to_show_full_ui - I know it was used in the previous code, but it is logically incorrect. I'm surprised we did not catch it in the review back when it was introduced. EnsoGL is a rendering framework for vector shapes – there is no ui abstraction here. There an UI library build on top of EnsoGL, but this library does not know about it.
6. There are other var names that should be improved (revisit all of them, please). This time I'm talking about max_translation_limit - earlier you are using translation for a geometric vector, a 3D value. Now, you define max_translation_limit as max_distance - position.z which seems like 1D value or a number. Geometric representation of a translation (3D) limit, should be a point in space (3D) or a plane (3D), not a number (1D). This makes the code strange in other places too. For example the translation var clearly refers to a geometric vector (3D variable), so writing translation.z > max_translation_limit does not make sense. In previous code direction.z > max_zoom_limit was way better, because zoom is a number, direction is 3D variable, so you can compare direction.z to a number.
7. I do not understand where the equations for min_distance and max_distance come from. Please refactor them well named variables that explain what all the components of the equation do. I'm especially afraid of the 1/max_zoom part – my intuition tells me that something is wrong there. Anyway, I'm not sure because these equations are not explained.
8. The code is not aligned – the if-then-else block is messy.

[vitvakatu]

1. Fair point, fixed it. The comment was left for the block of code (separated by blank lines) which is responsible for a distinct part of the computation.
2. Fixed, by introducing a closure in scope of the function. There are no places in the codebase where this equation is used, as far as I'm aware of.
3. This is a common pattern in Rust codebases, to shadow variable name with a new binding applying some additional operations on it. However, I slightly refactored that part of the code, as Adam suggested in his review.
4. I don't understand why zoom_factor is a better name than limiting_factor. We limit the movements of the camera near min/max distance along the z-axis. I've left it as it is (added a small comment), please suggest a better name if you want to change it. zoom_factor is extremely confusing, because we already had zoom_amount and we've already multiplied it by direction/translation.
5. Makes sense, however, it's hard to come up with a short name here. I choose distance_to_zoom_factor_of_1 which is rather long but looks self-explanatory to me. Please suggest a better name if you have an idea.
6. It's not clear to me, what other variable names I should revisit. You covered every one of them in this message, with an exception of too_far and too_close. Please ping me if you want to change anything you didn't explicitly mention. I renamed max(min)_translation_limit to positive(negative)_z_translation_limit. However, I don't agree that direction.z > max_zoom_limit was any better, mainly because max_zoom_limit was representing distance rather than actual zoom limit.
7. You're right, it doesn't make much sense from math standpoint, I was playing with different equations while trying to understand this code, and completely missed that 1.0 / x * y can obviously be replaced with y / x. I changed it. I'm not sure I should (and I can) explain why min_distance = distance_to_zoom_factor_of_1 / max_zoom - it's just how zooming works, we have a proportion of 1.0 / distance_1 = max_zoom / min_distance.
8. Fair point, however, we do not have such requirements in our Code Style guide. I've left a comment about it in Update Rust style guide #3172

[wdanilo]

Hi, thanks for the update:

3. I know that this is possible in Rust, but it just logically does not make sense to me. When reading the codebase I am trying to understand what the variables contain. You already have a variable max_zoom, so why are you computing it again? If the variable max_zoom does not contain real "max zoom", it should be renamed.

4. If I ask anyone from the team without a context what is zoom_factor, they will have intuition what it is. If I ask anyone what is limiting_factor no one would understand that this is a z-axis / depth translation limit. This var name just does not tell what it is about. If you really want to use limiting word, you can use z_axis_movement_limiting_factor as the var name instead. I just care a lot about how semantically clear var names are.

5. IMO distance_to_zoom_factor_of_1 is perfect. Sure, its kind of ugly, but semantically clear vars are way better than short vars.

6. I was just mentioning all vars in general - if we covered almost all of them, then probably not much is left. I was unsure if there are any left TBH.

7. I'll re-review the code in a second and I will check it again :)

8. OH WOW, I completely missed that PR. We should finish it and close it. I will check it out today.

[farmaazon]

Why do we use Rc::clone( notations instead of just .clone()?

[vitvakatu]

To clearly disambiguate cloning of pointer from cloning of the underlying data. It's a pretty common pattern in the Rust codebases I've worked with: https://rust-lang.github.io/rust-clippy/master/#clone_on_ref_ptr

[wdanilo]

We are using clone_ref here instead everywhere to show the same thing :)

[vitvakatu]

Apparently, not everywhere, as you can see in this code :) I can surely change it if you say so.

[wdanilo]

@vitvakatu absolutely! Also, clone_ref was not always available, so some old code can use different patterns. Sorry if my message was not clear enough – I wanted t write that nowadays we normally use clone_ref :)

[vitvakatu]

Changed

[vitvakatu]

@wdanilo I've updated video with a more high-res one:

1. I would like to see how the scene behaves with slow zoom to the limit or very fast zoom to the limit. Also, not only zooming in the middle of the screen, but also more to a corner of a screen.
   2. What we should check is also how the zooming animation behaves when we are zooming to the limit while constantly zooming with our fingers (e.g. on touchpad) and how it behaves when we started zooming really fast, stopped touching touchpad, and the zooming increases because of inertia operator. It is very important that in both cases, when the zoom hits its limits, the scene is not accidentally paned.
   3. Also, how zooming behaves when its done just after the scene was panned (when the panning animation is still happening)?

Regarding corned cases:

1. Tried to show it on video, both slow and fast zoom movements. Zooming focus point behavior (when you zoom slightly to the side( wasn't changed.
2. It's hard to show on video, so I suggest a QA/Reviewer check this scenario locally. The updated video actually covers this scenario, but it's impossible to distinguish when it happens. Basically the camera smothly stops near the limit no matter how fast you zoom in/out.
3. This wasn't changed, it looks like panning animation stops in such a case and we begin zooming. It's hard to check it using the touchpad though.

[farmaazon]

As our formatter does not align = in code, I would resign from alignment in macro blocks too.

[vitvakatu]

Please make a consolidated decision with @wdanilo on that matter, as he explicitly asked to align the code in #3273 (comment)

[wdanilo]

@farmaazon is right. The code should be aligned as our formatter does. What I told in my PR is that the code is partially aligned and looks messy (some { are aligned, = are aligned, some { are not aligned). But basically Adam is right:

[vitvakatu]

Changed

[wdanilo]

The code is super clean now and the vars are very understandable! Thank you for making it this way!