Details

Reviewers

Richard Antalik (ISS)

Commits

rBf013e3de81da: VSE: Speed Effect layout updates

Summary

Patch taken from D6110: VSE UX: F-curve drawing on Speed effect strips (original from @Peter Fog (tintwotin))

The Speed Effect panel is confusing and non-intuitive because:

the "Stretch to input strip length" option, when enabled, hides all other options.
"Speed factor" and "Frame Number" share the same value. However one is a factor and the other is an integer.
"Scale to length" doesn't have much to do with the "Frame Number".

This patch proposes to simplify it, by adding an enum corresponding to different configurations.

Changes:

The new enums correspond to 4 modes: Stretch, Multiply, Frame Number and Length.
The "Multiply Factor" has been removed;
Value corresponding to "use as speed" enabled is now the value appended to the Multiply enum;
Value corresponding to "use as speed" disabled is now the value appended to the Frame Number enum;
Value corresponding to "Scale to Length" enabled is now the value appended to the Length enum;
Except Stretch each mode has now its respective control values.

Diff Detail

Repository: rB Blender

Event Timeline

Germano Cavalcante (mano-wii) requested review of this revision.Jul 8 2021, 8:10 PM

Germano Cavalcante (mano-wii) created this revision.

Harbormaster completed remote builds in B15702: Diff 39319.Jul 8 2021, 8:10 PM

Germano Cavalcante (mano-wii) mentioned this in D6110: VSE UX: F-curve drawing on Speed effect strips.Jul 8 2021, 8:14 PM

Thanks for splitting this patch and providing brief description.

the "Stretch to input strip length" option, when enabled, hides all other options except "Multiply Speed" which has no effect.

This is not true, multiply_speed has influence. Even in this patch when you switch to Multiply mode, set Boost to 2, then switch back to Stretch mode it will have influence, but it is not editable. So I think, that this property should be always editable.

I really don't like property name "Boost". I would suggest to change property name to "Multiply" or "Multiply Frame", and change docstring ot "Multiply the resulting frame" since that is what the property does. Probably best is to handle further changes of this property in separate patch.

Versioning code tested - seems OK, but we may want to preserve f-curves here as it is relatively simple to implement.

As far as functionality goes, I don't think I have any more concerns.

source/blender/makesrna/intern/rna_sequencer.c
2781	`{SEQ_SPEED_STRETCH, "STRETCH", 0, "Stretch", "Adjust input playback speed, so its duration fits strip length"},` Not sure if I can phrase this better, or whether it is clear enough.
2782	shouldn't use fullstop
2788	shouldn't use fullstop
2796	shouldn't use fullstop
2826–2828	This change is unrelated.
source/blender/sequencer/intern/effects.c
3150	I would say that it seems 4th mode is missing, but what does this even do? Seems like unused function really.
3155–3156	Either remove line or revert change.

This revision now requires changes to proceed.Jul 9 2021, 8:53 AM

In D11856#305500, @Richard Antalik (ISS) wrote:

the "Stretch to input strip length" option, when enabled, hides all other options except "Multiply Speed" which has no effect.

This is not true, multiply_speed has influence. Even in this patch when you switch to Multiply mode, set Boost to 2, then switch back to Stretch mode it will have influence, but it is not editable. So I think, that this property should be always editable.

Question is maybe rather if it makes sense to Stretch and then be able to Boost? Imo, it doesn't, stretching means stretching between two points, and when boosting this, it's not stretching anymore. On top of this, there is a Multiply Speed Control mode, which does what Boost/Multiply does.

I really don't like property name "Boost". I would suggest to change property name to "Multiply" or "Multiply Frame", and change docstring ot "Multiply the resulting frame" since that is what the property does.

The word Multiply is already used as a Speed Control mode(which multiplies the frame), and it would make it confusing to have a Multiply mode and an additional Multiply value. The word "Frame" doesn't really have any significant meaning in the panel (except for Frame Number), because all of the setting are affecting the frames, making it redundant(including Frame Interpolation).

The Boost value doesn't seem to be clear how/if it actually affects animated values, since it doesn't affect the f-curve. As a "master" speed control, I would expect it push the entire animated curve up or down in the graph editor(and on the strips). So, imo, in many ways is the Boost value is redundant, adds to the confusion and could be removed entirely.

