timeline: Only draw InvisibleButton if size greater than zero #87
Conversation
timeline.cpp
Outdated
| // See definition of ImGui::InvisibleButton in imgui_widgets.cpp: | ||
| if (size.x <= 0.0 || size.y <= 0.0) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
This check could move up to line 380 next to label_visible
|
Do any of the other InvisibleButtons cause problems when they are too small? In your use case is it because the timeline is zoomed out so far, or is the clip with the effect actually zero duration? |
|
I think you're right actually yes. On line 362 we use |
|
In OTIO it is valid to have a clip with zero duration. Sometimes that's meaningful when it occurs before/after a transition. So that case will come up in practice. Does Raven need to draw it? No. But It would be nice if Raven did let you find those and inspect them. We don't yet have #73 so there isn't a way to select something with zero duration at the moment. I'm open to suggestions about how to do that - but your fix to avoid rendering something with zero pixel width seems totally fine. |
InvisibleButton size must be greater than zero in both x and y so only draw Effect button if this is the case. Signed-off-by: Thomas Wilshaw <thomaswilshaw@gmail.com>
Only draw Items, Effects and Transitions if their durations are greater than zero. Signed-off-by: Thomas Wilshaw <thomaswilshaw@gmail.com>
ThomasWilshaw
force-pushed
the
invisible_button
branch
from
c464d91 to
f1b0e0c
Compare
|
Updated based on your feedback, should I include markers too? I know they often have a duration of zero and their width is already based of off the track height |
|
If the markers also cause a crash we should fix that too. The rest looks great. |
|
i assume markers of zero length will be drawn as ??? a thick line ???, not skipped? |
|
@meshula , I had misunderstood how typical markers are stored, I thought they would have a duration of one frame but I now realise they have a duration of zero |
|
Hi, sorry to bump this, I just wanted to check if you are waiting on changes from me with? |
|
@ThomasWilshaw I was hoping the duration test would allow drawing items of zero length, and also that the width would be clamped to a minimum size to avoid drawing zero area markers (maybe that's already in place, I'm not sure). |
|
@meshula I possibly misunderstanding what you mean but how would you display something on the timeline if it's duration was zero? I've only come across it in one file so far but is it actually fairly common? When you say a zero area marker do you mean one that only marks a single frame rather than a range of frames? |
|
Markers have a duration, and a marked frame may have a duration of one frame as you say. That's not an issue, that works with the code already. However, markers may be instantaneous with a zero duration, and appear at any time, not necessarily aligned on a frame. A typical method of displaying zero length markers is to use a hair line of a certain number of pixels centered on the time. Typically a visual affordance can be provided, such as an inverted triangle or a circle filled with the marker's color, atop the hair line, giving the appearance of a traffic sign or lollipop in the time line, and also providing an easy handle for hovering, selection, and dragging. Adding a "lollipop" and interaction affordances is extra candy though. I'm not proposing that you take that part on, just proposing that you draw a hair line with a small bounding box for hovering and selection. |
|
I think all that is already supported. Here we have a selected marker (the white one) that has a duration of zero and starts on an integer frame: Here we have a multi frame marker that starts part way through a frame (again in white as it's selected): Single marker that starts part way through a frame (although that's not reflected in the inspector so I'm displaying the JSON): |
|
Got it, I thought this change was going to not render those because of the zero length. |
InvisibleButton size must be greater than zero in both x and y so only draw Effect button if this is the case. As discussed in #85
I now notice there are several other
InvisibleButtons intimeline, is it worth adding the same check to all of them or is there a more elegant approach?