Differential D11370

Fix T88263: Incorrect image offset from old file
ClosedPublic
Actions

Authored by Richard Antalik (ISS) on May 24 2021, 7:28 AM.

Details

Reviewers

Sergey Sharybin (sergey)

Maniphest Tasks

T88263: After updated, offset broken in video editing

Commits

rB4891da8ae225: Fix T88263: Incorrect image offset from old file

Summary

Versioning code for converting strip offset property doesn't work, when
property was animated and disabled or when crop was used.

When offset property is animated and offset is enabled, animation is
converted to be used with new transform design. When offset is disabled,
animation is left untouched. New transform design doesn't have option
to disable offset, and therefore old unconverted animation is used
instead of converted static value.

Remove animation from propery if it was unused.

Another issue was that both X and Y offset animation was being corrected
by factor caluclated for X channel.

Diff Detail

Repository: rB Blender

Event Timeline

Richard Antalik (ISS) requested review of this revision.May 24 2021, 7:28 AM

Richard Antalik (ISS) created this revision.

Harbormaster completed remote builds in B14723: Diff 37412.May 24 2021, 7:28 AM

Sergey Sharybin (sergey) added inline comments.May 26 2021, 10:17 AM

source/blender/blenloader/intern/versioning_290.c
135	Maybe worth stating why only do it if the crop is not used. Maybe something like: /* Convert offset animation. * * Note that offset was only taken into account when crop is not used. * So if there is no crop we convert f-curve and otherwise remove the * f-curve so that it does not affect the sequence. */
149–151	Shall we reset the offset stored in the strip somehow? Thing is, f-curve will write new values to the sequence. So removing the f-curve might not be enough to get rid of unwanted offset.

Address inlines, bit of cleanup.

Harbormaster completed remote builds in B14824: Diff 37599.May 28 2021, 12:58 PM

@Sergey Sharybin (sergey) would you consider this patch as a fix for 2.93? I tried to ask for confirmation of fix in T88263 by providing build with this patch, because he may still encounter some things that would be easy to fix, but this may take time to communicate back and forth.

source/blender/blenloader/intern/versioning_290.c
149–151	With offset unused, values are reset to 0. I think it would make sense to reset values even if it's due to crop usage.

Sergey Sharybin (sergey) accepted this revision.May 28 2021, 1:02 PM

This revision is now accepted and ready to land.May 28 2021, 1:02 PM

It doesn't seem to be a regression in 2.93 (the report mentions 2.92 is already broken). Think it is safer to not rush such fix, and let it be handled as part of LTS maintenance after the patch had some time to be tested.

In D11370#292196, @Sergey Sharybin (sergey) wrote:

It doesn't seem to be a regression in 2.93 (the report mentions 2.92 is already broken). Think it is safer to not rush such fix, and let it be handled as part of LTS maintenance after the patch had some time to be tested.

Agree, so I will wait for positive confirmation and commit after that, so this fix won't be broken up into more

Update fix after confirmation by user

Harbormaster completed remote builds in B15097: Diff 38136.Jun 10 2021, 1:38 PM

Richard Antalik (ISS) edited the summary of this revision. (Show Details)Jun 10 2021, 1:40 PM

Closed by commit rB4891da8ae225: Fix T88263: Incorrect image offset from old file (authored by Richard Antalik (ISS)). · Explain WhyJun 16 2021, 12:46 AM

This revision was automatically updated to reflect the committed changes.

Richard Antalik (ISS) added a commit: rB4891da8ae225: Fix T88263: Incorrect image offset from old file.