Page MenuHome

Correct timing of animations in sequencer scene strips (fixes T49261 / T49658)
Needs RevisionPublic

Authored by Olly Funkster (Funkster) on Dec 13 2018, 2:59 PM.

Details

Summary

Use correct timing offsets for animations & keyframes in scene strips in the sequencer that are not placed at frame 1. See T49261 and T49658.

Based on the patch from @Alberto Mardegan (mardy) (D2558), I have updated it to apply against current master, and set the option by default when adding a new scene strip so that the correct behaviour is used without the user needing to tick any boxes (but still maintaining compatibility with old files).

Diff Detail

Repository
rB Blender

Event Timeline

This comes handy!

Question is if it wouldn't be better to do timing conversion in do_versions and not add "Correct Timing" toggle to UI.
Would make UI and code a bit cleaner.

Is there any sensible reason to have such toggle?

Question is if it wouldn't be better to do timing conversion in do_versions and not add "Correct Timing" toggle to UI.
Would make UI and code a bit cleaner.
Is there any sensible reason to have such toggle?

I can't think of any reason to have the toggle - the option was added by Alberto, presumably at someone else's request. I think we should use the correct behaviour in all cases, since it was completely unusable before I can't think that anyone would have sub-scenes with animation in any position other than at frame 1.

Still, if it really mustn't change the behaviour of old files, having it done in versions and not in the UI is preferable. The checkbox is clutter and could be very confusing if accidentally un-ticked.

I was not aware of the versions stuff, and indeed that looks like a much better option than having a UI option.

btw @Alberto Mardegan (mardy) I wasn't having a go at you for adding the option, apologies if that was rude. Preserving behaviour of existing files makes sense in a lot of cases, but not here in my opinion :)

btw @Alberto Mardegan (mardy) I wasn't having a go at you for adding the option, apologies if that was rude.

It didn't sound rude at all, no worries :-)

Preserving behaviour of existing files makes sense in a lot of cases, but not here in my opinion :)

I think we should always strive for that: you can never know whether some user already worked around the issue in some other means, and if we fix it unconditionally we'll most likely break his files. I think deciding to apply the fix based on the project's version is still the best idea; then of course it might not be worth the effort, but I think we should at least investigate the possibility.

Campbell Barton (campbellbarton) requested changes to this revision.Jan 7 2019, 12:42 AM
Campbell Barton (campbellbarton) added inline comments.
source/blender/blenkernel/intern/anim_sys.c
2961–2966

Instead of using this a linked lists See: IS_TAGGED, LIB_TAG_DOIT.

source/blender/makesrna/intern/rna_sequencer.c
1990

Would flip this value use_legacy_timing, see: RNA_def_property_boolean_negative_sdna

This revision now requires changes to proceed.Jan 7 2019, 12:42 AM
release/scripts/startup/bl_ui/space_sequencer.py
982–983

Is this really required? Why would one disable option called "Correct"?
Can we always make sequencer correct? If not, is there a more descriptive name, which will be more suggestive about what's the difference in behavior?

source/blender/blenkernel/intern/anim_sys.c
2957–2960

The comment should be explaining why something is done, not how. From quick reading i am still not sure why would order be important here.

And if it is, isn't it something to be moved to a dependency graph?