If it is decided to keep it, Boost was once called Global Speed: https://developer.blender.org/rBSc5157cda88938177685225bc8f264a4a46883250

In D11856#305523, @Peter Fog (tintwotin) wrote:

The Boost value doesn't seem to be clear how/if it actually affects animated values, since it doesn't affect the f-curve. As a "master" speed control, I would expect it push the entire animated curve up or down in the graph editor(and on the strips). So, imo, in many ways is the Boost value is redundant, adds to the confusion and could be removed entirely.

I agree that it should be removed. I was commenting on implementation in this patch. Not sure if removing it in this patch is best approach, but I don't see why not.

Remove "Multiply Speed"("globalSpeed") and do versioning
Change "speed_length" from percentage to factor (simplifies versioning)
Update fcurves in versioning
Remove unused store_icu_yrange callback
Improve/fix documentation

Harbormaster completed remote builds in B15737: Diff 39371.Jul 9 2021, 5:15 PM

Germano Cavalcante (mano-wii) added inline comments.Jul 9 2021, 5:16 PM

source/blender/sequencer/intern/effects.c
3150	In fact this callback is not used. Removed now, but it could be a separate change.

Germano Cavalcante (mano-wii) edited the summary of this revision. (Show Details)Jul 9 2021, 5:17 PM

Germano Cavalcante (mano-wii) edited the summary of this revision. (Show Details)

Germano Cavalcante (mano-wii) edited the summary of this revision. (Show Details)Jul 9 2021, 5:23 PM

missed fullstop

Harbormaster completed remote builds in B15738: Diff 39373.Jul 9 2021, 5:33 PM

Versioning: Fallback to Multiply when "Stretch to input strip length" + "Multiply Factor"

Now it's ready for review

Harbormaster completed remote builds in B15741: Diff 39377.Jul 9 2021, 5:57 PM

In Multiply, the default value of speed factor should to be 1(normal speed).
The current default is 0(freeze on the first frame).

In Length, the value has been changed from percentage to fraction.
Imo, it is much less clear what the value means when it is a fraction:

In percentage a Length of "100%" means the last frame.
In fraction a Length of "1" is the last frame...?

Maybe "Various speed control methods" would be more precise as "Speed control method"?

Move control values to the SpeedControlVars struct and add defaults
Use percentage for speed_length
Improve speed_control documentation

Harbormaster completed remote builds in B15752: Diff 39417.Jul 12 2021, 2:32 PM

The default value of Length is set to 100%, this results in a black frame for me, maybe 0% is better as default value?

Init Length and Frame Number with zero

Harbormaster completed remote builds in B15760: Diff 39428.Jul 12 2021, 4:58 PM

In D11856#306103, @Peter Fog (tintwotin) wrote:

The default value of Length is set to 100%, this results in a black frame for me, maybe 0% is better as default value?

This is probably, because last frame of source is black. Quite often also first frame of movies is black. With Raw footage this won't happen that likely. But I agree that 0% would be better.

source/blender/makesrna/intern/rna_sequencer.c
2821	Missing full stop at the end. Can't you remap 0-100 range in `seq_effect_speed_rebuild_map`? Looks rather easy to do. (just don't forget to change versoning) Even in versioning this should be easy as else if (v->flags & SEQ_SPEED_COMPRESS_IPO_Y) { globalSpeed *= 100.0f; ... }

This revision is now accepted and ready to land.Jul 12 2021, 5:34 PM

Actually I wanted to check on inline before I accept, but forgot to change status. But patch looks fine, so I would be fine even with 0-100 to 0-1 conversion in RNA...

Just wanted to be sure, that I don't request something impossible, this seems to work fine for me:

P2247 (An Untitled Masterwork)

