Fix #110053: Crash on linked animation data with drivers #110795

Christoph Lendenfeld · 2023-08-04T14:51:10+02:00

2023-08-04 14:51:10 +02:00

The setup is the following
3 objects are in a source file and their collection gets linked into another file (A, B and C)
2 of those objects get a library override on their translations (A and B)
1 channel of those overrides gets a driver assigned (A) that is driven by C

(Download the files for easy testing, they are the same as on the bugreport)

Now when you link (CTRL+L) the animation data from A onto B they both appear to have the driver.
It did create a crash though when moving C because AnimData was a nullptr

The reason it crashed is because there was no AnimData on the object because this line
DEG_id_tag_update(&ob_dst->ID_RECALC_TRANSFORM | ID_RECALC_GEOMETRY | ID_RECALC_ANIMATION);
was never called.

I moved the line before the check if (ob_dst->data && ob_src->data) to make it work

The setup is the following 3 objects are in a source file and their collection gets linked into another file (A, B and C) 2 of those objects get a library override on their translations (A and B) 1 channel of those overrides gets a driver assigned (A) that is driven by C (Download the files for easy testing, they are the same as on the bugreport) Now when you link (CTRL+L) the animation data from A onto B they both appear to have the driver. It did create a crash though when moving C because `AnimData` was a `nullptr` The reason it crashed is because there was no `AnimData` on the object because this line `DEG_id_tag_update(&ob_dst->ID_RECALC_TRANSFORM | ID_RECALC_GEOMETRY | ID_RECALC_ANIMATION);` was never called. I moved the line before the check `if (ob_dst->data && ob_src->data)` to make it work

linking.blend

862 KiB

source.blend

892 KiB

Christoph Lendenfeld added the

Module

Animation & Rigging

label 2023-08-04 14:51:10 +02:00

Christoph Lendenfeld added 1 commit 2023-08-04 14:51:20 +02:00

the fix 9616451c2d

Christoph Lendenfeld changed title from ~~Fix #110053: Crash on linked animation data with drivers~~ to WIP: Fix #110053: Crash on linked animation data with drivers

2023-08-04 14:53:18 +02:00

Christoph Lendenfeld added 1 commit 2023-08-11 16:47:09 +02:00

Merge branch 'main' into fix_linked_driver_crash 6339ca0319

Christoph Lendenfeld commented

2023-08-11 17:34:26 +02:00

It seems like while during OBJECT_OT_make_links_data the object has AnimData (adt)
During BKE_animsys_eval_driver it is a nullptr
Not sure why

It seems like while during `OBJECT_OT_make_links_data` the object has `AnimData` (adt) During `BKE_animsys_eval_driver` it is a `nullptr` Not sure why

Christoph Lendenfeld added 2 commits 2023-08-17 16:22:49 +02:00

Merge branch 'main' into fix_linked_driver_crash 0e09efcfa3

remove old if check 8f2e879a98

Christoph Lendenfeld changed title from ~~WIP: Fix #110053: Crash on linked animation data with drivers~~ to Fix #110053: Crash on linked animation data with drivers

2023-08-17 16:22:55 +02:00

Christoph Lendenfeld requested review from Brecht Van Lommel 2023-08-17 16:26:19 +02:00

Christoph Lendenfeld requested review from Bastien Montagne 2023-08-17 16:26:27 +02:00

Brecht Van Lommel requested changes 2023-08-17 16:37:51 +02:00

Brecht Van Lommel left a comment

From what I can tell this make it so the linked datablock is now edited, but that's not allowed.

I think the correct fix is to restructure the code so that DEG_id_tag_update is called?

From what I can tell this make it so the linked datablock is now edited, but that's not allowed. I think the correct fix is to restructure the code so that `DEG_id_tag_update` is called?

Christoph Lendenfeld added 1 commit 2023-08-17 16:51:34 +02:00

move DEG_id update before if check 88d1627489

Christoph Lendenfeld commented

2023-08-17 16:52:47 +02:00

thanks for the review
changed it so the DEG_id_tag_update happens before the if (ob_dst->data && ob_src->data)

thanks for the review changed it so the `DEG_id_tag_update` happens before the `if (ob_dst->data && ob_src->data)`

Christoph Lendenfeld requested review from Brecht Van Lommel 2023-08-17 16:53:00 +02:00

Brecht Van Lommel approved these changes 2023-08-17 16:56:00 +02:00

Bastien Montagne requested changes 2023-08-17 17:31:09 +02:00

Bastien Montagne left a comment

I'd rather restructure the code that way:

            if (ob_dst->data && ob_src->data) {
              if (BKE_id_is_editable(bmain, obdata_id)) {
                BKE_animdata_copy_id(bmain, (ID *)ob_dst->data, (ID *)ob_src->data, 0);
              }
              else {
                is_lib = true;
              }
            }
            DEG_id_tag_update(&ob_dst->id,
                              ID_RECALC_TRANSFORM | ID_RECALC_GEOMETRY | ID_RECALC_ANIMATION);

... such that it remains clear that the call to DEG_id_tag_update is last, after all editing changes have been performed.

Not that proposed fix here is wrong, but I find it better from a logical and readability PoV. It makes the code easier to follow and safer to modify in the future.

I'd rather restructure the code that way: ``` if (ob_dst->data && ob_src->data) { if (BKE_id_is_editable(bmain, obdata_id)) { BKE_animdata_copy_id(bmain, (ID *)ob_dst->data, (ID *)ob_src->data, 0); } else { is_lib = true; } } DEG_id_tag_update(&ob_dst->id, ID_RECALC_TRANSFORM | ID_RECALC_GEOMETRY | ID_RECALC_ANIMATION); ``` ... such that it remains clear that the call to `DEG_id_tag_update` is last, after all editing changes have been performed. Not that proposed fix here is wrong, but I find it better from a logical and readability PoV. It makes the code easier to follow and safer to modify in the future.

Christoph Lendenfeld added 1 commit 2023-08-17 17:52:55 +02:00

implement review comment d37c66773e

Christoph Lendenfeld commented

2023-08-17 17:55:50 +02:00

agree that looks a lot less confusing

Christoph Lendenfeld requested review from Bastien Montagne 2023-08-17 17:55:57 +02:00

Bastien Montagne approved these changes 2023-08-17 18:49:16 +02:00

Christoph Lendenfeld merged commit e42c70fd31 into main

2023-08-18 09:18:02 +02:00

Christoph Lendenfeld deleted branch fix_linked_driver_crash

2023-08-18 09:18:04 +02:00

Christoph Lendenfeld referenced this issue from a commit