Differential D6110
VSE UX: F-curve drawing on Speed effect strips Authored by Peter Fog (tintwotin) on Oct 21 2019, 1:45 PM.
Details Drawing Changes:
General view of the new drawing for each speed effect mode:  Detail of the horizontal zero (blue) line(later changed to black grid color): 
Diff Detail
Event TimelineThere are a very large number of changes, so older changes are hidden. Show Older Changes Comment Actions
Sorry for the delay.
This takes a lot of time. Comment Actions As mentioned above, I've abandoned this patch, I'm out of time and motivation, so someone else will have to deal with incredible weak UI/UX. Comment Actions
Comment Actions Nice to have(but currently not implemented): Auto adjusting of endframe, when using Multiply or Boost. Comment Actions In Frame Number or Length modes the f-curve areas drawn outside of the frame are "hold"(freeze frame) areas. Since "hold" is illustrated with a darker color, these areas could be drawn like this:  As for the Stretch mode, basically two durations are needed: One for the source and one for the target. As it is now, the source and the target in and out points are confusing mess, ignoring trimmed in and out points of the source strip. A way to solve this is to use the the trimmed in and out points of the source strip as source duration and use the Speed strip as target duration, keeping the same frame for start and end of both strips. This means that the speed strip should have handles and it's durations should be adjusted independently of the source strip, however it should be anchored/parented to the source strip, so they will be moved together around in the sequencer.  Comment Actions Hi, I have talked with @Hjalti Hjálmarsson (hjalti), we both agree, that these changes in functionality look reasonable. This patch must be split into 2 parts - fcurve drawing and changes in current functionality. Also noticed, that files DNA_ipo_types.h and ipo.c are appended. These are legacy systems that should be only used for versioning for old files. If these changes are necessary, it may be good idea to do some cleanup prior to this change. Comment Actions In fact the changes to ipo are not necessary, as it is only used for versioning. Comment Actions @Germano Cavalcante (mano-wii) @Richard Antalik (ISS) Great to see this is finally moving forward. As mentioned earlier part of the confusion is due because it is not clear what in/out/durations are used to what calculations. This way the user always will know the final duration, because it's the duration of the speed strip. What do you think about this idea? Comment Actions Landed by accident. Comment Actions I'm not sure what Many tweaks to f-curve drawing code means, but I guess it is code optimizations, because the functionality seems to be the same?
 Unrelated to this patch, but related to Speed strips, there are still problems with the preview isn't updated when adding a new key. Could inserting/changing a key force a ctrl+r refresh, instead of having to manually do it? I'm not sure if you have done something about the Multiply calculations, but now it is working as expected, were positive values will play forward, zero will freeze and negative will play backwards(@Richard Antalik (ISS)):  Comment Actions I have already applied this patch so it's probably best to respond (already...). My version doesn't work for some reason(I don't see curves), but I should address concept first anyway: I probably wouldn't find this feature very useful in terms of visualizing what will be done just from looking at curve, but perhaps it's useful in terms of quicker orientation and showing complexity of animation. Comment Actions @Richard Antalik (ISS) For stretch the default speed could be a centered blue line indicating the normal speed. Like this:  (Snapping at hold is quite important, because that would mean snapping to normal speed) BLI_assert(v->speed_control_type == SEQ_SPEED_STRETCH);
const int target_strip_length = seq_effect_speed_get_strip_content_length(seq->seq1);
if ((seq->seq1->enddisp != seq->seq1->start) && (target_strip_length != 0)) {
curve_val = (float)target_strip_length /
(float)(seq->seq1->enddisp - seq->seq1->start) / 2.0f;
ymin = -1.0f;
}For testing the curve drawing: Comment Actions I still don't see any functional changes? Are there any? How do you like the Stretch drawing suggestion? As shown, it is very easy to implement. Comment Actions There were no functional changes. Sounds good, but I have to look deeper into the current solution. Comment Actions I'll check again when I'm at my computer again. Could a test build be made of this, so more users could test it? [EDIT: It is working as advertised here, I don't know what caused the fallout] Comment Actions Sorry, I must have been doing something wrong, I have re-checked and I see curves now. I see they are normalized too. I would probably suggest using different color for 0 line. I find it distracting when it is same as current frame. Personally I would go for 1px thin darker line in same hue as strip body. Or not draw it at all if it is still distracting. Comment Actions @Richard Antalik (ISS) The horizontal line is drawn in consistency with the horizontal line in the graph editor(for quick check see the gif above). Comment Actions That line in graph editor is not line for 0 value, it is cursor position line, similar to one for time. You can move it with shift+right click. Comment Actions Nb. 1px lines are supposedly invisible on Macs.  Not drawing the line at zero will make it impossible to see when ex. the strip is playing the frame order in reverse, so imo it is pretty crucial to make it visible. Maybe the color of the x-axis in the 3D View can be used?  Comment Actions That can't be correct. Yes, something like this is what I would suggest. It doesn't distract, but you see it quite well. I think this would be also quite distracting because of contrasting colors. Still better than color of playhead though. Comment Actions
I hope you read the text too? This line is already existing. If it is implemented on all strips, you'll basically have two lines on strips looking like that if f-curves are drawing - unless the zero line isn't visible, and users may think that the separator line is the zero line. Comment Actions It's always good to improve the design, but I'm not sure what changes have been decided here. So, if anyone is interested in moving things forward, feel free to command and edit the patch ;) Comment Actions
 Comment Actions LGTM. But it is almost certain that the penalty in performance is negligible. And this is the same drawing solution seen for other strip types. Comment Actions LazyDodo did a build with this patch for you guys and users to test. It's D6110 and can be downloaded here: https://builder.blender.org/download/patch/ (Switch between the different speed control modes, after selecting the speed strip, to see the various f-curve drawings) Unrelated to this patch is there something broken with Multiply mode, where the last frame gets "stuck": https://developer.blender.org/T90920 Comment Actions Looks better, but even better would be if it used color of strip body with lower intensity, so with custom themes this won't look out-of place.