1	diff --git a/source/blender/blenloader/intern/versioning_300.c b/source/blender/blenloader/intern/versioning_300.c
2	index a6ca64b34a0..d9e74aad9d6 100644
3	--- a/source/blender/blenloader/intern/versioning_300.c
4	+++ b/source/blender/blenloader/intern/versioning_300.c
5	@@ -119,6 +119,7 @@ static void do_versions_sequencer_speed_effect_recursive(const Scene *scene,
6	v->speed_fader = seq->speed_fader * globalSpeed;
7	}
8	else if (v->flags & SEQ_SPEED_COMPRESS_IPO_Y) {
9	+ globalSpeed *= 100.0f;
10	v->speed_control_type = SEQ_SPEED_LENGTH;
11	v->speed_fader_length = seq->speed_fader * globalSpeed;
12	substr = "speed_length";
13	diff --git a/source/blender/makesrna/intern/rna_sequencer.c b/source/blender/makesrna/intern/rna_sequencer.c
14	index 6a2ef537932..45fc77442ab 100644
15	--- a/source/blender/makesrna/intern/rna_sequencer.c
16	+++ b/source/blender/makesrna/intern/rna_sequencer.c
17	@@ -1330,20 +1330,6 @@ static float rna_Sequence_fps_get(PointerRNA *ptr)
18	return SEQ_time_sequence_get_fps(scene, seq);
19	}
20
21	-static float rna_Sequence_speed_length_get(PointerRNA *ptr)
22	-{
23	- Sequence seq = (Sequence )(ptr->data);
24	- SpeedControlVars v = (SpeedControlVars )seq->effectdata;
25	- return v->speed_fader_length * 100.0f;
26	-}
27	-
28	-static void rna_Sequence_speed_length_set(PointerRNA *ptr, float value)
29	-{
30	- Sequence seq = (Sequence )(ptr->data);
31	- SpeedControlVars v = (SpeedControlVars )seq->effectdata;
32	- v->speed_fader_length = value / 100.0f;
33	-}
34	-
35	#else
36
37	static void rna_def_strip_element(BlenderRNA *brna)
38	@@ -2838,9 +2824,6 @@ static void rna_def_speed_control(StructRNA *srna)
39	prop = RNA_def_property(srna, "speed_length", PROP_FLOAT, PROP_PERCENTAGE);
40	RNA_def_property_float_sdna(prop, NULL, "speed_fader_length");
41	RNA_def_property_ui_text(prop, "Length", "Percentage of input strip length");
42	- /* Stupid 0-1 --> 0-100 */
43	- RNA_def_property_float_funcs(
44	- prop, "rna_Sequence_speed_length_get", "rna_Sequence_speed_length_set", NULL);
45	RNA_def_property_ui_range(prop, 0.0f, 100.0f, 1, -1);
46	RNA_def_property_update(prop, NC_SCENE \| ND_SEQUENCER, "rna_Sequence_invalidate_raw_update");
47
48	diff --git a/source/blender/sequencer/intern/effects.c b/source/blender/sequencer/intern/effects.c
49	index fb614b12500..a29ab475eee 100644
50	--- a/source/blender/sequencer/intern/effects.c
51	+++ b/source/blender/sequencer/intern/effects.c
52	@@ -3263,6 +3263,7 @@ void seq_effect_speed_rebuild_map(Scene scene, Sequence seq, bool force)
53
54	if (v->speed_control_type == SEQ_SPEED_LENGTH) {
55	facf *= target_strip_length;
56	+ facf /= 100.0f;
57	}
58
59	if (facf >= target_strip_length) {

Use range 100.0 for percent in Python (RNA) and in C

Thanks @Richard Antalik (ISS) and @Peter Fog (tintwotin) for the review and the original patch :)

Good to see these improvements. It really makes the speed effect setup more intuitive.

I'll do some more tests and prepare the release note before commit. (And update the other patch later).

Feel free to comment on any other changes in the meantime.

Harbormaster completed remote builds in B15765: Diff 39433.Jul 12 2021, 6:59 PM

I noticed a problem when inserting a keyframe for Length or Frame Number (shortcut I).
In these cases the effect is not updated.
This also happens without this patch.
I'm not sure how to fix this, but it doesn't seem like something to pass up.

Same thing with clicking the dot/diamond next to the animation value in the vse sidebar to add/update the keyframe value needs ctrl+r to invalidate/update the cache or in other words to see the change when playing.

And when the first keyframe is set, you get no visual feedback when changing the value either, so you're working in the dark(especially without the curves drawn on the strips).

