Fix #110108: NLA absolute snapping #111984

Christoph Lendenfeld · 2023-09-05T16:43:32+02:00

2023-09-05 16:43:32 +02:00

Before this patch the following snapping options were broken

snap to markers
snap to seconds with "Absolute Time Snap" turned on
technically also snap to Frames with "Absolute Time Snap" turned on (but made no real difference)

The issue was that each end of the strip would snap individually. Potentially collapsing the strip.
This would happen e.g. when snapping to markers but with only 1 marker present.

This PR fixes the issue by finding out the closest snap offset and shifting the strip as a whole by that amount.
That means either the start or the end of the strip will snap to the target.
By doing it that way the length of the strip is never changed by snapping.

This means when snapping to Seconds with "Absolute Time Snap" turned on, the start of the strip might not be on a full second when the end is.

Here is a video demo as an example.

Before this patch the following snapping options were broken * snap to markers * snap to seconds with "Absolute Time Snap" turned on * technically also snap to Frames with "Absolute Time Snap" turned on (but made no real difference) The issue was that each end of the strip would snap individually. Potentially collapsing the strip. This would happen e.g. when snapping to markers but with only 1 marker present. This PR fixes the issue by finding out the closest snap offset and shifting the strip as a whole by that amount. That means either the start or the end of the strip will snap to the target. By doing it that way the length of the strip is never changed by snapping. This means when snapping to Seconds with "Absolute Time Snap" turned on, the start of the strip might not be on a full second when the end is. Here is a video demo as an example. <video src="/attachments/2c48fbe8-59ae-43c5-b2b0-cc3e8a35a034" controls></video>

nla_snap 2023-09-05 16-40-51.mp4

580 KiB

Christoph Lendenfeld added the

Module

Animation & Rigging

label 2023-09-05 16:43:32 +02:00

Christoph Lendenfeld added 6 commits 2023-09-05 16:43:45 +02:00

1f1b328971 wip: proper NLA snapping

ed6157514a Merge branch 'main' into fix_nla_marker_snap

c84d663c9e wip

ba6893f278 wips

352ff1dbd5 Merge branch 'main' into fix_nla_marker_snap

buildbot/vexp-code-patch-coordinator Build done. Details

4696880494 snap working

Christoph Lendenfeld requested review from Nate Rupsis 2023-09-05 16:43:58 +02:00

Christoph Lendenfeld requested review from Brad Clark 2023-09-05 16:45:14 +02:00

Christoph Lendenfeld commented

2023-09-05 17:07:28 +02:00

@blender-bot package

Blender Bot commented

2023-09-05 17:07:31 +02:00

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR111984) when ready.

Christoph Lendenfeld added this to the Animation & Rigging project 2023-09-05 17:21:20 +02:00

Nate Rupsis reviewed 2023-09-06 16:43:20 +02:00

source/blender/editors/transform/transform_convert_nla.cc Outdated

						
				@ -668,0 +671,4 @@

				    transform_snap_anim_flush_data(t, &td, snap_mode, &snap_value);

				    /* In order to move the strip in a block and not each end individually, find the minimal snap

				     * offset and shift the whole strip by that amount. */

Nate Rupsis commented

2023-09-06 16:43:20 +02:00

Minor: first line in comment is a little long.

/* In order to move the strip in a block and not each end individually,
 * find the minimal snap offset and shift the whole strip by that amount. */

Might read better.

Minor: first line in comment is a little long. ``` /* In order to move the strip in a block and not each end individually, * find the minimal snap offset and shift the whole strip by that amount. */ ``` Might read better.

Nate Rupsis reviewed 2023-09-06 16:44:04 +02:00

source/blender/editors/transform/transform_convert_nla.cc

						
				@ -668,0 +681,4 @@

				  for (int i = 0; i < tc->data_len; i++) {

				    TransData td = tc->data[i];

				    *td.loc = td.iloc[0] + offset;

Nate Rupsis commented

2023-09-06 16:44:04 +02:00

is there a reason we update *td.loc in it's own loop? Couldn't that be done at the bottom of the other loop?

is there a reason we update `*td.loc` in it's own loop? Couldn't that be done at the bottom of the other loop?

Christoph Lendenfeld commented

2023-09-08 15:27:04 +02:00

I need to first find the smallest snap delta of all points, then I can apply that offset to all points.
That gives me the behavior of snapping the whole block to the closest snapping point of either side.

I renamed the variables and added a comment to make that clearer

I need to first find the smallest snap delta of all points, then I can apply that offset to all points. That gives me the behavior of snapping the whole block to the closest snapping point of either side. I renamed the variables and added a comment to make that clearer

Germano Cavalcante commented

2023-09-06 16:52:43 +02:00

I didn't see it in detail, but if it doesn't change the behavior too much, could you use the generic snap solution?
See transform_snap_mixed_apply.
You would only need to edit the callbacks:
t->tsnap.snap_target_fn
t->tsnap.snap_source_fn
t->tsnap.snap_apply_fn

I didn't see it in detail, but if it doesn't change the behavior too much, could you use the generic snap solution? See `transform_snap_mixed_apply`. You would only need to edit the callbacks: `t->tsnap.snap_target_fn` `t->tsnap.snap_source_fn` `t->tsnap.snap_apply_fn`

Brad Clark approved these changes 2023-09-06 17:14:36 +02:00

Brad Clark left a comment

The fix to snap the entire clip is what I was expecting it to do and so while there are other issues to address in other patches (I was reminded) I am happy to see this current result.

❤️ 1

Christoph Lendenfeld commented

2023-09-08 15:16:42 +02:00

I didn't see it in detail, but if it doesn't change the behavior too much, could you use the generic snap solution?
See transform_snap_mixed_apply.
You would only need to edit the callbacks:
t->tsnap.snap_target_fn
t->tsnap.snap_source_fn
t->tsnap.snap_apply_fn

Thanks for the hint that those exist. I am not too familiar with the snapping system yet but will definitely have a look at those
I would keep that for a different patch though, that way this PR can be backported to the LTS tasks more easily.
But yeah upgrading to generic snapping is definitely the way to go.

> I didn't see it in detail, but if it doesn't change the behavior too much, could you use the generic snap solution? > See `transform_snap_mixed_apply`. > You would only need to edit the callbacks: > `t->tsnap.snap_target_fn` > `t->tsnap.snap_source_fn` > `t->tsnap.snap_apply_fn` Thanks for the hint that those exist. I am not too familiar with the snapping system yet but will definitely have a look at those I would keep that for a different patch though, that way this PR can be backported to the LTS tasks more easily. But yeah upgrading to generic snapping is definitely the way to go.

Christoph Lendenfeld added 3 commits 2023-09-08 15:26:00 +02:00

3738321aef Merge branch 'main' into fix_nla_marker_snap

f9fc90ed3f Merge branch 'main' into fix_nla_marker_snap

a5f3817961 add comment

Nate Rupsis approved these changes 2023-09-14 22:05:38 +02:00

Christoph Lendenfeld merged commit bd305c8d18 into main

2023-09-15 09:52:48 +02:00

Christoph Lendenfeld deleted branch fix_nla_marker_snap

2023-09-15 09:52:49 +02:00

Christoph Lendenfeld referenced this issue from a commit

2023-09-15 09:52:50 +02:00

Fix #110108: NLA absolute snapping

Christoph Lendenfeld removed this from the Animation & Rigging project 2023-11-23 10:54:03 +01:00

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Fix #110108: NLA absolute snapping #111984