Add support for panning with arrow keys

[bryevdv]

This PR adds support for panning the legion profile viewer viewport using the arrow keys:

• left/right pans the viewport 5%
• shift + left/right pans the viewport 1%

The code changes here are very simple but I recall some discussion about possible complications, so please let me know of any other considerations I may have missed.

I also split the timestamp "tests" module into separate sub-modules to cover timestamps and intervals. Are inline tests idiomatic for Rust? Generally speaking I am more a fan of putting tests out-of-tree.

cc @elliottslaughter @manopapad @lightsighter

[manopapad]

The main concern that @lightsighter had brought up is that the profiler does aggressive culling of boxes based on the viewport. Therefore we'd just want to make sure that when the viewport moves, this visibility information is updated, so that any boxes that should become visible indeed show up. Does this appear to be the case if you expand one of the timelines, to see the individual operation boxes?

[bryevdv]

Unless I am misunderstanding, it seems to be behaving as expected:

[elliottslaughter]

@manopapad The culling is fully dynamic. I think that part is fine.

The pan() function need to copy some code from zoom(), or else the UI controls won't get updated. I pointed that out in a comment.

[elliottslaughter]

So, if this is how y'all want the UI to work, I'm fine with it. However, arrow keys are such an essential key binding that I want to make sure there isn't anything else we'd want to use it for.

One thing I had considered using arrow keys for was selecting panels in the sidebar. E.g., pressing DOWN SPACE (with nothing selected) would collapse node 0. Pressing DOWN DOWN RIGHT SPACE (with nothing selected) would collapse node 1 GPU. Etc.

I'm not saying that's a better use of the arrow keys, but I want to make sure we make a conscious decision to allocate arrow keys for this purpose, with the understanding that it precludes other uses.

[bryevdv]

Agree regarding arrow key consensus. If it is desirable to reserve arrows for the time being, then some alternatives:

• "a" and "d" (inspired by gameplay WASD) plus shift-variant
• "j" and "l" (inspired by vim) plus shift variant
• use arrow keys, but only with modifiers (shift and shift+ctrl? "shift" could be loosely for "pan")
• shift+drag (this would take me somewhat longer to figure out how to add without disturbing the current drag feature)
• something else?

[manopapad]

I would also suggesr Shift + mouse wheel, and horizontal mousepad move. Adding a horizontal scrollbar in the main viewport might enable this gesture automatically.

[bryevdv]

I would also suggest Shift + mouse wheel

I'm not a huge fan of mouse-wheel, since its arbitrary which scroll direction corresponds to what. But also not all mice have scroll wheels. I would only suggest adding scroll wheel as a secondary option after e.g. a keyboard option

and horizontal mousepad move.

There has to be some keymod + click + drag I think, since bare click + drag is already taken, is that the suggestion? I don't think we can (should) just hijack all plain mouse movements, even confined to the viewport.

Adding a horizontal scrollbar in the main viewport might enable this gesture automatically.

@elliottslaughter will have to comment for certain, but I don't have the impression that egui functions this way.

FWIW I'd suggest that any non-keyboard options would be best as a secondary to a primary keyboard interaction. But, I am just always going to be more favorable to keyboard solutions since they are more direct and pleasant to access IMO. (Maybe that's exactly what you are suggesting, it's not clear if you mean "in addition to" or "instead of")

[elliottslaughter]

I agree that having some sort of key binding is good. It's just a question of whether to use arrows keys or something else.

