From 580833165ca105016af5e5b7338232de429f203b Mon Sep 17 00:00:00 2001 From: Hans Goudey Date: Wed, 26 Jul 2023 17:04:13 +0200 Subject: [PATCH] Fix #109583: Avoid non-threadsafe writing to custom normals data Currently, while calculating face corner normals, Blender retrieves custom normal data with write access. When the the custom normals in a single smooth corner fan don't match, they are reset to the average value. This behavior is very old, but it comes from when Blender didn't have a strong idea of const correctness. Indeed, modifying custom normal data while calculating normals isn't threadsafe, which is important because normals are calculated for viewport drawing, for example. And in the future, properly caching face corner normals (see #93551) will require the ability to calculate normals on a properly const mesh. The fix is to still use the average of custom normals in a fan, but not write that back to the custom data array. In my testing the results are the same. Setting custom normals still fills the same value for all corners in a fan. Pull Request: https://projects.blender.org/blender/blender/pulls/110478 --- source/blender/blenkernel/BKE_mesh.hh | 2 +- .../blenkernel/intern/data_transfer.cc | 6 +-- source/blender/blenkernel/intern/key.cc | 6 +-- source/blender/blenkernel/intern/mesh.cc | 7 ++- .../blender/blenkernel/intern/mesh_mirror.cc | 2 +- .../blender/blenkernel/intern/mesh_normals.cc | 47 +++++-------------- .../blender/blenkernel/intern/mesh_remap.cc | 6 +-- .../draw_cache_extract_mesh_render_data.cc | 6 +-- .../modifiers/intern/MOD_normal_edit.cc | 2 +- .../modifiers/intern/MOD_weighted_normal.cc | 4 +- 10 files changed, 31 insertions(+), 57 deletions(-) diff --git a/source/blender/blenkernel/BKE_mesh.hh b/source/blender/blenkernel/BKE_mesh.hh index 6eac45cbd67..19303ba408a 100644 --- a/source/blender/blenkernel/BKE_mesh.hh +++ b/source/blender/blenkernel/BKE_mesh.hh @@ -156,9 +156,9 @@ void normals_calc_loop(Span vert_positions, Span face_normals, const bool *sharp_edges, const bool *sharp_faces, + const short2 *clnors_data, bool use_split_normals, float split_angle, - short2 *clnors_data, CornerNormalSpaceArray *r_lnors_spacearr, MutableSpan r_loop_normals); diff --git a/source/blender/blenkernel/intern/data_transfer.cc b/source/blender/blenkernel/intern/data_transfer.cc index f667b765cda..2b7d76461bc 100644 --- a/source/blender/blenkernel/intern/data_transfer.cc +++ b/source/blender/blenkernel/intern/data_transfer.cc @@ -371,8 +371,8 @@ static void data_transfer_dtdata_type_preprocess(const Mesh *me_src, BLI_assert(CustomData_get_layer(&me_src->loop_data, CD_NORMAL) != nullptr); (void)me_src; - blender::short2 *custom_nors_dst = static_cast( - CustomData_get_layer_for_write(ldata_dst, CD_CUSTOMLOOPNORMAL, me_dst->totloop)); + const blender::short2 *custom_nors_dst = static_cast( + CustomData_get_layer(ldata_dst, CD_CUSTOMLOOPNORMAL)); /* Cache loop nors into a temp CDLayer. */ blender::float3 *loop_nors_dst = static_cast( @@ -398,9 +398,9 @@ static void data_transfer_dtdata_type_preprocess(const Mesh *me_src, me_dst->face_normals(), sharp_edges, sharp_faces, + custom_nors_dst, use_split_nors_dst, split_angle_dst, - custom_nors_dst, nullptr, {loop_nors_dst, me_dst->totloop}); } diff --git a/source/blender/blenkernel/intern/key.cc b/source/blender/blenkernel/intern/key.cc index 373f13d8fb4..6a62c0cb910 100644 --- a/source/blender/blenkernel/intern/key.cc +++ b/source/blender/blenkernel/intern/key.cc @@ -2281,8 +2281,8 @@ void BKE_keyblock_mesh_calc_normals(const KeyBlock *kb, {reinterpret_cast(vert_normals), mesh->totvert}); } if (loop_normals_needed) { - blender::short2 *clnors = static_cast(CustomData_get_layer_for_write( - &mesh->loop_data, CD_CUSTOMLOOPNORMAL, corner_verts.size())); + const blender::short2 *clnors = static_cast( + CustomData_get_layer(&mesh->loop_data, CD_CUSTOMLOOPNORMAL)); const bool *sharp_edges = static_cast( CustomData_get_layer_named(&mesh->edge_data, CD_PROP_BOOL, "sharp_edge")); const bool *sharp_faces = static_cast( @@ -2298,9 +2298,9 @@ void BKE_keyblock_mesh_calc_normals(const KeyBlock *kb, {reinterpret_cast(face_normals), faces.size()}, sharp_edges, sharp_faces, + clnors, (mesh->flag & ME_AUTOSMOOTH) != 0, mesh->smoothresh, - clnors, nullptr, {reinterpret_cast(r_loop_normals), corner_verts.size()}); } diff --git a/source/blender/blenkernel/intern/mesh.cc b/source/blender/blenkernel/intern/mesh.cc index 3595fcb82c2..825d839c314 100644 --- a/source/blender/blenkernel/intern/mesh.cc +++ b/source/blender/blenkernel/intern/mesh.cc @@ -1789,9 +1789,8 @@ void BKE_mesh_calc_normals_split_ex(Mesh *mesh, ((mesh->flag & ME_AUTOSMOOTH) != 0); const float split_angle = (mesh->flag & ME_AUTOSMOOTH) != 0 ? mesh->smoothresh : float(M_PI); - /* may be nullptr */ - blender::short2 *clnors = (blender::short2 *)CustomData_get_layer_for_write( - &mesh->loop_data, CD_CUSTOMLOOPNORMAL, mesh->totloop); + const blender::short2 *clnors = static_cast( + CustomData_get_layer(&mesh->loop_data, CD_CUSTOMLOOPNORMAL)); const bool *sharp_edges = static_cast( CustomData_get_layer_named(&mesh->edge_data, CD_PROP_BOOL, "sharp_edge")); const bool *sharp_faces = static_cast( @@ -1808,9 +1807,9 @@ void BKE_mesh_calc_normals_split_ex(Mesh *mesh, mesh->face_normals(), sharp_edges, sharp_faces, + clnors, use_split_normals, split_angle, - clnors, nullptr, {reinterpret_cast(r_corner_normals), mesh->totloop}); } diff --git a/source/blender/blenkernel/intern/mesh_mirror.cc b/source/blender/blenkernel/intern/mesh_mirror.cc index 0b89bbc697d..e06c2b2d381 100644 --- a/source/blender/blenkernel/intern/mesh_mirror.cc +++ b/source/blender/blenkernel/intern/mesh_mirror.cc @@ -417,9 +417,9 @@ Mesh *BKE_mesh_mirror_apply_mirror_on_axis_for_modifier(MirrorModifierData *mmd, result->face_normals(), sharp_edges, sharp_faces, + clnors, true, result->smoothresh, - clnors, &lnors_spacearr, loop_normals); diff --git a/source/blender/blenkernel/intern/mesh_normals.cc b/source/blender/blenkernel/intern/mesh_normals.cc index ad8e5b20050..872e9fed5ef 100644 --- a/source/blender/blenkernel/intern/mesh_normals.cc +++ b/source/blender/blenkernel/intern/mesh_normals.cc @@ -700,7 +700,6 @@ struct LoopSplitTaskDataCommon { * different elements in the arrays. */ CornerNormalSpaceArray *lnors_spacearr; MutableSpan loop_normals; - MutableSpan clnors_data; /* Read-only. */ Span positions; @@ -712,6 +711,7 @@ struct LoopSplitTaskDataCommon { Span loop_to_face; Span face_normals; Span vert_normals; + Span clnors_data; }; #define INDEX_UNSET INT_MIN @@ -916,7 +916,6 @@ static void split_loop_nor_fan_do(LoopSplitTaskDataCommon *common_data, { CornerNormalSpaceArray *lnors_spacearr = common_data->lnors_spacearr; MutableSpan loop_normals = common_data->loop_normals; - MutableSpan clnors_data = common_data->clnors_data; const Span positions = common_data->positions; const Span edges = common_data->edges; @@ -926,6 +925,7 @@ static void split_loop_nor_fan_do(LoopSplitTaskDataCommon *common_data, const Span edge_to_loops = common_data->edge_to_loops; const Span loop_to_face = common_data->loop_to_face; const Span face_normals = common_data->face_normals; + const Span clnors_data = common_data->clnors_data; const int face_index = loop_to_face[ml_curr_index]; const int ml_prev_index = face_corner_prev(faces[face_index], ml_curr_index); @@ -946,11 +946,7 @@ static void split_loop_nor_fan_do(LoopSplitTaskDataCommon *common_data, float3 vec_org; float3 lnor(0.0f); - /* We validate clnors data on the fly - cheapest way to do! */ int2 clnors_avg(0); - const short2 *clnor_ref = nullptr; - int clnors_count = 0; - bool clnors_invalid = false; Vector processed_corners; @@ -995,19 +991,6 @@ static void split_loop_nor_fan_do(LoopSplitTaskDataCommon *common_data, /* Calculate angle between the two face edges incident on this vertex. */ lnor += face_normals[loop_to_face[mlfan_curr_index]] * saacos(math::dot(vec_curr, vec_prev)); - if (!clnors_data.is_empty()) { - /* Accumulate all clnors, if they are not all equal we have to fix that! */ - const short2 &clnor = clnors_data[mlfan_vert_index]; - if (clnors_count) { - clnors_invalid |= *clnor_ref != clnor; - } - else { - clnor_ref = &clnor; - } - clnors_avg += int2(clnor); - clnors_count++; - } - processed_corners.append(mlfan_vert_index); if (lnors_spacearr) { @@ -1018,6 +1001,9 @@ static void split_loop_nor_fan_do(LoopSplitTaskDataCommon *common_data, if (!lnors_spacearr->corners_by_space.is_empty()) { lnors_spacearr->corners_by_space[space_index] = processed_corners.as_span(); } + if (!clnors_data.is_empty()) { + clnors_avg += int2(clnors_data[mlfan_vert_index]); + } } if (IS_EDGE_SHARP(edge_to_loops[corner_edges[mlfan_curr_index]]) || (edge == edge_orig)) { @@ -1058,18 +1044,8 @@ static void split_loop_nor_fan_do(LoopSplitTaskDataCommon *common_data, edge_vectors->clear(); if (!clnors_data.is_empty()) { - if (clnors_invalid) { - clnors_avg /= clnors_count; - /* Fix/update all clnors of this fan with computed average value. */ - if (G.debug & G_DEBUG) { - printf("Invalid clnors in this fan!\n"); - } - clnors_data.fill_indices(processed_corners.as_span(), short2(clnors_avg)); - } - /* Extra bonus: since small-stack is local to this function, - * no more need to empty it at all cost! */ - - lnor = lnor_space_custom_data_to_normal(lnor_space, *clnor_ref); + clnors_avg /= processed_corners.size(); + lnor = lnor_space_custom_data_to_normal(lnor_space, short2(clnors_avg)); } } @@ -1238,9 +1214,9 @@ void normals_calc_loop(const Span vert_positions, const Span face_normals, const bool *sharp_edges, const bool *sharp_faces, + const short2 *clnors_data, bool use_split_normals, float split_angle, - short2 *clnors_data, CornerNormalSpaceArray *r_lnors_spacearr, MutableSpan r_loop_normals) { @@ -1314,8 +1290,7 @@ void normals_calc_loop(const Span vert_positions, LoopSplitTaskDataCommon common_data; common_data.lnors_spacearr = r_lnors_spacearr; common_data.loop_normals = r_loop_normals; - common_data.clnors_data = {reinterpret_cast(clnors_data), - clnors_data ? corner_verts.size() : 0}; + common_data.clnors_data = {clnors_data, clnors_data ? corner_verts.size() : 0}; common_data.positions = vert_positions; common_data.edges = edges; common_data.faces = faces; @@ -1425,9 +1400,9 @@ static void mesh_normals_loop_custom_set(Span positions, face_normals, sharp_edges.data(), sharp_faces, + r_clnors_data.data(), use_split_normals, split_angle, - r_clnors_data.data(), &lnors_spacearr, loop_normals); @@ -1547,9 +1522,9 @@ static void mesh_normals_loop_custom_set(Span positions, face_normals, sharp_edges.data(), sharp_faces, + r_clnors_data.data(), use_split_normals, split_angle, - r_clnors_data.data(), &lnors_spacearr, loop_normals); } diff --git a/source/blender/blenkernel/intern/mesh_remap.cc b/source/blender/blenkernel/intern/mesh_remap.cc index 0b1aed17a25..dab4444f61c 100644 --- a/source/blender/blenkernel/intern/mesh_remap.cc +++ b/source/blender/blenkernel/intern/mesh_remap.cc @@ -1343,8 +1343,8 @@ void BKE_mesh_remap_calc_loops_from_mesh(const int mode, face_normals_dst = mesh_dst->face_normals(); } if (need_lnors_dst) { - blender::short2 *custom_nors_dst = static_cast( - CustomData_get_layer_for_write(ldata_dst, CD_CUSTOMLOOPNORMAL, numloops_dst)); + const blender::short2 *custom_nors_dst = static_cast( + CustomData_get_layer(ldata_dst, CD_CUSTOMLOOPNORMAL)); /* Cache loop normals into a temporary custom data layer. */ loop_normals_dst = static_cast( @@ -1372,9 +1372,9 @@ void BKE_mesh_remap_calc_loops_from_mesh(const int mode, mesh_dst->face_normals(), sharp_edges, sharp_faces, + custom_nors_dst, use_split_nors_dst, split_angle_dst, - custom_nors_dst, nullptr, {loop_normals_dst, numloops_dst}); } diff --git a/source/blender/draw/intern/draw_cache_extract_mesh_render_data.cc b/source/blender/draw/intern/draw_cache_extract_mesh_render_data.cc index 70233c0e116..8935c131eb4 100644 --- a/source/blender/draw/intern/draw_cache_extract_mesh_render_data.cc +++ b/source/blender/draw/intern/draw_cache_extract_mesh_render_data.cc @@ -337,8 +337,8 @@ void mesh_render_data_update_normals(MeshRenderData *mr, const eMRDataType data_ } if (((data_flag & MR_DATA_LOOP_NOR) && is_auto_smooth) || (data_flag & MR_DATA_TAN_LOOP_NOR)) { mr->loop_normals.reinitialize(mr->corner_verts.size()); - blender::short2 *clnors = static_cast(CustomData_get_layer_for_write( - &mr->me->loop_data, CD_CUSTOMLOOPNORMAL, mr->me->totloop)); + const blender::short2 *clnors = static_cast( + CustomData_get_layer(&mr->me->loop_data, CD_CUSTOMLOOPNORMAL)); const bool *sharp_edges = static_cast( CustomData_get_layer_named(&mr->me->edge_data, CD_PROP_BOOL, "sharp_edge")); blender::bke::mesh::normals_calc_loop(mr->vert_positions, @@ -351,9 +351,9 @@ void mesh_render_data_update_normals(MeshRenderData *mr, const eMRDataType data_ mr->face_normals, sharp_edges, mr->sharp_faces, + clnors, is_auto_smooth, split_angle, - clnors, nullptr, mr->loop_normals); } diff --git a/source/blender/modifiers/intern/MOD_normal_edit.cc b/source/blender/modifiers/intern/MOD_normal_edit.cc index 6f62a59b6e4..974f1ac6e3e 100644 --- a/source/blender/modifiers/intern/MOD_normal_edit.cc +++ b/source/blender/modifiers/intern/MOD_normal_edit.cc @@ -553,9 +553,9 @@ static Mesh *normalEditModifier_do(NormalEditModifierData *enmd, result->face_normals(), sharp_edges.span.data(), sharp_faces, + clnors, true, result->smoothresh, - clnors, nullptr, loop_normals); } diff --git a/source/blender/modifiers/intern/MOD_weighted_normal.cc b/source/blender/modifiers/intern/MOD_weighted_normal.cc index 07956abf5fb..c6cb0a85afc 100644 --- a/source/blender/modifiers/intern/MOD_weighted_normal.cc +++ b/source/blender/modifiers/intern/MOD_weighted_normal.cc @@ -231,9 +231,9 @@ static void apply_weights_vertex_normal(WeightedNormalModifierData *wnmd, wn_data->face_normals, wn_data->sharp_edges.data(), wn_data->sharp_faces, + has_clnors ? clnors.data() : nullptr, true, split_angle, - has_clnors ? clnors.data() : nullptr, &lnors_spacearr, loop_normals); @@ -362,9 +362,9 @@ static void apply_weights_vertex_normal(WeightedNormalModifierData *wnmd, face_normals, wn_data->sharp_edges.data(), wn_data->sharp_faces, + has_clnors ? clnors.data() : nullptr, true, split_angle, - has_clnors ? clnors.data() : nullptr, nullptr, loop_normals);