Page MenuHome

Fix T65175: Animation Data on lamp stays linked even after unlinking all its data
ClosedPublic

Authored by Philipp Oeser (lichtwerk) on May 27 2019, 1:31 PM.

Diff Detail

Repository
rB Blender

Event Timeline

Harbormaster completed remote builds in B3742: Diff 15627.

Any reason this is not using BKE_animdata_copy_id_action like other places using the USER_DUP_ACT flag?

Any reason this is not using BKE_animdata_copy_id_action like other places using the USER_DUP_ACT flag?

hm, this was already called in fact later here (but doesnt seem to give expected results...)
my attempt was actually copied from rBa75ac18638f4...

So that could mean:

  • there is something wrong with BKE_animdata_copy_id_action (in this particular case)
  • I need more digging to find out why it doesnt give expected results

also: I am a bit confused why [but that might be a different story...]

checking further...

Bastien Montagne (mont29) requested changes to this revision.May 27 2019, 2:36 PM

This patch is not correct at the very least because it's not setting light_new->id.new pointer properly (what ID_NEW_SET macro is doing).

But beside that, this issue affects all obdata (all the cases in that switch), not only lights.

In fact, ideally, I would rather refactor a bit that whole thing and use BKE_id_copy_ex() (with the right set of flags) everywhere… Better not do that now though, so just dos as @Brecht Van Lommel (brecht) suggested, add this after the whole switch should fix it all at once:

if (dupflag & USER_DUP_ACT) {
  BKE_animdata_copy_id_action(bmain, (ID *)obn->data, true);
}
This revision now requires changes to proceed.May 27 2019, 2:36 PM

Grmll, you updated in the mean time, cancel my comment then… ;)

I guess BKE_animdata_copy_id_action could use ntreeFromID and copy any actions in that node tree as well.

I guess BKE_animdata_copy_id_action could use ntreeFromID and copy any actions in that node tree as well.

@Bastien Montagne (mont29): does this fit your plans for refactor?

@Philipp Oeser (lichtwerk) the double materials handling is necessary because mats can be stored in two places, Object itself, and/or its ObData (mesh, curve etc.)…

regarding refactor, would not do it in fact, that objetc duplication uses very specific code paths… generic lib code is not directly usable here I think.

And yes, @Brecht Van Lommel (brecht) suggestion looks good, that way it will also handle material nodes anim (which are most likely broken the same way light ones are currently).

You'll have to carefully check all usages of BKE_animdata_copy_id_action then, though, since in a few places that issue was already 'addressed' (see e.g. if (man->nodetree != NULL) { BKE_animdata_copy_id_action(bmain, &man->nodetree->id, false); } in single_mat_users()…).

Philipp Oeser (lichtwerk) updated this revision to Diff 15632.EditedMay 27 2019, 3:01 PM
  • use ID_NEW_SET
  • (this is just to correct the previous version)
  • (now looking into more generic handling in BKE_animdata_copy_id_action)
  • also copy nodetree actions in BKE_animdata_copy_id_action
  • use nodetree action copying from BKE_animdata_copy_id_action

note: while testing, I managed to run into BLI_assert failed: /blender/source/blender/gpu/intern/gpu_codegen.c:2165, gpu_pass_free(), at 'pass->refcount == 0' when quitting blender.
(not quite sure yet if this is related to this pathch though...)

Besides note below (with current code, we can probably end up duplicating animdata's actions of materials twice or more in some cases), LGTM.

source/blender/blenkernel/intern/object.c
1718–1720

That should be put inside the else part above, otherwise you may end up duplicating animdata twice for some materials…

Same goes for first material processing btw (for object materials)… catching another bug with a same stone. ;)

This revision is now accepted and ready to land.May 27 2019, 7:52 PM

note: while testing, I managed to run into BLI_assert failed: /blender/source/blender/gpu/intern/gpu_codegen.c:2165, gpu_pass_free(), at 'pass->refcount == 0' when quitting blender.
(not quite sure yet if this is related to this pathch though...)

no related to this patch [happens in master as well... will report/check that separately...]

Same goes for first material processing btw (for object materials)… catching another bug with a same stone. ;)

Done that for the psys case as well: paranoid doublecheck: I assume this is OK?

Philipp Oeser (lichtwerk) marked an inline comment as done.May 28 2019, 12:54 PM

note: while testing, I managed to run into BLI_assert failed: /blender/source/blender/gpu/intern/gpu_codegen.c:2165, gpu_pass_free(), at 'pass->refcount == 0' when quitting blender.
(not quite sure yet if this is related to this pathch though...)

no related to this patch [happens in master as well... will report/check that separately...]

Good!

Same goes for first material processing btw (for object materials)… catching another bug with a same stone. ;)

Done that for the psys case as well: paranoid doublecheck: I assume this is OK?

Yes, indeed, totally OK, good catch.

Patch LGTM.

This revision was automatically updated to reflect the committed changes.