(Personally, I don't pan a lot, but it sounds pretty important to Mike's workflow, so I'm willing to let this take the arrows keys if people feel it's sufficiently important. This is the sort of thing we want users to weigh in on, because they're the ones who use it.)

I would also like horizontal scroll to be an option, because (in my opinion) it's the most obvious gesture people will try (at least on a trackpad). Again, it doesn't have to be the only option, but it would be good to support it.

For what it's worth, I tried the "just enable horizontal scroll" option and it didn't work. Or at least was behaving unintuitively given the amount of time I could invest. In principle egui supports this, but someone will need to look and see what I missed or got wrong.

[elliottslaughter]

Forgot to add: I'm less of a fan of modifier + mouse drag. It's undiscoverable (unless users go into the controls), and while there are definitely graphical tools that use such gestures (I think several 3D drawing tools use it), it's much less common. I think between keyboard and horizontal scroll we should be good.

[elliottslaughter]

Just for posterity, here's the main ScrollArea where we put the canvas:

https://github.com/StanfordLegion/prof-viewer/blob/f0d1d37db7ab75f36369ccb2b5fbf4ff08e99609/src/app.rs#L1360

ScrollArea has a both() API which, in principle, enables horizontal scroll. But it did not appear to have the behavior I was expecting, so I didn't go very far with it:

https://docs.rs/egui/latest/egui/containers/scroll_area/struct.ScrollArea.html#method.both

(To be clear, let's NOT add horizontal scroll in this change. But we can follow up with it in another PR.)

[elliottslaughter]

There's one other behavior we have to decide: how does this interact with the zoom history?

The current code does push a new zoom history item. I think that makes sense.

But should we modify the existing zoom history item? I.e., if you do something like:

1. Zoom to interval.
2. Pan left.
3. Undo zoom.

Should you go back to the zoom state at (1) or (2)? (I.e., before or after the pan?)

I think it would make sense to go back to (2), which would imply that panning should modify the zoom history entry at the current index.

[bryevdv]

horizontal scroll

I think I need to plead ignorance / ask for clarification. What is meant exactly by "horizontal scroll"? I've interpreted it as the effect, resulting from an interaction of some sort. But you both are using it to mean an interaction, I think.

Do you mean: a (single) visible scrollbar at the bottom that can be grabbed by clicking on it and dragged to scrub it in either direction? If not, I need a more detailed breakdown of specific gestures and/or widgets.

[manopapad]

FWIW I'd suggest that any non-keyboard options would be best as a secondary to a primary keyboard interaction

Makes sense. In that case I would personally be in favor of reserving the arrow keys for panning.

Another alternative, which allows us to accomodate both panning and selecting UI elements would be to allow Tab-ing to change focus between different parts of the UI; if the viewport has focus then arrow keys pan, if the the sidebar has focus then arrow keys move between elements in the sidebar.

There's one other behavior we have to decide: how does this interact with the zoom history?

I suggest that the first "undo zoom" should undo the pan (without actually changing the zoom level), then the second "undo zoom" should actually go back to the previous zoom level. This is how nsight systems viewer works, see https://github.com/StanfordLegion/prof-viewer/assets/215728/9f835e1a-4f86-4cef-ac0c-15854aa0e84a. We could call it "back" instead of "undo zoom" to be more accurate to the behavior.

I would be in favor of combining multiple consecutive pans as a single undoable action (it looks like nsight only does this if consecutive pans happen within a short timeout).

What is meant exactly by "horizontal scroll"?

I was thinking of horizontal scrolling on a trackpad (2-finger scroll gesture to the left or right).

[lightsighter]

Do you mean: a (single) visible scrollbar at the bottom that can be grabbed by clicking on it and dragged to scrub it in either direction?

Just clarifying since I originally asked for this. I want the same behavior that we currently have with vertical scroll for the horizontal scroll too: meaning there is a scroll bar on the side that you can click and drag if you want to, and preferably any gestures for horizontal scrolling on a track pad (two finger drag on my Mac) work to move from side to side also (just like they do in any other page or app). Note that vertical scrolling already just works like this for me at least.

(To be clear, let's NOT add horizontal scroll in this change. But we can follow up with it in another PR.)

Agreed. I'll take side to side arrows over nothing. :)

Personally, I don't pan a lot, but it sounds pretty important to Mike's workflow, so I'm willing to let this take the arrows keys if people feel it's sufficiently important. This is the sort of thing we want users to weigh in on, because they're the ones who use it

I think using the arrow keys for this is fine as I can't think of a more intuitive use for the arrow keys. Although if we decide to not use the arrow keys, then I'd like the vim key bindings for stepping in all four direction ('hjkl'), but that's just my preference as a vim user.

[bryevdv]

I was thinking of horizontal scrolling on a trackpad (2-finger scroll gesture to the left or right).

Ah, I never use that / always disable it. It's not entirely clear to me from these docs that that event is exposed in egui but can investigate more in the future.

[bryevdv]

We could call it "back" instead of "undo zoom" to be more accurate to the behavior.

+1 along these lines @elliottslaughter I would propose renaming:

• interval_state -> select_state (or select_interval_state)
• zoom_state -> interval_state (or view_interval_state)

[elliottslaughter]

We could call it "back" instead of "undo zoom" to be more accurate to the behavior.

This is too generic. "Back" what?

Personally (and maybe this is just me being weird), I think the word "zoom" generalizes to panning. (I see a pan as a special, limited form of zooming, which is exactly why I don't need the pan controls in the first place. I just undo the zoom and redo to what I want to look at.)

But if y'all disagree with me that "zoom" generalizes to panning, then at least it should be "Undo zoom/pan". That's more specific.

I suggest that the first "undo zoom" should undo the pan (without actually changing the zoom level)

Ok. So what happens if you press pan twice? Is that two entries in the zoom/pan history or one?

If N pans gives you N entries in the zoom history, what happens when we add true horizontal scrolling and you get 60 pan entries in the history per second (or whatever your FPS is)? I think it becomes obvious that this hits usability limits pretty quickly.

If we want pan entries to be recorded in the zoom history but only take up one entry, that implies encoding the type of zoom/pan event in the history so we can collapse consecutive pans.

@elliottslaughter I would propose renaming:

interval_state -> select_state (or select_interval_state)

I would call it interval_select_state to match the type. Using select on its own is too generic; there are many things the user can select.

zoom_state -> interval_state (or view_interval_state)

Again, I personally think zoom generalizes, though perhaps that's idiosyncratic. If you disagree, then I think the name has to be something like view_history_state or view_interval_history. interval alone is not specific (there are many intervals in the profiler). And the view_interval can potentially have many kinds of state; this is the history of the intervals we've picked.

[manopapad]

But if y'all disagree with me that "zoom" generalizes to panning, then at least it should be "Undo zoom/pan". That's more specific.

I don't really mind what we call it, I'm fine with calling it "undo zoom" even if it also undoes pans.

Ok. So what happens if you press pan twice? Is that two entries in the zoom/pan history or one?

I would be in favor of combining multiple consecutive pans as a single undoable action.

[bryevdv]

Again, I personally think zoom generalizes, though perhaps that's idiosyncratic.

FWIW I definitely have a strong association of "zoom" with a change in overall scale. I have not personally encountered it used to cover things like panning where the scale is unchanged.

If we want pan entries to be recorded in the zoom history but only take up one entry, that implies encoding the type of zoom/pan event in the history so we can collapse consecutive pans.

Agreed. I'd probably look to have the pan codepath collapse the latest entry right at the time of the pan, in case the latest is also a pan. Then the undo still only just has to pop the last interval to apply.

I'll proceed with renaming to interval_select_state and view_interval_history for now, since I think these will be appropriate and a bit more descriptive, regardless of how the other questions are decided.

I'm fine with calling it "undo zoom" even if it also undoes pans.

Ideas: "undo", "undo view", "last view", "view back", "reset view", "revert view", all seem comprehensible to me. As a user, "undo zoom" would be confusing to me here.

[elliottslaughter]

Ideas: "undo", "undo view", "last view", "view back", "reset view", "revert view", all seem comprehensible to me. As a user, "undo zoom" would be confusing to me here.

What about "undo zoom/pan"? It's wordier, but I think it's clear.

I had considered "view" as a generic for "zoom/pan", but I think it's less clear since view can be both a verb and noun.

[bryevdv]

What about "undo zoom/pan"? It's wordier, but I think it's clear.

Also seems very clear! Do we want to proceed with that in this PR or in a follow-on?

[bryevdv]

@elliottslaughter I went ahead and added the undo/redo work in https://github.com/StanfordLegion/prof-viewer/pull/27/commits/49254cb80c8c65894caedac2f44643e48c15d444. For some reason undo/redo implementations easily leave me turned around, so let me know if there's anything off. It behaved as discussed above as best I can tell.

Edit: whoops, control help text update in 2ba4bc0

[elliottslaughter]

Confirmed that it's working the way I expect. Merged.

Thanks again!