Details
- Reviewers
Bastien Montagne (mont29) Brecht Van Lommel (brecht) - Maniphest Tasks
- T65175: Animation Data on lamp stays linked even after unlinking all its data
- Commits
- rBSdb0568329e46: Fix T65175: nodetree animation stays linked after duplicating a lamp
rBdb0568329e46: Fix T65175: nodetree animation stays linked after duplicating a lamp
Diff Detail
- Repository
- rB Blender
- Branch
- T65175 (branched from master)
- Build Status
Buildable 3744 Build 3744: arc lint + arc unit
Event Timeline
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...]
- we are doing this https://developer.blender.org/diffusion/B/browse/master/source/blender/blenkernel/intern/object.c$1507
- and again this https://developer.blender.org/diffusion/B/browse/master/source/blender/blenkernel/intern/object.c$1705
checking further...
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); }
Afaict, BKE_animdata_copy_id_action is already called, but not working, see https://developer.blender.org/diffusion/B/browse/master/source/blender/blenkernel/intern/object.c$1699
I guess BKE_animdata_copy_id_action could use ntreeFromID and copy any actions in that node tree as well.
@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()…).
- 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 | ||
|---|---|---|
| 1728–1730 | 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. ;) | |
no related to this patch [happens in master as well... will report/check that separately...]
Done that for the psys case as well: paranoid doublecheck: I assume this is OK?