From 9cb4d3bbff85f74cf13c97683d1a590261a994c0 Mon Sep 17 00:00:00 2001 From: YimingWu Date: Wed, 16 Oct 2024 15:15:11 +0200 Subject: [PATCH] Fix #128887: LineArt: Prevent iterating over not evaluated objects Line art uses `DEG_OBJECT_ITER_BEGIN` to load all objects that has a geometry in the scene, which is kind of a hack since the beginning. This left potential problem where the iterator could go through some objects that line art didn't have a dependency on (e.g. other grease pencil objects): - Since those "other" objects often evaluates fast enough, so line art always end up having valid data from everywhere after lengthy mesh evaluation prior to itself, so this problem was not prominent. - However, in rare cases, there are other objects that takes a lot of time to evaluate, this causes line art to potentially iterate to objects that are still invalid. This fix will build a `Vector` during `update_depsgraph`, and use such list inside `DEGObjectIterSettings` to filter out objects that the modifier isn't dependent on, thus remove the possibility of reading objects that hasn't been evaluated. Since Line art will not add grease pencil objects as potential geometry inputs, all mesh/curve types of geometries generated by geometry nodes modifier directly inside other grease pencil objects won't be loaded. This can be mitigated by having a third mesh object that reads the result from the grease pencil object that generates geometries, then directly output them for line art to read. This also fixes #128888. Pull Request: https://projects.blender.org/blender/blender/pulls/128890 --- .../blender/depsgraph/DEG_depsgraph_query.hh | 7 +++ .../depsgraph/intern/depsgraph_query_iter.cc | 15 +++++ .../intern/grease_pencil_lineart.cc | 2 +- source/blender/makesdna/DNA_modifier_types.h | 3 + source/blender/modifiers/CMakeLists.txt | 4 +- .../blender/modifiers/intern/MOD_lineart.cc | 55 +++++++++++++++++-- .../lineart/{MOD_lineart.h => MOD_lineart.hh} | 8 +++ .../modifiers/intern/lineart/lineart_chain.cc | 4 +- .../modifiers/intern/lineart/lineart_cpu.cc | 17 ++++-- .../{lineart_intern.h => lineart_intern.hh} | 3 +- .../intern/lineart/lineart_shadow.cc | 18 ++++-- .../modifiers/intern/lineart/lineart_util.cc | 4 +- 12 files changed, 117 insertions(+), 23 deletions(-) rename source/blender/modifiers/intern/lineart/{MOD_lineart.h => MOD_lineart.hh} (98%) rename source/blender/modifiers/intern/lineart/{lineart_intern.h => lineart_intern.hh} (98%) diff --git a/source/blender/depsgraph/DEG_depsgraph_query.hh b/source/blender/depsgraph/DEG_depsgraph_query.hh index f268b4d6396..ef3576f5073 100644 --- a/source/blender/depsgraph/DEG_depsgraph_query.hh +++ b/source/blender/depsgraph/DEG_depsgraph_query.hh @@ -12,6 +12,7 @@ #include "BLI_function_ref.hh" #include "BLI_iterator.h" +#include "BLI_set.hh" #include "BLI_utildefines.h" #include "DEG_depsgraph.hh" @@ -199,6 +200,12 @@ struct DEGObjectIterSettings { * geometry for the viewer path included in the iterator. */ const ViewerPath *viewer_path; + + /** + * If not empty, the iterator should only return objects that are in this list (or their + * instances are in it). Pointers in this span should be the original data-block. + */ + blender::Set *included_objects; }; /** diff --git a/source/blender/depsgraph/intern/depsgraph_query_iter.cc b/source/blender/depsgraph/intern/depsgraph_query_iter.cc index 02a30bf1906..a22fa8c8c44 100644 --- a/source/blender/depsgraph/intern/depsgraph_query_iter.cc +++ b/source/blender/depsgraph/intern/depsgraph_query_iter.cc @@ -145,6 +145,14 @@ bool deg_iterator_duplis_step(DEGObjectIterData *data) continue; } + DEGObjectIterSettings *settings = data->settings; + if (settings->included_objects) { + Object *object_orig = DEG_get_original_object(obd); + if (!settings->included_objects->contains(object_orig)) { + continue; + } + } + free_owned_memory(data); data->dupli_object_current = dob; @@ -240,6 +248,13 @@ bool deg_iterator_objects_step(DEGObjectIterData *data) Object *object = (Object *)id_node->id_cow; Object *object_orig = DEG_get_original_object(object); + DEGObjectIterSettings *settings = data->settings; + if (settings->included_objects) { + if (!settings->included_objects->contains(object_orig)) { + continue; + } + } + /* NOTE: The object might be invisible after the latest depsgraph evaluation, in which case * going into its evaluated state might not be safe. For example, its evaluated mesh state * might point to a freed data-block if the mesh is animated. diff --git a/source/blender/editors/grease_pencil/intern/grease_pencil_lineart.cc b/source/blender/editors/grease_pencil/intern/grease_pencil_lineart.cc index 46b2b961c8d..1e2d6fb7392 100644 --- a/source/blender/editors/grease_pencil/intern/grease_pencil_lineart.cc +++ b/source/blender/editors/grease_pencil/intern/grease_pencil_lineart.cc @@ -40,7 +40,7 @@ #include "DEG_depsgraph.hh" #include "DEG_depsgraph_query.hh" -#include "MOD_lineart.h" +#include "MOD_lineart.hh" namespace blender::ed::greasepencil { diff --git a/source/blender/makesdna/DNA_modifier_types.h b/source/blender/makesdna/DNA_modifier_types.h index cb373f27822..32a1e71dafe 100644 --- a/source/blender/makesdna/DNA_modifier_types.h +++ b/source/blender/makesdna/DNA_modifier_types.h @@ -3219,6 +3219,9 @@ typedef struct GreasePencilLineartModifierData { /* Keep a pointer to the render buffer so we can call destroy from #ModifierData. */ struct LineartData *la_data_ptr; + + /* Points to a `LineartModifierRuntime`, which includes the object dependency list. */ + void *runtime; } GreasePencilLineartModifierData; typedef struct GreasePencilArmatureModifierData { diff --git a/source/blender/modifiers/CMakeLists.txt b/source/blender/modifiers/CMakeLists.txt index cc8b24bcb13..753bd5844f3 100644 --- a/source/blender/modifiers/CMakeLists.txt +++ b/source/blender/modifiers/CMakeLists.txt @@ -135,8 +135,8 @@ set(SRC intern/MOD_ui_common.hh intern/MOD_util.hh intern/MOD_weightvg_util.hh - intern/lineart/MOD_lineart.h - intern/lineart/lineart_intern.h + intern/lineart/MOD_lineart.hh + intern/lineart/lineart_intern.hh ) set(LIB diff --git a/source/blender/modifiers/intern/MOD_lineart.cc b/source/blender/modifiers/intern/MOD_lineart.cc index bd32e87619e..9b21b29a40a 100644 --- a/source/blender/modifiers/intern/MOD_lineart.cc +++ b/source/blender/modifiers/intern/MOD_lineart.cc @@ -27,7 +27,7 @@ #include "UI_resources.hh" #include "MOD_grease_pencil_util.hh" -#include "MOD_lineart.h" +#include "MOD_lineart.hh" #include "MOD_modifiertypes.hh" #include "MOD_ui_common.hh" @@ -82,6 +82,32 @@ static void init_data(ModifierData *md) static void copy_data(const ModifierData *md, ModifierData *target, const int flag) { BKE_modifier_copydata_generic(md, target, flag); + + const GreasePencilLineartModifierData *source_lmd = + reinterpret_cast(md); + const LineartModifierRuntime *source_runtime = reinterpret_cast( + source_lmd->runtime); + + GreasePencilLineartModifierData *target_lmd = + reinterpret_cast(target); + + target_lmd->runtime = MEM_new(__func__); + LineartModifierRuntime *target_runtime = reinterpret_cast( + target_lmd->runtime); + + blender::Set *object_dependencies = source_runtime->object_dependencies.get(); + target_runtime->object_dependencies.reset( + new blender::Set(*object_dependencies)); +} + +static void free_data(ModifierData *md) +{ + GreasePencilLineartModifierData *lmd = reinterpret_cast(md); + if (lmd->runtime) { + LineartModifierRuntime *runtime = reinterpret_cast(lmd->runtime); + MEM_delete(runtime); + lmd->runtime = nullptr; + } } static bool is_disabled(const Scene * /*scene*/, ModifierData *md, bool /*use_render_params*/) @@ -107,7 +133,8 @@ static bool is_disabled(const Scene * /*scene*/, ModifierData *md, bool /*use_re static void add_this_collection(Collection &collection, const ModifierUpdateDepsgraphContext *ctx, - const int mode) + const int mode, + Set &object_dependencies) { bool default_add = true; /* Do not do nested collection usage check, this is consistent with lineart calculation, because @@ -124,13 +151,15 @@ static void add_this_collection(Collection &collection, { DEG_add_object_relation(ctx->node, ob, DEG_OB_COMP_GEOMETRY, "Line Art Modifier"); DEG_add_object_relation(ctx->node, ob, DEG_OB_COMP_TRANSFORM, "Line Art Modifier"); + object_dependencies.add(ob); } } if (ob->type == OB_EMPTY && (ob->transflag & OB_DUPLICOLLECTION)) { if (!ob->instance_collection) { continue; } - add_this_collection(*ob->instance_collection, ctx, mode); + add_this_collection(*ob->instance_collection, ctx, mode, object_dependencies); + object_dependencies.add(ob); } } FOREACH_COLLECTION_VISIBLE_OBJECT_RECURSIVE_END; @@ -147,8 +176,24 @@ static void update_depsgraph(ModifierData *md, const ModifierUpdateDepsgraphCont /* Do we need to distinguish DAG_EVAL_VIEWPORT or DAG_EVAL_RENDER here? */ - add_this_collection(*ctx->scene->master_collection, ctx, DAG_EVAL_VIEWPORT); + LineartModifierRuntime *runtime = reinterpret_cast(lmd->runtime); + if (!runtime) { + runtime = MEM_new(__func__); + lmd->runtime = runtime; + runtime->object_dependencies = nullptr; + } + Set *object_dependencies = runtime->object_dependencies.get(); + if (!object_dependencies) { + runtime->object_dependencies = std::make_unique>(); + object_dependencies = runtime->object_dependencies.get(); + } + object_dependencies->clear(); + add_this_collection( + *ctx->scene->master_collection, ctx, DAG_EVAL_VIEWPORT, *object_dependencies); + + /* No need to add any non-geometry objects into `lmd->object_dependencies` because we won't be + * loading */ if (lmd->calculation_flags & MOD_LINEART_USE_CUSTOM_CAMERA && lmd->source_camera) { DEG_add_object_relation( ctx->node, lmd->source_camera, DEG_OB_COMP_TRANSFORM, "Line Art Modifier"); @@ -852,7 +897,7 @@ ModifierTypeInfo modifierType_GreasePencilLineart = { /*init_data*/ blender::init_data, /*required_data_mask*/ nullptr, - /*free_data*/ nullptr, + /*free_data*/ blender::free_data, /*is_disabled*/ blender::is_disabled, /*update_depsgraph*/ blender::update_depsgraph, /*depends_on_time*/ nullptr, diff --git a/source/blender/modifiers/intern/lineart/MOD_lineart.h b/source/blender/modifiers/intern/lineart/MOD_lineart.hh similarity index 98% rename from source/blender/modifiers/intern/lineart/MOD_lineart.h rename to source/blender/modifiers/intern/lineart/MOD_lineart.hh index c4af5ee495c..1f4ac8a04d5 100644 --- a/source/blender/modifiers/intern/lineart/MOD_lineart.h +++ b/source/blender/modifiers/intern/lineart/MOD_lineart.hh @@ -12,6 +12,7 @@ #include "BLI_listbase.h" #include "BLI_math_matrix_types.hh" #include "BLI_math_vector.h" +#include "BLI_set.hh" #include "BLI_threads.h" #include "ED_grease_pencil.hh" @@ -23,6 +24,13 @@ extern "C" { #endif +typedef struct LineartModifierRuntime { + /* This list is constructed during `update_depsgraph()` call, and stays valid until the next + * update. This way line art can load objects from this list instead of iterating over all + * obejcts that may or may not have finished evaluating. */ + std::unique_ptr> object_dependencies; +} LineartModifierRuntime; + typedef struct LineartStaticMemPoolNode { Link item; size_t size; diff --git a/source/blender/modifiers/intern/lineart/lineart_chain.cc b/source/blender/modifiers/intern/lineart/lineart_chain.cc index 97fff9da2b8..1af076339ed 100644 --- a/source/blender/modifiers/intern/lineart/lineart_chain.cc +++ b/source/blender/modifiers/intern/lineart/lineart_chain.cc @@ -9,9 +9,9 @@ #include "BLI_listbase.h" #include "BLI_math_geom.h" -#include "MOD_lineart.h" +#include "MOD_lineart.hh" -#include "lineart_intern.h" +#include "lineart_intern.hh" #include /* For `min/max`. */ #include diff --git a/source/blender/modifiers/intern/lineart/lineart_cpu.cc b/source/blender/modifiers/intern/lineart/lineart_cpu.cc index 9703087ee91..a93bd91d38b 100644 --- a/source/blender/modifiers/intern/lineart/lineart_cpu.cc +++ b/source/blender/modifiers/intern/lineart/lineart_cpu.cc @@ -8,7 +8,7 @@ #include -#include "MOD_lineart.h" +#include "MOD_lineart.hh" #include "BLI_listbase.h" #include "BLI_math_base.hh" @@ -62,7 +62,7 @@ #include "GEO_join_geometries.hh" -#include "lineart_intern.h" +#include "lineart_intern.hh" #include /* For `min/max`. */ @@ -2551,7 +2551,8 @@ void lineart_main_load_geometries(Depsgraph *depsgraph, LineartData *ld, bool allow_duplicates, bool do_shadow_casting, - ListBase *shadow_elns) + ListBase *shadow_elns, + blender::Set *included_objects) { double proj[4][4], view[4][4], result[4][4]; float inv[4][4]; @@ -2615,9 +2616,8 @@ void lineart_main_load_geometries(Depsgraph *depsgraph, DEGObjectIterSettings deg_iter_settings = {nullptr}; deg_iter_settings.depsgraph = depsgraph; deg_iter_settings.flags = flags; + deg_iter_settings.included_objects = included_objects; - /* XXX(@Yiming): Temporary solution, this iterator is technically unsafe to use *during* - * depsgraph evaluation, see D14997 for detailed explanations. */ DEG_OBJECT_ITER_BEGIN (°_iter_settings, ob) { obindex++; @@ -5093,13 +5093,18 @@ bool MOD_lineart_compute_feature_lines_v3(Depsgraph *depsgraph, /* Get view vector before loading geometries, because we detect feature lines there. */ lineart_main_get_view_vector(ld); + LineartModifierRuntime *runtime = reinterpret_cast(lmd.runtime); + blender::Set *included_objects = runtime ? runtime->object_dependencies.get() : + nullptr; + lineart_main_load_geometries(depsgraph, scene, lineart_camera, ld, lmd.calculation_flags & MOD_LINEART_ALLOW_DUPLI_OBJECTS, false, - shadow_elns); + shadow_elns, + included_objects); if (shadow_generated) { lineart_main_transform_and_add_shadow(ld, shadow_veln, shadow_eeln); diff --git a/source/blender/modifiers/intern/lineart/lineart_intern.h b/source/blender/modifiers/intern/lineart/lineart_intern.hh similarity index 98% rename from source/blender/modifiers/intern/lineart/lineart_intern.h rename to source/blender/modifiers/intern/lineart/lineart_intern.hh index e2d616faf2e..c925bfc764c 100644 --- a/source/blender/modifiers/intern/lineart/lineart_intern.h +++ b/source/blender/modifiers/intern/lineart/lineart_intern.hh @@ -131,7 +131,8 @@ void lineart_main_load_geometries(struct Depsgraph *depsgraph, struct LineartData *ld, bool allow_duplicates, bool do_shadow_casting, - struct ListBase *shadow_elns); + struct ListBase *shadow_elns, + blender::Set *included_objects); /** * The calculated view vector will point towards the far-plane from the camera position. */ diff --git a/source/blender/modifiers/intern/lineart/lineart_shadow.cc b/source/blender/modifiers/intern/lineart/lineart_shadow.cc index 3db269e68a3..f33b6edc6c1 100644 --- a/source/blender/modifiers/intern/lineart/lineart_shadow.cc +++ b/source/blender/modifiers/intern/lineart/lineart_shadow.cc @@ -6,9 +6,9 @@ * \ingroup modifiers */ -#include "MOD_lineart.h" +#include "MOD_lineart.hh" -#include "lineart_intern.h" +#include "lineart_intern.hh" #include "BKE_global.hh" #include "BKE_grease_pencil.hh" @@ -1251,8 +1251,18 @@ bool lineart_main_try_generate_shadow_v3(Depsgraph *depsgraph, lineart_main_get_view_vector(ld); - lineart_main_load_geometries( - depsgraph, scene, nullptr, ld, lmd->flags & MOD_LINEART_ALLOW_DUPLI_OBJECTS, true, nullptr); + LineartModifierRuntime *runtime = reinterpret_cast(lmd->runtime); + blender::Set *included_objects = runtime ? runtime->object_dependencies.get() : + nullptr; + + lineart_main_load_geometries(depsgraph, + scene, + nullptr, + ld, + lmd->flags & MOD_LINEART_ALLOW_DUPLI_OBJECTS, + true, + nullptr, + included_objects); if (!ld->geom.vertex_buffer_pointers.first) { /* No geometry loaded, return early. */ diff --git a/source/blender/modifiers/intern/lineart/lineart_util.cc b/source/blender/modifiers/intern/lineart/lineart_util.cc index c0df207c60d..c96bf755a0f 100644 --- a/source/blender/modifiers/intern/lineart/lineart_util.cc +++ b/source/blender/modifiers/intern/lineart/lineart_util.cc @@ -16,9 +16,9 @@ #include "BLI_math_matrix.h" -#include "MOD_lineart.h" +#include "MOD_lineart.hh" -#include "lineart_intern.h" +#include "lineart_intern.hh" /* Line art memory and list helper */