It is the same problem in 2.93, so it's unrelated to this patch.

In D11856#306277, @Germano Cavalcante (mano-wii) wrote:

I noticed a problem when inserting a keyframe for Length or Frame Number (shortcut I).
In these cases the effect is not updated.
This also happens without this patch.
I'm not sure how to fix this, but it doesn't seem like something to pass up.

I was looking into this, and I couldn't reproduce issue because strip have a keyframe to reproduce the bug. I think this is caused by temp cache so fix will be very unrelated, but lookin into it now.

However I found quite bad bug - speed effect properties are not saved. I thought this was due to missing subversion, but even when subversion is bumped, it doesn't work correctly.

This revision now requires changes to proceed.Jul 13 2021, 1:51 PM

Checked the issue with animated values not updating in preview:

It's not issue with temp cache as I thought. Function SEQ_relations_invalidate_cache_raw which is RNA update function calls seq_effect_speed_rebuild_map which updates "speed map". So far it looks good.
seq_effect_speed_rebuild_map however looks at values from curve, if it exists and ignores speed property value. Finally when you add keyframe, there is assumption that preview is already updated by RNA updte function so no update is needed.

Not sure if this is meant by comment

/* XXX(campbell): new in 2.5x. should we use the animation system this way?
 * The fcurve is needed because many frames need evaluating at once. */

In any case, best solution seems to be removing frameMap and doing math "on the fly". Should be no issue even for current interpolation feature.
I would rather do this in separate patch though.

I have investigated issues with versioning. After rebasing to latest master, I can't reproduce any issues, so accepting again.
Not sure what the problem was, subversion was set correctly in both cases.

This revision is now accepted and ready to land.Jul 15 2021, 6:07 PM

Richard Antalik (ISS) added inline comments.Jul 15 2021, 7:18 PM

source/blender/blenloader/intern/versioning_300.c
98	Using `const Scene` produces warning, because of `id_data_find_fcurve()` Forgot to mention..

Richard Antalik (ISS) mentioned this in D11939: VSE: Cleanup speed effect math.Jul 15 2021, 7:26 PM

Richard Antalik (ISS) added inline comments.Jul 16 2021, 12:48 AM

source/blender/makesdna/DNA_sequence_types.h
350	Another issue @Peter Fog (tintwotin) noticed is that when this value is integer, it makes frame interpolation impossible, so perhaps it is best to keep this float and in RNA too.

Silence warnings
Turn speed_frame_number a float

Harbormaster completed remote builds in B15927: Diff 39753.Jul 20 2021, 10:31 PM

Closed by commit rBf013e3de81da: VSE: Speed Effect layout updates (authored by Germano Cavalcante (mano-wii)). · Explain WhyJul 22 2021, 10:56 PM

This revision was automatically updated to reflect the committed changes.

Germano Cavalcante (mano-wii) added a commit: rBf013e3de81da: VSE: Speed Effect layout updates.

Committed.
Also added a release note --> https://wiki.blender.org/wiki/Reference/Release_Notes/3.0/VFX
Later today or tomorrow I intend to update the manual.

Thank you for your perseverance. Is the strip drawing part of the patch no go?

Peter Fog (tintwotin) mentioned this in T90920: VSE: In Speed strip's multiply mode is the last frame getting "stuck"..Aug 27 2021, 2:36 PM

VSE UX: Make Speed Effect strips more user friendly
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 39834

release/scripts/startup/bl_ui/space_sequencer.py

source/blender/blenloader/intern/versioning_300.c

source/blender/makesdna/DNA_sequence_types.h

source/blender/makesrna/intern/rna_sequencer.c

source/blender/sequencer/SEQ_effects.h

source/blender/sequencer/intern/effects.c

VSE UX: Make Speed Effect strips more user friendlyClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 39834

release/scripts/startup/bl_ui/space_sequencer.py

source/blender/blenloader/intern/versioning_300.c

source/blender/makesdna/DNA_sequence_types.h

source/blender/makesrna/intern/rna_sequencer.c

source/blender/sequencer/SEQ_effects.h

source/blender/sequencer/intern/effects.c

VSE UX: Make Speed Effect strips more user friendly
ClosedPublic
Actions

Revision Contents
Changeset List