From a3b8ea843c699eeeec8472fdc76b4c55e9542570 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sybren=20A=2E=20St=C3=BCvel?= Date: Thu, 1 May 2025 11:49:21 +0200 Subject: [PATCH] Anim: Add 'unshare' node to driver evaluation dependency graph Add a new node to the dependency graph, to act as a single entry point before drivers are evaluated in parallel. The node will take all the driven RNA properties, and write their current value to the property again. This ensures that any implicitly shared data is copied to ensure writability. Subsequent concurrent writes by the driver evaluation will then be safe, as the thread-unsafe part has already been performed. Fixes: #132423 Pull Request: https://projects.blender.org/blender/blender/pulls/135802 --- source/blender/blenkernel/BKE_animsys.h | 9 +++- source/blender/blenkernel/intern/anim_sys.cc | 51 ++++++++++++++++--- .../intern/builder/deg_builder_nodes.cc | 29 +++++++++-- .../intern/builder/deg_builder_nodes.h | 1 + .../intern/builder/deg_builder_relations.cc | 9 +++- .../builder/deg_builder_relations_drivers.cc | 14 +++++ .../builder/deg_builder_relations_drivers.h | 12 +++++ .../intern/node/deg_node_operation.cc | 2 + .../intern/node/deg_node_operation.hh | 2 + 9 files changed, 114 insertions(+), 15 deletions(-) diff --git a/source/blender/blenkernel/BKE_animsys.h b/source/blender/blenkernel/BKE_animsys.h index f356cd00e23..58073920e70 100644 --- a/source/blender/blenkernel/BKE_animsys.h +++ b/source/blender/blenkernel/BKE_animsys.h @@ -290,8 +290,14 @@ bool BKE_animsys_rna_path_resolve(struct PointerRNA *ptr, bool BKE_animsys_read_from_rna_path(struct PathResolvedRNA *anim_rna, float *r_value); /** * Write the given value to a setting using RNA, and return success. + * + * \param force_write When false, this function will only call the RNA setter when `value` is + * different from the property's current value. When true, this function will skip that check and + * always call the RNA setter. */ -bool BKE_animsys_write_to_rna_path(struct PathResolvedRNA *anim_rna, float value); +bool BKE_animsys_write_to_rna_path(struct PathResolvedRNA *anim_rna, + float value, + bool force_write = false); /** * Evaluation loop for evaluation animation data @@ -355,6 +361,7 @@ void animsys_evaluate_action_group(struct PointerRNA *ptr, struct Depsgraph; void BKE_animsys_eval_animdata(struct Depsgraph *depsgraph, struct ID *id); +void BKE_animsys_eval_driver_unshare(Depsgraph *depsgraph, ID *id); void BKE_animsys_eval_driver(struct Depsgraph *depsgraph, struct ID *id, int driver_index, diff --git a/source/blender/blenkernel/intern/anim_sys.cc b/source/blender/blenkernel/intern/anim_sys.cc index 1d31bfcbb45..9d87d3c1a87 100644 --- a/source/blender/blenkernel/intern/anim_sys.cc +++ b/source/blender/blenkernel/intern/anim_sys.cc @@ -453,7 +453,9 @@ bool BKE_animsys_read_from_rna_path(PathResolvedRNA *anim_rna, float *r_value) return true; } -bool BKE_animsys_write_to_rna_path(PathResolvedRNA *anim_rna, const float value) +bool BKE_animsys_write_to_rna_path(PathResolvedRNA *anim_rna, + const float value, + const bool force_write) { PropertyRNA *prop = anim_rna->prop; PointerRNA *ptr = &anim_rna->ptr; @@ -462,13 +464,15 @@ bool BKE_animsys_write_to_rna_path(PathResolvedRNA *anim_rna, const float value) /* caller must ensure this is animatable */ BLI_assert(RNA_property_animateable(ptr, prop) || ptr->owner_id == nullptr); - /* Check whether value is new. Otherwise we skip all the updates. */ - float old_value; - if (!BKE_animsys_read_from_rna_path(anim_rna, &old_value)) { - return false; - } - if (old_value == value) { - return true; + if (!force_write) { + /* Check whether value is new. Otherwise we skip all the updates. */ + float old_value; + if (!BKE_animsys_read_from_rna_path(anim_rna, &old_value)) { + return false; + } + if (old_value == value) { + return true; + } } switch (RNA_property_type(prop)) { @@ -4184,6 +4188,37 @@ void BKE_animsys_update_driver_array(ID *id) } } +void BKE_animsys_eval_driver_unshare(Depsgraph *depsgraph, ID *id_eval) +{ + BLI_assert(DEG_is_evaluated_id(id_eval)); + + AnimData *adt = BKE_animdata_from_id(id_eval); + PointerRNA id_ptr = RNA_id_pointer_create(id_eval); + const bool is_active_depsgraph = DEG_is_active(depsgraph); + + LISTBASE_FOREACH (FCurve *, fcu, &adt->drivers) { + /* Resolve the driver RNA path. */ + PathResolvedRNA anim_rna; + if (!BKE_animsys_rna_path_resolve(&id_ptr, fcu->rna_path, fcu->array_index, &anim_rna)) { + continue; + } + + /* Write the current value back to RNA. */ + float curval; + if (!BKE_animsys_read_from_rna_path(&anim_rna, &curval)) { + continue; + } + if (!BKE_animsys_write_to_rna_path(&anim_rna, curval, /*force_write=*/true)) { + continue; + } + + if (is_active_depsgraph) { + /* Also un-share the original data, as the driver evaluation will write here too. */ + animsys_write_orig_anim_rna(&id_ptr, fcu->rna_path, fcu->array_index, curval); + } + } +} + void BKE_animsys_eval_driver(Depsgraph *depsgraph, ID *id, int driver_index, FCurve *fcu_orig) { BLI_assert(fcu_orig != nullptr); diff --git a/source/blender/depsgraph/intern/builder/deg_builder_nodes.cc b/source/blender/depsgraph/intern/builder/deg_builder_nodes.cc index 84317cf0ab1..16e9314d69a 100644 --- a/source/blender/depsgraph/intern/builder/deg_builder_nodes.cc +++ b/source/blender/depsgraph/intern/builder/deg_builder_nodes.cc @@ -105,6 +105,7 @@ #include "intern/builder/deg_builder.h" #include "intern/builder/deg_builder_key.h" +#include "intern/builder/deg_builder_relations_drivers.h" #include "intern/builder/deg_builder_rna.h" #include "intern/depsgraph.hh" #include "intern/depsgraph_light_linking.hh" @@ -1261,11 +1262,7 @@ void DepsgraphNodeBuilder::build_animdata(ID *id) build_animdata_nlastrip_targets(&nlt->strips); } /* Drivers. */ - int driver_index = 0; - LISTBASE_FOREACH (FCurve *, fcu, &adt->drivers) { - /* create driver */ - build_driver(id, fcu, driver_index++); - } + build_animdata_drivers(id, adt); } void DepsgraphNodeBuilder::build_animdata_nlastrip_targets(ListBase *strips) @@ -1312,6 +1309,28 @@ void DepsgraphNodeBuilder::build_action(bAction *action) add_operation_node(&action->id, NodeType::ANIMATION, OperationCode::ANIMATION_EVAL); } +void DepsgraphNodeBuilder::build_animdata_drivers(ID *id, AnimData *adt) +{ + bool needs_unshare = false; + + /* Drivers. */ + int driver_index; + LISTBASE_FOREACH_INDEX (FCurve *, fcu, &adt->drivers, driver_index) { + build_driver(id, fcu, driver_index); + needs_unshare = needs_unshare || data_path_maybe_shared(*id, fcu->rna_path); + } + + if (!needs_unshare) { + return; + } + + ID *id_cow = get_cow_id(id); + ensure_operation_node( + id, NodeType::PARAMETERS, OperationCode::DRIVER_UNSHARE, [id_cow](::Depsgraph *depsgraph) { + BKE_animsys_eval_driver_unshare(depsgraph, id_cow); + }); +} + void DepsgraphNodeBuilder::build_driver(ID *id, FCurve *fcurve, int driver_index) { /* Create data node for this driver */ diff --git a/source/blender/depsgraph/intern/builder/deg_builder_nodes.h b/source/blender/depsgraph/intern/builder/deg_builder_nodes.h index 2769988891a..8c92dad34e6 100644 --- a/source/blender/depsgraph/intern/builder/deg_builder_nodes.h +++ b/source/blender/depsgraph/intern/builder/deg_builder_nodes.h @@ -221,6 +221,7 @@ class DepsgraphNodeBuilder : public DepsgraphBuilder { virtual void build_animation_images(ID *id); virtual void build_action(bAction *action); + virtual void build_animdata_drivers(ID *id, AnimData *adt); /** * Build graph node(s) for Driver * \param id: ID-Block that driver is attached to diff --git a/source/blender/depsgraph/intern/builder/deg_builder_relations.cc b/source/blender/depsgraph/intern/builder/deg_builder_relations.cc index 5948f7aaeb1..7eefd046ebe 100644 --- a/source/blender/depsgraph/intern/builder/deg_builder_relations.cc +++ b/source/blender/depsgraph/intern/builder/deg_builder_relations.cc @@ -102,6 +102,7 @@ #include "intern/builder/deg_builder.h" #include "intern/builder/deg_builder_pchanmap.h" +#include "intern/builder/deg_builder_relations_drivers.h" #include "intern/debug/deg_debug.h" #include "intern/depsgraph_physics.hh" #include "intern/depsgraph_tag.hh" @@ -1745,10 +1746,12 @@ void DepsgraphRelationBuilder::build_animdata_nlastrip_targets(ID *id, void DepsgraphRelationBuilder::build_animdata_drivers(ID *id) { AnimData *adt = BKE_animdata_from_id(id); - if (adt == nullptr) { + if (adt == nullptr || BLI_listbase_is_empty(&adt->drivers)) { return; } ComponentKey adt_key(id, NodeType::ANIMATION); + OperationKey driver_unshare_key(id, NodeType::PARAMETERS, OperationCode::DRIVER_UNSHARE); + LISTBASE_FOREACH (FCurve *, fcu, &adt->drivers) { OperationKey driver_key(id, NodeType::PARAMETERS, @@ -1763,6 +1766,10 @@ void DepsgraphRelationBuilder::build_animdata_drivers(ID *id) if (adt->action || adt->nla_tracks.first) { add_relation(adt_key, driver_key, "AnimData Before Drivers"); } + + if (data_path_maybe_shared(*id, fcu->rna_path)) { + add_relation(driver_unshare_key, driver_key, "Un-share shared data before drivers"); + } } } diff --git a/source/blender/depsgraph/intern/builder/deg_builder_relations_drivers.cc b/source/blender/depsgraph/intern/builder/deg_builder_relations_drivers.cc index cca36c25be4..6aff7c1bfd2 100644 --- a/source/blender/depsgraph/intern/builder/deg_builder_relations_drivers.cc +++ b/source/blender/depsgraph/intern/builder/deg_builder_relations_drivers.cc @@ -240,4 +240,18 @@ void DepsgraphRelationBuilder::build_driver_relations(IDNode *id_node) } } +bool data_path_maybe_shared(const ID &id, const StringRef data_path) +{ + /* As it is hard to generally detect implicit sharing, this is implemented as + * a 'known to not share' list. */ + + if (GS(id.name) == ID_OB) { + const Object &ob = *reinterpret_cast(&id); + const bool is_thread_safe = (ob.type == OB_ARMATURE && data_path.startswith("pose.bones[")); + return !is_thread_safe; + } + + return true; +} + } // namespace blender::deg diff --git a/source/blender/depsgraph/intern/builder/deg_builder_relations_drivers.h b/source/blender/depsgraph/intern/builder/deg_builder_relations_drivers.h index c1ea43401b6..42e0d76c637 100644 --- a/source/blender/depsgraph/intern/builder/deg_builder_relations_drivers.h +++ b/source/blender/depsgraph/intern/builder/deg_builder_relations_drivers.h @@ -58,4 +58,16 @@ class DriverDescriptor { bool resolve_rna(); }; +/** + * Returns whether the data at the given path may be implicitly shared (also see + * #ImplicitSharingInfo). If it is shared, writing to it through RNA will make a + * local copy that can be edited without affecting the other users. + * + * If multi-threaded writing to the path is required, one should trigger making + * the mutable copy before multi-threaded writing starts. Otherwise there is a + * race condition where each thread tries to make its own copy. The "unsharing" + * can be triggered by doing a dummy-write to it. + */ +bool data_path_maybe_shared(const ID &id, StringRef data_path); + } // namespace blender::deg diff --git a/source/blender/depsgraph/intern/node/deg_node_operation.cc b/source/blender/depsgraph/intern/node/deg_node_operation.cc index 6e264a39fd9..62ae8f51aef 100644 --- a/source/blender/depsgraph/intern/node/deg_node_operation.cc +++ b/source/blender/depsgraph/intern/node/deg_node_operation.cc @@ -43,6 +43,8 @@ const char *operationCodeAsString(OperationCode opcode) return "ANIMATION_EXIT"; case OperationCode::DRIVER: return "DRIVER"; + case OperationCode::DRIVER_UNSHARE: + return "DRIVER_UNSHARE"; /* Scene related. */ case OperationCode::SCENE_EVAL: return "SCENE_EVAL"; diff --git a/source/blender/depsgraph/intern/node/deg_node_operation.hh b/source/blender/depsgraph/intern/node/deg_node_operation.hh index b481e986e4b..50620f7f126 100644 --- a/source/blender/depsgraph/intern/node/deg_node_operation.hh +++ b/source/blender/depsgraph/intern/node/deg_node_operation.hh @@ -49,6 +49,8 @@ enum class OperationCode { ANIMATION_EXIT, /* Driver */ DRIVER, + /* Writes to RNA properties to ensure implicitly-shared data is un-shared. */ + DRIVER_UNSHARE, /* Scene related. ------------------------------------------------------- */ SCENE_EVAL,