From fc7141f582569c7b7366df3efe97189365bb09f3 Mon Sep 17 00:00:00 2001 From: Campbell Barton Date: Thu, 21 Aug 2025 12:03:33 +0000 Subject: [PATCH] Fix #142137: Dynamic paint crashes when evaluating poses DEG_add_collision_relations was only adding relationships for objects in the brush collection, however dynamic paint also updates parent objects and armature poses which weren't accounted for. Failing to include these relations meant the evaluating the depsgraph could evaluate a pose object from multiple threads at once - causing a crash. Resolve by sharing logic between DEG_add_collision_relations & BKE_object_modifier_update_subframe so depsgraph relationships match objects the dynamic-paint modifier updates as part of its evaluation. Ref !144844 --- source/blender/blenkernel/BKE_object.hh | 14 +- source/blender/blenkernel/intern/fluid.cc | 9 +- source/blender/blenkernel/intern/object.cc | 154 ++++++++++++------ .../depsgraph/intern/depsgraph_physics.cc | 84 ++++++++++ 4 files changed, 209 insertions(+), 52 deletions(-) diff --git a/source/blender/blenkernel/BKE_object.hh b/source/blender/blenkernel/BKE_object.hh index dd680b24bcd..3f7cd3440b2 100644 --- a/source/blender/blenkernel/BKE_object.hh +++ b/source/blender/blenkernel/BKE_object.hh @@ -13,6 +13,7 @@ #include "BLI_bounds_types.hh" #include "BLI_compiler_attrs.h" +#include "BLI_function_ref.hh" #include "BLI_math_matrix_types.hh" #include "BLI_math_vector_types.hh" #include "BLI_sys_types.h" @@ -663,7 +664,7 @@ KDTree_3d *BKE_object_as_kdtree(Object *ob, int *r_tot); * \note this function should eventually be replaced by depsgraph functionality. * Avoid calling this in new code unless there is a very good reason for it! */ -bool BKE_object_modifier_update_subframe(Depsgraph *depsgraph, +void BKE_object_modifier_update_subframe(Depsgraph *depsgraph, Scene *scene, Object *ob, bool update_mesh, @@ -671,6 +672,17 @@ bool BKE_object_modifier_update_subframe(Depsgraph *depsgraph, float frame, int /*ModifierType*/ modifier_type); +/** + * Call `update_or_tag_fn` on all objects which #BKE_object_modifier_update_subframe would update. + * Used by the depsgraph to setup relations. + */ +void BKE_object_modifier_update_subframe_only_callback( + Object *ob, + bool update_mesh, + int parent_recursion_limit, + int /*ModifierType*/ modifier_type, + blender::FunctionRef update_or_tag_fn); + bool BKE_object_empty_image_frame_is_visible_in_view3d(const Object *ob, const RegionView3D *rv3d); bool BKE_object_empty_image_data_is_visible_in_view3d(const Object *ob, const RegionView3D *rv3d); diff --git a/source/blender/blenkernel/intern/fluid.cc b/source/blender/blenkernel/intern/fluid.cc index 987262c65ed..d7e27fc21f7 100644 --- a/source/blender/blenkernel/intern/fluid.cc +++ b/source/blender/blenkernel/intern/fluid.cc @@ -2739,8 +2739,13 @@ static void compute_flowsemission(Scene *scene, * BLI_mutex_lock() called in manta_step(), so safe to update subframe here * TODO(sebbas): Using BKE_scene_ctime_get(scene) instead of new DEG_get_ctime(depsgraph) * as subframes don't work with the latter yet. */ - BKE_object_modifier_update_subframe( - depsgraph, scene, flowobj, true, 5, BKE_scene_ctime_get(scene), eModifierType_Fluid); + BKE_object_modifier_update_subframe(depsgraph, + scene, + flowobj, + true, + OBJECT_MODIFIER_UPDATE_SUBFRAME_RECURSION_DEFAULT, + BKE_scene_ctime_get(scene), + eModifierType_Fluid); /* Emission from particles. */ if (ffs->source == FLUID_FLOW_SOURCE_PARTICLES) { diff --git a/source/blender/blenkernel/intern/object.cc b/source/blender/blenkernel/intern/object.cc index 37c3125fa64..fd8a5147378 100644 --- a/source/blender/blenkernel/intern/object.cc +++ b/source/blender/blenkernel/intern/object.cc @@ -5213,18 +5213,41 @@ static void object_cacheIgnoreClear(Object *ob, const bool state) BLI_freelistN(&pidlist); } -bool BKE_object_modifier_update_subframe(Depsgraph *depsgraph, - Scene *scene, - Object *ob, - const bool update_mesh, - const int parent_recursion_limit, - const float frame, - const /*ModifierType*/ int modifier_type) -{ - const bool flush_to_original = DEG_is_active(depsgraph); - ModifierData *md = BKE_modifiers_findby_type(ob, ModifierType(modifier_type)); +struct ObjectModifierUpdateContext { + ModifierType modifier_type; - if (modifier_type == eModifierType_DynamicPaint) { + /** Update or tagging logic should go here. */ + blender::FunctionRef update_or_tag_fn; +}; + +/** + * Utility used to implement + * - #BKE_object_modifier_update_subframe + * - #BKE_object_modifier_update_subframe_only_callback + * + * The actual updating may be done by a callback: `ctx.update_or_tag_fn`, + * called by this function for each object. + */ +static bool object_modifier_recurse_for_update_subframe(const ObjectModifierUpdateContext &ctx, + Object *ob, + const bool update_mesh, + const int parent_recursion_limit) +{ + /* NOTE: this function must not modify the object + * since this is used for setting up depsgraph relationships. + * + * Although the #ObjectModifierUpdateContext::update_or_tag_fn callback may change the object + * as this is needed to implement #BKE_object_modifier_update_subframe. */ + + /* NOTE(@ideasman42): `parent_recursion_limit` is used to prevent this function attempting to + * scan object hierarchies infinitely, needed since constraint targets are also included. + * A more elegant alternative may be to track which objects have been handled. + * Since #BKE_object_modifier_update_subframe is documented not to be used + * for new code (if possible), leave as-is. */ + + ModifierData *md = BKE_modifiers_findby_type(ob, ctx.modifier_type); + + if (ctx.modifier_type == eModifierType_DynamicPaint) { DynamicPaintModifierData *pmd = (DynamicPaintModifierData *)md; /* if other is dynamic paint canvas, don't update */ @@ -5232,7 +5255,7 @@ bool BKE_object_modifier_update_subframe(Depsgraph *depsgraph, return true; } } - else if (modifier_type == eModifierType_Fluid) { + else if (ctx.modifier_type == eModifierType_Fluid) { FluidModifierData *fmd = (FluidModifierData *)md; if (fmd && (fmd->type & MOD_FLUID_TYPE_DOMAIN) != 0) { @@ -5245,12 +5268,10 @@ bool BKE_object_modifier_update_subframe(Depsgraph *depsgraph, const int recursion = parent_recursion_limit - 1; bool no_update = false; if (ob->parent) { - no_update |= BKE_object_modifier_update_subframe( - depsgraph, scene, ob->parent, false, recursion, frame, modifier_type); + no_update |= object_modifier_recurse_for_update_subframe(ctx, ob->parent, false, recursion); } if (ob->track) { - no_update |= BKE_object_modifier_update_subframe( - depsgraph, scene, ob->track, false, recursion, frame, modifier_type); + no_update |= object_modifier_recurse_for_update_subframe(ctx, ob->track, false, recursion); } /* Skip sub-frame if object is parented to vertex of a dynamic paint canvas. */ @@ -5265,8 +5286,7 @@ bool BKE_object_modifier_update_subframe(Depsgraph *depsgraph, if (BKE_constraint_targets_get(con, &targets)) { LISTBASE_FOREACH (bConstraintTarget *, ct, &targets) { if (ct->tar) { - BKE_object_modifier_update_subframe( - depsgraph, scene, ct->tar, false, recursion, frame, modifier_type); + object_modifier_recurse_for_update_subframe(ctx, ct->tar, false, recursion); } } /* free temp targets */ @@ -5275,41 +5295,77 @@ bool BKE_object_modifier_update_subframe(Depsgraph *depsgraph, } } - /* was originally ID_RECALC_ALL - TODO: which flags are really needed??? */ - /* TODO(sergey): What about animation? */ - const AnimationEvalContext anim_eval_context = BKE_animsys_eval_context_construct(depsgraph, - frame); - - ob->id.recalc |= ID_RECALC_ALL; - if (update_mesh) { - BKE_animsys_evaluate_animdata( - &ob->id, ob->adt, &anim_eval_context, ADT_RECALC_ANIM, flush_to_original); - /* Ignore cache clear during sub-frame updates to not mess up cache validity. */ - object_cacheIgnoreClear(ob, true); - BKE_object_handle_update(depsgraph, scene, ob); - object_cacheIgnoreClear(ob, false); - } - else { - BKE_object_where_is_calc_time(depsgraph, scene, ob, frame); - } - - /* for curve following objects, parented curve has to be updated too */ - if (ob->type == OB_CURVES_LEGACY) { - Curve *cu = (Curve *)ob->data; - BKE_animsys_evaluate_animdata( - &cu->id, cu->adt, &anim_eval_context, ADT_RECALC_ANIM, flush_to_original); - } - /* and armatures... */ - if (ob->type == OB_ARMATURE) { - bArmature *arm = (bArmature *)ob->data; - BKE_animsys_evaluate_animdata( - &arm->id, arm->adt, &anim_eval_context, ADT_RECALC_ANIM, flush_to_original); - BKE_pose_where_is(depsgraph, scene, ob); - } + ctx.update_or_tag_fn(ob, update_mesh); return false; } +void BKE_object_modifier_update_subframe_only_callback( + Object *ob, + const bool update_mesh, + const int parent_recursion_limit, + const /*ModifierType*/ int modifier_type, + blender::FunctionRef update_or_tag_fn) +{ + ObjectModifierUpdateContext ctx = { + ModifierType(modifier_type), + update_or_tag_fn, + }; + object_modifier_recurse_for_update_subframe(ctx, ob, update_mesh, parent_recursion_limit); +} + +void BKE_object_modifier_update_subframe(Depsgraph *depsgraph, + Scene *scene, + Object *ob, + const bool update_mesh, + const int parent_recursion_limit, + const float frame, + const /*ModifierType*/ int modifier_type) +{ + const bool flush_to_original = DEG_is_active(depsgraph); + + auto update_or_tag_fn = [depsgraph, scene, frame, flush_to_original](Object *ob, + const bool update_mesh) { + /* NOTE: changes here may require updates to #DEG_add_collision_relations + * so the depsgraph is handling updates correctly. */ + + /* was originally ID_RECALC_ALL - TODO: which flags are really needed??? */ + /* TODO(sergey): What about animation? */ + const AnimationEvalContext anim_eval_context = BKE_animsys_eval_context_construct(depsgraph, + frame); + + ob->id.recalc |= ID_RECALC_ALL; + if (update_mesh) { + BKE_animsys_evaluate_animdata( + &ob->id, ob->adt, &anim_eval_context, ADT_RECALC_ANIM, flush_to_original); + /* Ignore cache clear during sub-frame updates to not mess up cache validity. */ + object_cacheIgnoreClear(ob, true); + BKE_object_handle_update(depsgraph, scene, ob); + object_cacheIgnoreClear(ob, false); + } + else { + BKE_object_where_is_calc_time(depsgraph, scene, ob, frame); + } + + /* for curve following objects, parented curve has to be updated too */ + if (ob->type == OB_CURVES_LEGACY) { + Curve *cu = (Curve *)ob->data; + BKE_animsys_evaluate_animdata( + &cu->id, cu->adt, &anim_eval_context, ADT_RECALC_ANIM, flush_to_original); + } + /* and armatures... */ + if (ob->type == OB_ARMATURE) { + bArmature *arm = (bArmature *)ob->data; + BKE_animsys_evaluate_animdata( + &arm->id, arm->adt, &anim_eval_context, ADT_RECALC_ANIM, flush_to_original); + BKE_pose_where_is(depsgraph, scene, ob); + } + }; + + BKE_object_modifier_update_subframe_only_callback( + ob, update_mesh, parent_recursion_limit, modifier_type, update_or_tag_fn); +} + void BKE_object_update_select_id(Main *bmain) { Object *ob = (Object *)bmain->objects.first; diff --git a/source/blender/depsgraph/intern/depsgraph_physics.cc b/source/blender/depsgraph/intern/depsgraph_physics.cc index aca8653431e..9d2f67a232b 100644 --- a/source/blender/depsgraph/intern/depsgraph_physics.cc +++ b/source/blender/depsgraph/intern/depsgraph_physics.cc @@ -13,8 +13,10 @@ #include "BLI_listbase.h" #include "BKE_collision.h" +#include "BKE_dynamicpaint.h" #include "BKE_effect.h" #include "BKE_modifier.hh" +#include "BKE_object.hh" #include "DNA_collection_types.h" #include "DNA_object_force_types.h" @@ -86,6 +88,24 @@ ListBase *DEG_get_collision_relations(const Depsgraph *graph, /********************** Depsgraph Building API ************************/ +/** + * Flags to store point-cache relations which have been calculated. + * This avoid adding relations multiple times. + * + * \note This could be replaced by bit-shifting #eDepsObjectComponentType values, + * although this would limit them to integer size. + */ +enum class CollisionComponentFlag : uint8_t { + None = 0, + /** #DEG_OB_COMP_TRANSFORM is set. */ + Transform = 1 << 0, + /** #DEG_OB_COMP_GEOMETRY is set. */ + Geometry = 1 << 1, + /** #DEG_OB_COMP_EVAL_POSE is set. */ + EvalPose = 1 << 2, +}; +ENUM_OPERATORS(CollisionComponentFlag, CollisionComponentFlag::EvalPose); + void DEG_add_collision_relations(DepsNodeHandle *handle, Object *object, Collection *collection, @@ -96,6 +116,18 @@ void DEG_add_collision_relations(DepsNodeHandle *handle, Depsgraph *depsgraph = DEG_get_graph_from_handle(handle); deg::Depsgraph *deg_graph = (deg::Depsgraph *)depsgraph; ListBase *relations = build_collision_relations(deg_graph, collection, modifier_type); + + /* Expand tag objects, matching: #BKE_object_modifier_update_subframe behavior. */ + + /* NOTE: #eModifierType_Fluid should be included, + * leave out for the purpose of validating the fix for dynamic paint only. */ + const bool use_recursive_parents = (modifier_type == eModifierType_DynamicPaint); + + blender::Map *object_component_map = nullptr; + if (use_recursive_parents) { + object_component_map = MEM_new>(__func__); + } + LISTBASE_FOREACH (CollisionRelation *, relation, relations) { Object *ob1 = relation->ob; if (ob1 == object) { @@ -107,9 +139,61 @@ void DEG_add_collision_relations(DepsNodeHandle *handle, continue; } + if (use_recursive_parents) { + /* Add relations for `ob1` and other objects it references, + * using `object_component_map` to avoid redundant calls. + * + * When #BKE_object_modifier_update_subframe is used by a modifier, + * it's important the depsgraph tags objects this modifier uses. + * + * Without this, access to objects is not thread-safe, see: #142137. + * + * NOTE(@ideasman42): #BKE_object_modifier_update_subframe calls + * #BKE_animsys_evaluate_animdata, depending on the object type. + * Equivalent relations could be added here. + * This was not done and there are no bug reports relating to this, + * so leave as-is unless the current code is failing in a real world scenario. */ + + BKE_object_modifier_update_subframe_only_callback( + ob1, + true, + OBJECT_MODIFIER_UPDATE_SUBFRAME_RECURSION_DEFAULT, + modifier_type, + [&handle, &name, &object_component_map](Object *ob, const bool update_mesh) { + CollisionComponentFlag &update_flag = object_component_map->lookup_or_add_default(ob); + { + constexpr CollisionComponentFlag test_flag = CollisionComponentFlag::Transform; + if ((update_flag & test_flag) == CollisionComponentFlag::None) { + update_flag |= test_flag; + DEG_add_object_pointcache_relation(handle, ob, DEG_OB_COMP_TRANSFORM, name); + } + } + if (update_mesh) { + constexpr CollisionComponentFlag test_flag = CollisionComponentFlag::Geometry; + if ((update_flag & test_flag) == CollisionComponentFlag::None) { + update_flag |= test_flag; + DEG_add_object_pointcache_relation(handle, ob, DEG_OB_COMP_GEOMETRY, name); + } + } + if (ob->type == OB_ARMATURE) { + constexpr CollisionComponentFlag test_flag = CollisionComponentFlag::EvalPose; + if ((update_flag & test_flag) == CollisionComponentFlag::None) { + update_flag |= test_flag; + DEG_add_object_pointcache_relation(handle, ob, DEG_OB_COMP_EVAL_POSE, name); + } + } + }); + + continue; + } + DEG_add_object_pointcache_relation(handle, ob1, DEG_OB_COMP_TRANSFORM, name); DEG_add_object_pointcache_relation(handle, ob1, DEG_OB_COMP_GEOMETRY, name); } + + if (use_recursive_parents) { + MEM_delete(object_component_map); + } } void DEG_add_forcefield_relations(DepsNodeHandle *handle,