I still think it would be better to just print speed factor exactly. I would only draw f-curves really. Above are my only issues as far as usability goes. That said, I would appreciate third opinion on these.
Comment Actions Maybe you could make a mock-up of how visible you want it?
Can you elaborate on why you want to draw the speed factor on the strip, if you want it exposed then is should be in the sidebar? I don't understand the include comment, and I don't think it is something I've touched, maybe @Germano Cavalcante (mano-wii) can help out with this? Comment Actions Sure, let's say you have sped up strip where you used stretch mode. Now you want composite another strip, but you must syncronize playback rate.  Now which one of these visualizations does show you more information? To me this looks that this is really visualization for sake of having visualization. In addition you may have strips that look like they are muted/hidden, while they are not.
Now seq_effect_speed_get_strip_content_length is intended to be only utility function for internal sequencer logic. Drawing should not directly use internal logic of any editor. If this function has to be exposed to outside world, I would perhaps change it a bit. Not sure if I would change only it's argument to be parent effect or perhaps try to keep math inside of sequencer and only provide value for drawing, because drawing code should not contain much of internal logic. My point with inline mainly was that this is not allowed unless absolutely necessary in general.
Comment Actions Add: The requested line color. When in stretch, and users add a Speed strip, the line is at normal speed. When users then change the duration of the source strip ex. shortening it, they will see that the curve is moving above the normal speed line, indicating that the strip will now play faster, same thing when they stretch out the duration, then the curve will go below the line indicating that the speed will be slower than normal speed. I think this is the main use of it - making it more intuitive what will happen.  It seems to me, that what you're after is something else, which will need to expose the "Target Frame" value at the current frame, but afaik this is not available to expose in the sidebar(where it belongs, imo). In addition to this, you'll need to be able to change this value and have it linked to source strip duration to be able to reuse values from one strip to another. An easier way to solve this would be to use the same value in Multiplier mode for both strips. On the f-curves looking like hold areas, I already tried to address this in the vse chat with a proposition to change the visual identity of hold areas:  | ||||||||||||||||