Page MenuHome

Fix T49658: animation of recursive scenes in VSE
Needs ReviewPublic

Authored by Alberto Mardegan (mardy) on Mar 12 2017, 10:38 PM.

Details

Summary

This change adds a new checkbox in scene strips which, when checked, properly shift the timing of the animations living in the scene.

The rason why we are not fixing the timings for all scene strips is that fixing this bug might break .blend files which relied on the broken behaviour (see discussion in T49261 as well).

Diff Detail

Repository
rB Blender

Event Timeline

Alberto Mardegan (mardy) retitled this revision from to WIP: Fix T49658: animation of recursive scenes in VSE.Mar 12 2017, 10:38 PM
Alberto Mardegan (mardy) updated this object.
Alberto Mardegan (mardy) set the repository for this revision to rB Blender.
Alberto Mardegan (mardy) updated this revision to Diff 8415.

Sorry it's taken me so long to chip in... this seems to work fine, both on the example provided and on my ones from T49658. It also looks to be a little more CPU-efficient than my attempt and it certainly touches less files.

It would be really nice to fix this defect without having to do all the for-loops every frame... what thoughts of caching the relationships once found for a given strip and then using the cache for subsequent frames?

It would be really nice to fix this defect without having to do all the for-loops every frame... what thoughts of caching the relationships once found for a given strip and then using the cache for subsequent frames?

Thanks for the review! OK, it makes sense to add a cache of subscenes: I'll try to work on that.

I happen to have a question as well: do we really need to animate all scenes? If -- as it seems reasonable to me -- it's enough to animate only those scenes used in the current scene's sequencer, then we can get rid of the unprocessed_scenes list.

Alberto Mardegan (mardy) retitled this revision from WIP: Fix T49658: animation of recursive scenes in VSE to Fix T49658: animation of recursive scenes in VSE.Apr 11 2017, 5:33 PM
Alberto Mardegan (mardy) edited the summary of this revision. (Show Details)
Alberto Mardegan (mardy) updated this revision to Diff 8604.

I've added the missing UI option to toggle whether the user wants this fix or not.

@Alberto Mardegan (mardy) Hi, sorry for the big pause! 3 things:

  1. This doesn't apply against the current master
  2. When you change the "shift all timings" option, you need to invalidate the cache so that the viewport updates
  3. I think the option should default to true for new files, since shifting the timing is really the "correct" behaviour - but be false on old files with scenes-within-scenes so as to not change those files' behaviour on first load.

Thoughts? Is it okay if I change the diff so it applies against current master?

Thanks!

Hi Olly! You are very welcome to fix/improve my work, because in this period I don't have time to do it myself. :-)

I don't seem to be able to update this diff directly (I suppose that's reasonable) so here's the latest diff. I've taken the liberty of changing the wording a little bit, and had a go at improving the behaviour when you check / uncheck the box in terms of forcing a redraw, but I still can't make it refresh the preview window for the frame that is currently displayed.

The option is now set by default for new scene strips.

Hi Olly, I 'd suggest you to open a new merge proposal with your patch, post the link to it in this merge proposal, and then close this one. So it will be easier to review.