Timeline arrow key navigation #89
Conversation
Add inital support for left and right arrow key navigation in the timeline. It was necessary to turn off the default navigation as it was interacting badly. Signed-off-by: Thomas Wilshaw <thomaswilshaw@gmail.com>
Signed-off-by: Thomas Wilshaw <thomaswilshaw@gmail.com>
Signed-off-by: Thomas Wilshaw <thomaswilshaw@gmail.com>
Signed-off-by: Thomas Wilshaw <thomaswilshaw@gmail.com>
Signed-off-by: Thomas Wilshaw <thomaswilshaw@gmail.com>
Signed-off-by: Thomas Wilshaw <thomaswilshaw@gmail.com>
|
I'm marking this as ready because it does work. However, I feel it's a bit clunky and could be polished so any feedback would be great. 2024-12-02.19-02-48.mp4 |
There was a problem hiding this comment.
This looks great! I just tried it out and it is so much better than before.
I left a couple of comments about checking for null to avoid crashing in some edge cases.
A couple of additions that would be helpful (and could be added later):
- Scrolling up/down when the selection is off the top/bottom.
- Stepping from the bottom video track to the top audio track and vice-versa.
| // Left Arrow | ||
| if (ImGui::IsKeyPressed(ImGuiKey::ImGuiKey_LeftArrow)) { | ||
| std::string selected_type = appState.selected_object->schema_name(); | ||
| if (selected_type == "Clip" || selected_type == "Gap" || selected_type == "Transition") { |
There was a problem hiding this comment.
Instead of checking the schema_name, it would be safer and shorter to check if the dynamic_cast to Composable returned a value. If it was non-null, then you can find the parent. For safety you might also want to check that parent() was non-null, since the top-level stack won't have a parent. That will cover all the generalized cases - though the left/right navigation is really assuming the parent is a Track (vs a Stack).
So maybe the logic could be:
- Try dynamic_cast to Composable.
- Get the parent()
- Check that the parent is not null
- Check that the parent is a Track
| if (selected_type == "Clip" || selected_type == "Gap") { | ||
| auto child = dynamic_cast<otio::Item*>(appState.selected_object); | ||
| start_time = child->range_in_parent().start_time(); | ||
| } else if (selected_type == "Transition") { |
There was a problem hiding this comment.
You could avoid this special case by using parent->range_of_child(child) which works on all Composables.
| } | ||
|
|
||
| // Get the parent object and track stack | ||
| auto parent = dynamic_cast<otio::Composable*>(appState.selected_object)->parent(); |
There was a problem hiding this comment.
There's already a parent variable on line 1574.
|
|
||
| // Get the parent object and track stack | ||
| auto parent = dynamic_cast<otio::Composable*>(appState.selected_object)->parent(); | ||
| auto tracks = dynamic_cast<otio::Stack*>(parent->parent()); |
There was a problem hiding this comment.
You'll need to check that tracks was non-null. If you have an OTIO with nested compositions, the parent's parent might not be a Stack. At the moment, I'm not sure you could end up selecting such an item in Raven, but we will allow for it eventually.
| if (track == parent) { | ||
| // Down Arrow and Video or Up Arrow and Audio | ||
| if ((ImGui::IsKeyPressed(ImGuiKey::ImGuiKey_DownArrow) && selected_track_type == "Video") || | ||
| (ImGui::IsKeyPressed(ImGuiKey::ImGuiKey_UpArrow) && selected_track_type == "Audio")) { |
There was a problem hiding this comment.
Oof, sorry about this track order hassle. I often wish we had designed Timeline to have separate lists for video and audio (or other) tracks. I suppose Raven could make separate lists when it first loads? Anyways, the solution you have here is fine.
| otio::Track* next_track = dynamic_cast<otio::Track*>(next_it); | ||
|
|
||
| // Only iterate over tracks of the same kind | ||
| if(next_track->kind() != selected_track_type){ |
There was a problem hiding this comment.
Do you need to check for next_track being null here?
I mad a start on tackling #73 here and I think I made some decent progress. This currently adds support for using the left and right arrow keys to move around the timeline and change the selected item. It's a little clunky when moving off the edge of the visible workarea and I'd love some input on improving that. At the moment it jumps forwards or backwards by the size of the newly selected item.
I'm not entirely sure I've implemented the keyboard control in the most elegant way or put the code in the correct place in the code base, imeadiate mode GUIs are still new to me. The other thing to note is it was necessary to disable the built in keyboard navigation to stop it conflicting
with the new controls in odd ways. Is that okay? I think the way we're using a table here is not exactly how it was intended to be used.
Fixes #73
2024-11-29.17-11-26.mp4