Page MenuHome

Fix T101536: Missing node tree updates after remapping id.
ClosedPublic

Authored by Jacques Lucke (JacquesLucke) on Oct 18 2022, 6:58 PM.

Details

Summary

The main problem is that the node tree was not properly updated after a property in the tree changed. More specifically, the collection pointer in the Collection Info node was cleared, but the node tree was not updated after that (usually this is handled by rna updates).

ID remapping only updated the node tree under specific circumstances so far using libblock_remap_data_postprocess_nodetree_update. However, currently that only updates node trees when the remapped id is a node tree as well. Unfortunately, we don't really have a system for dependencies between dna data yet which is why these postprocess operations are necessary.

A potential solution is to tag all the node trees that changed for update during the remap phase and then do the update afterwards. I'm mainly not sure at which point the update should happen. Should it be part of the id remap or happen later as part of the operator. Making it part of the operator requires changes in more places but has the benefit that the correct notifiers for certain node tree changes can be send as well (we don't really send notifiers from blenkernel afaik).

Diff Detail

Repository
rB Blender
Branch
propagate-node-tree-change-after-id-remap (branched from master)
Build Status
Buildable 24316
Build 24316: arc lint + arc unit

Event Timeline

Jacques Lucke (JacquesLucke) requested review of this revision.Oct 18 2022, 6:58 PM
Jacques Lucke (JacquesLucke) created this revision.
  • Merge branch 'master' into propagate-node-tree-change-after-id-remap
source/blender/blenkernel/intern/lib_remap.c
122

I find it somewhat weird that we need specific extra tagging for nodetrees here... But anyway, wouldn't be BKE_ntree_update_tag_id_changed a better call here then?

source/blender/blenkernel/intern/lib_remap.c
122

I find it somewhat weird that we need specific extra tagging for nodetrees here...

Yeah it's annoying but also not too bad imo. Tagging the node tree that is has changed after changing it seems fairly reasonable. The thing is that this is handling dependencies in original data instead of in the depsgraph. Indirectly this will also make sure that other ids that depend on the node tree output will be updated (not every change to a node tree changes its output).

But anyway, wouldn't be BKE_ntree_update_tag_id_changed a better call here then?

Generally might be nice to use a more specific update function, but BKE_ntree_update_tag_id_changed is for a different purpose. It's used when data within an id changed instead of the pointer to the id. Ideally, we would know the node whose property was changed. Then we could use BKE_ntree_update_tag_node_property which is much more targeted and can lead to fewer unnecessary updates. However, that's probably a bit out of scope for now (would require passing the node pointer to`BKE_LIB_FOREACHID_PROCESS_ID`).

  • Merge branch 'master' into propagate-node-tree-change-after-id-remap
  • add comment

Only doing tagging in the core ID remapping code is indeed the way to go. Am still a bit sad that nodes require extra processing here, but all in all this patch indeed seems to be the best solution for the time being.

This revision is now accepted and ready to land.Nov 14 2022, 6:00 PM