High-level fj-viewer interface

[jeevcat]

See #764 (comment)

Open questions:

1. Does it make sense to pass Actions from fj-window to fj-viewer and back again?
2. fj-viewer now uses resolution-independent cursor positions everywhere (normalized to [-1, 1]). This means I had to change the magic number here to feel right. What was the meaning/units of this number? https://github.com/hannobraun/Fornjot/blob/edf7a6636d1d0323d7e56943c3fc96ab4cfd46a3/crates/fj-viewer/src/input/rotation.rs#L32
3. I've introduced a small change in behavior of the "focus point" which might not be desirable. On left and right clicks a new focus point is chosen. There is no way to remove an old focus point, only replacing when clicking again. Zooming now uses the most recently created focus point, instead of the current cursor position. Is this fine, or should I reimplement the old behavior (Mouse down creates focus point, mouse up removes focus point, temporary focus point calculated for each zoom event)?

[hannobraun]

Thank you for the pull request, @jeevcat! I intend to take a look later today and will answer your questions then.

[hannobraun]

Thanks again, @jeevcat! I definitely like where this is going. I've left a comment with a bit of a nitpick, but that isn't a blocker for merging.

The only thing I'm not so sure about is the new focus point behavior. See my answer below.

On to your questions.

---

Does it make sense to pass Actions from fj-window to fj-viewer and back again?

No, not really. In fact, I think Actions should no longer exist, as it is now redundant with input::Event. input::Handle::handle_event should receive a &mut DrawConfig instead.

fj-viewer now uses resolution-independent cursor positions everywhere (normalized to [-1, 1]). This means I had to change the magic number here to feel right. What was the meaning/units of this number?

It's just that: a number to make to make the input feel right. f probably stands for "factor", so no unit.

I think it would make sense to give it a real name, change it to a const, and add some documentation. But that doesn't need to be done here and now (but feel free to, if you want).

I've introduced a small change in behavior of the "focus point" which might not be desirable. On left and right clicks a new focus point is chosen. There is no way to remove an old focus point, only replacing when clicking again. Zooming now uses the most recently created focus point, instead of the current cursor position. Is this fine, or should I reimplement the old behavior (Mouse down creates focus point, mouse up removes focus point, temporary focus point calculated for each zoom event)?

My immediate reaction is that I don't like the new behavior, for the following reasons:

• It's slightly more complicated, for no clear (to me) benefit.
• It introduces state that is hidden from the user, which could lead to confusion.

But that is not a very strong opinion. What do you think, does my reasoning make sense?

---

I think this pull request is mostly ready to be merged! The only blocker is to decide whether to revert to the old focus point behavior or not.

Any other improvements can be done in follow-up PRs and don't need to hold up merging this one.

[hannobraun]

The metaphor that Fornjot's UI follows, is that you manipulate the model, not an abstract camera. So maybe Translate and Rotate would be better names than Pan/Orbit.

I realize that this metaphor isn't fully followed everywhere, especially in code comments. So there's no need to follow it here right now, and I certainly won't block merging this pull request on this detail. I'm just mentioning it, in case you have thoughts, and to document where I'd like to see things go mid- to long-term.

(Side note: Maybe we should rename Camera to Observer or Spectator to reinforce the metaphor and reduce confusion. Maybe not a good idea.)

[jeevcat]

Makes sense. I'll fix this up.

On a related note. I feel like the camera object shouldn't exist in fj-window at all. Seems like it belongs in fj-viewer.

[hannobraun]

It already lives in fj-viewer: https://github.com/hannobraun/Fornjot/blob/main/crates/fj-viewer/src/camera.rs

[jeevcat]

Hm, I guess I mean the lifetime could also be controlled by the viewer. Right now it is instantiated in fj-window.

[hannobraun]

Yeah, that makes sense. The way all of this is structured pretty much reflects the fact, that it lived in a single crate not long ago. It probably makes sense to have a single Viewer struct with methods like handle_input and draw_graphics. No need, really, for fj-window to deal with input::Handler, Renderer, etc. separately.

[hannobraun]

Now that I think about it, maybe f should live in fj-window, together with the other tuning values.

[jeevcat]

Thanks for all the feedback. Regarding the focus point behavior, the main reason for changing it was just to reduce complexity in implementation. But I agree with your points, so I'll make sure it works the same as previously.

Let me make the following changes then I'll un-draft this PR:

• Ensure same focus point behavior
• Remove Actions
• Extract and document rotation constant
• Rename events in accordance with model manipulation metaphor

[hannobraun]

I've already un-drafted it for some reason, but otherwise 👍

[hannobraun]

Thanks again, @jeevcat. Looks great!