Simplify zoom and adapt for larger models

[jeevcat]

This PR removes the interpolated zoom system and replaces it with a basic instantaneous zoom system. The new algorithm moves the camera proportionally based on the current distance from the focus point. This means smaller changes in zoom when close to an object and larger changes in zoom when further away.

Please treat this PR as more of a proposal, as I have I deviated significantly from the initial design.

Close #677.
Close #25.

[hannobraun]

Thank you for the pull request, @jeevcat!

I haven't had a chance to take a close look and test this yet. I will do so later. If it works reasonably well, I will likely merge it. The current system isn't working so well, after all, and this is much less code.

[jeevcat]

I was just reading the description of #678, and noticed that I'm also using the origin as a fallback for the focus point. If you're happy with the new zoom behavior you could just merge this PR and I'll make the suggest changes (always fallback to center of bounding box of model) in a following PR addressing #678. Otherwise, I could extend this PR with the changes.

[hannobraun]

Thanks again for the pull request, @jeevcat!

I've taken a look now. Your approach seems sound, but the value of ZOOM_FACTOR seems way too low. With the test model (the default one you get with cargo run) it takes forever to zoom in to look at details (using my mouse wheel; haven't tested with a track pad yet). Unfortunately, we don't have a very large model in the repository right now.

A value of 2.0 works pretty well for me. I wonder, if there's some difference with our hardware setups that makes a big difference?

I was just reading the description of #678, and noticed that I'm also using the origin as a fallback for the focus point. If you're happy with the new zoom behavior you could just merge this PR and I'll make the suggest changes (always fallback to center of bounding box of model) in a following PR addressing #678. Otherwise, I could extend this PR with the changes.

It probably makes sense to do that in a separate pull request, as the fallback focus point should be the same for all operations. Keeping that change in a separate PR form the zoom-specific changes makes things a bit easier down the line (when I have to write the Weekly Dev Log or update the changelog for the next release).

[jeevcat]

I've tested again and 2.0 is fine for my PC with a mouse wheel but is maybe a tiny bit too sensitive for my MacBook's trackpad. 1.0 seems pretty reasonable for me on both though. What do you think?

[hannobraun]

I've tested again and 2.0 is fine for my PC with a mouse wheel but is maybe a tiny bit too sensitive for my MacBook's trackpad. 1.0 seems pretty reasonable for me on both though. What do you think?

1.0 seems a tiny bit too slow with the mouse wheel, but I would accept it 😄

After some more experimentation, 2.0 seems a bit too fast though. I've settled on 1.5 as my new favorite.

Regarding the trackpad, please check out this piece of code in fj-window:
https://github.com/hannobraun/Fornjot/blob/db910c73dc0e546dd6fda17747a6546d7cae0084/crates/fj-window/src/run.rs#L169-L176

One of those variants provides the value for mouse wheel, the other for trackpad (I forget which is which; this could use some comments). Maybe you could play around with that to find a value that works for you, then I can double-check against my own laptop?

[jeevcat]

@hannobraun I've moved the zoom speed decision down into zoom.rs so we could remove the magic number in run.rs. Let me know how the behavior feels now.

[hannobraun]

Thanks again, @jeevcat! I'll test this later, once I get a chance.

I'm not sure that structuring the mouse scroll event like this is the best way to do it. A potential alternative Fornjot application that uses fj-viewer but not fj-window might need its own tuning values anyway, if whatever windowing solution it uses doesn't line up with winit in that regard. And even the winit-based fj-window might need different tuning values for different contexts (for example with touch-based input, but I'm just speculating).

In any case, this is fine for now. I'm happy to merge it like this, if it means zoom works better afterwards. I'm just speculating that we might want to revert that particular change at some point.

I also left a comment about some minor issues in the code.

[hannobraun]

A few issues with the documentation here:

• It says Line is horizontal, but they're both vertical.
• I believe the sign has the same meaning for both variants, so it probably makes sense to move the comment about positive values to the struct-level doc comment.

[hannobraun]

Okay, I decided to make some time for testing right now. Which was a strategic decision about what should go into the next release, and not at all an obvious ploy to further procrastinate on writing the release announcement.

Feels great, both with a mouse and a trackpad! I'm going to merge this right now. Any further improvements to architecture and/or doc comments can be done later.

[jeevcat]

Sounds great!

Any further improvements to architecture and/or doc comments can be done later.

When you get the chance, let me know what you're thinking and I'll happily tidy things up.

[hannobraun]

When you get the chance, let me know what you're thinking and I'll happily tidy things up.

Basically just what I said in this review. The documentation I commented on is an obvious opportunity for improvement, but I don't know, if it makes sense to tackle the architectural stuff now (i.e. whether to revert your zoom event change or not).

Might be best to just wait and address this later, once we have more information. What do you think?

[jeevcat]

I'm not sure that structuring the mouse scroll event like this is the best way to do it. A potential alternative Fornjot application that uses fj-viewer but not fj-window might need its own tuning values anyway, if whatever windowing solution it uses doesn't line up with winit in that regard. And even the winit-based fj-window might need different tuning values for different contexts (for example with touch-based input, but I'm just speculating).

So you're thinking move the tuning into fj-window, instead of where I put it in fj-viewer? My reasoning was that fj-viewer already exposes events like CursorMoved, Key and Scroll. If it exposed more abstract events like "Zoom" and "Pan" then something like ZoomDelta (which would be calculated in fj-window) instead of the current MouseScrollDelta would make more sense. But because we're passing these lower-level events, passing the type of scroll input made sense to me. What are your thoughts?

I'll fix up the documentation errors in a new PR now.

EDIT: here's the PR: #781.

[hannobraun]

I figure that tuning outside of fj-viewer might be necessary anyway, since different sources of input events might expose different values, and fj-viewer really can't (or shouldn't!) know about where these events are from. So yeah, moving the tuning outside might make sense. The event would then just transport "one unit of zoom", whatever that means, and fj-window would be responsible for making that feel good.

My reasoning was that fj-viewer already exposes events like CursorMoved, Key and Scroll. If it exposed more abstract events like "Zoom" and "Pan" then something like ZoomDelta (which would be calculated in fj-window) instead of the current MouseScrollDelta would make more sense. But because we're passing these lower-level events, passing the type of scroll input made sense to me. What are your thoughts?

Yeah, that reasoning is perfectly valid. That code is probably not the best thing to base your reasoning on though, but how would you know that, if I didn't document my intent properly 😄

I think having those higher-level events you talk about makes total sense. Here's my latest thinking, based on this discussion and previous ones:

The reason those events exist, is because the winit-specific stuff was moved out of fj-viewer. The reason that was done, was to enable alternative applications that don't use winit (one contributor was working on something, but I haven't heard about that for a while). We shouldn't assume that those alternative applications are similar to the current fj-app (they might be optimized for touch interfaces, for example), so exposing higher-level events makes more sense than exposing the current lower-level ones.

[jeevcat]

so exposing higher-level events makes more sense than exposing the current lower-level ones.

Okay I agree. Let me open a follow up PR which makes the input interface to fj-viewer more high-level and the we can discuss whether it makes sense to merge.

[hannobraun]

Let me open a follow up PR which makes the input interface to fj-viewer more high-level and the we can discuss whether it makes sense to merge.

Sounds great! 👍