Refactor: swap handle logic in Graph Editor transform code #121076

Christoph Lendenfeld · 2024-04-25T15:06:59+02:00

2024-04-25 15:06:59 +02:00

This PR changes the swap_handles variable of the BeztMap from short to bool.
The only reason it was a short was so that 0 could be interpreted as "not checked" within the sorting loop.
Instead, I moved the checking for swapping to a separate loop. That means the BeztMap array needs to be traversed one more time, but given sorting might already do that multiple times that won't be a performance issue (plus we don't have the if within the sorting loop potentially messing up branch prediction).

In addition to that this PR also removes prev_ipo and current_ipo from the BeztMap struct. Those were never used.

This is also partly a fix to restore 3.6 behavior. With the move to C++, swap_handles was never initalized, so the logic if (bezm->swap_handles == 0) would always be false.
That resulted in the following behavior when rotating a bunch of keys 180deg around their common center.

bug	fix

As can be seen on those pictures, the swap handles logic isn't perfect. It sometimes misses keys.
Since that is 3.6 behavior though, I think it is out of the scope of this refactor.
I suspect the issue is that handles are swapped based on their current position, which is usually clamped to the key.
So both handles have to cross the key threshold at the same time, else the clamping code will run, meaning they won't be able to cross the threshold later (due to use of < and > ).

This PR changes the `swap_handles` variable of the `BeztMap` from `short` to `bool`. The only reason it was a `short` was so that 0 could be interpreted as "not checked" within the sorting loop. Instead, I moved the checking for swapping to a separate loop. That means the `BeztMap` array needs to be traversed one more time, but given sorting might already do that multiple times that won't be a performance issue (plus we don't have the `if` within the sorting loop potentially messing up branch prediction). In addition to that this PR also removes `prev_ipo` and `current_ipo` from the `BeztMap` struct. Those were never used. This is also partly a fix to restore 3.6 behavior. With the move to C++, `swap_handles` was never initalized, so the logic ` if (bezm->swap_handles == 0)` would always be false. That resulted in the following behavior when **rotating a bunch of keys 180deg** around their common center. | bug | fix | | - | - | | ![image](/attachments/06b85d1f-cc52-4efc-9732-f2367f1aebc2) | ![image](/attachments/7e08318a-ce9d-40d8-9b94-5e37e79ff6b3) | ---- As can be seen on those pictures, the swap handles logic isn't perfect. It sometimes misses keys. Since that is 3.6 behavior though, I think it is out of the scope of this refactor. I suspect the issue is that handles are swapped based on their current position, which is usually clamped to the key. So both handles have to cross the key threshold at the same time, else the clamping code will run, meaning they won't be able to cross the threshold later (due to use of `<` and `>` ).

image.png

16 KiB

image.png

8.8 KiB

Christoph Lendenfeld added the

Module

Animation & Rigging

label 2024-04-25 15:06:59 +02:00

Christoph Lendenfeld added 3 commits 2024-04-25 15:07:09 +02:00

457bb2477e fix

a40409ef4c fix comment

09cf26f294 remove unused var

Sybren A. Stüvel approved these changes 2024-04-25 16:16:31 +02:00

Sybren A. Stüvel left a comment

Much better!

Just one note.

Much better! Just one note.

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

						
				@ -769,0 +761,4 @@

				  for (BeztMap &bezm : bezms) {

				    /* Handles are only swapped if they are both on the wrong side of the key. Otherwise the one

				     * handle out of place is just clamped at the key position later. */

				    if ((bezm.bezt->vec[0][0] > bezm.bezt->vec[1][0]) &&

Sybren A. Stüvel commented

2024-04-25 16:13:59 +02:00

This can just be:

bezm.swap_handles = (
  bezm.bezt->vec[0][0] > bezm.bezt->vec[1][0] &&
  bezm.bezt->vec[2][0] < bezm.bezt->vec[1][0]);

(but then formatted correctly ;-) )

For me this is easier to reason about, as I don't have to know the previous value for swap_handles to understand what the result of the code will be.

This can just be: ```cpp bezm.swap_handles = ( bezm.bezt->vec[0][0] > bezm.bezt->vec[1][0] && bezm.bezt->vec[2][0] < bezm.bezt->vec[1][0]); ``` (but then formatted correctly ;-) ) For me this is easier to reason about, as I don't have to know the previous value for `swap_handles` to understand what the result of the code will be.

👍 1

Christoph Lendenfeld added 1 commit 2024-04-25 16:47:17 +02:00

bf9b6700c4 remove if

Christoph Lendenfeld added 1 commit 2024-04-25 16:50:15 +02:00

3faaa86b0f remove unneeded brackets

Christoph Lendenfeld merged commit c464426475 into main

2024-04-25 16:52:27 +02:00

Christoph Lendenfeld referenced this issue from a commit

2024-04-25 16:52:28 +02:00

Refactor: swap handle logic in Graph Editor transform code

Christoph Lendenfeld deleted branch fix_swap_handles

2024-04-25 16:52:30 +02:00