griefith/test
1
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases Wiki Activity

Fix #109583: Avoid non-threadsafe writing to custom normals data

Browse Source 
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
This commit is contained in:
Hans Goudey
2023-07-26 17:04:13 +02:00
committed by Hans Goudey
parent 741c684bf6
commit 580833165c
10 changed files with 31 additions and 57 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
source/blender/blenkernel/BKE_mesh.hh
View File

@@ -156,9 +156,9 @@ void normals_calc_loop(Span<float3> vert_positions,
Span<float3> 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<float3> r_loop_normals);

6
source/blender/blenkernel/intern/data_transfer.cc
View File

@@ -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<blender::short2 *>(
CustomData_get_layer_for_write(ldata_dst, CD_CUSTOMLOOPNORMAL, me_dst->totloop));
const blender::short2 *custom_nors_dst = static_cast<const blender::short2 *>(
CustomData_get_layer(ldata_dst, CD_CUSTOMLOOPNORMAL));
/* Cache loop nors into a temp CDLayer. */
blender::float3 *loop_nors_dst = static_cast<blender::float3 *>(
@@ -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});
}

6
source/blender/blenkernel/intern/key.cc
View File

@@ -2281,8 +2281,8 @@ void BKE_keyblock_mesh_calc_normals(const KeyBlock *kb,
{reinterpret_cast<blender::float3 *>(vert_normals), mesh->totvert});
}
if (loop_normals_needed) {
blender::short2 *clnors = static_cast<blender::short2 *>(CustomData_get_layer_for_write(
&mesh->loop_data, CD_CUSTOMLOOPNORMAL, corner_verts.size()));
const blender::short2 *clnors = static_cast<const blender::short2 *>(
CustomData_get_layer(&mesh->loop_data, CD_CUSTOMLOOPNORMAL));
const bool *sharp_edges = static_cast<const bool *>(
CustomData_get_layer_named(&mesh->edge_data, CD_PROP_BOOL, "sharp_edge"));
const bool *sharp_faces = static_cast<const bool *>(
@@ -2298,9 +2298,9 @@ void BKE_keyblock_mesh_calc_normals(const KeyBlock *kb,
{reinterpret_cast<blender::float3 *>(face_normals), faces.size()},
sharp_edges,
sharp_faces,
clnors,
(mesh->flag & ME_AUTOSMOOTH) != 0,
mesh->smoothresh,
clnors,
nullptr,
{reinterpret_cast<blender::float3 *>(r_loop_normals), corner_verts.size()});
}

7
source/blender/blenkernel/intern/mesh.cc
View File

@@ -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<const blender::short2 *>(
CustomData_get_layer(&mesh->loop_data, CD_CUSTOMLOOPNORMAL));
const bool *sharp_edges = static_cast<const bool *>(
CustomData_get_layer_named(&mesh->edge_data, CD_PROP_BOOL, "sharp_edge"));
const bool *sharp_faces = static_cast<const bool *>(
@@ -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<float3 *>(r_corner_normals), mesh->totloop});
}

2
source/blender/blenkernel/intern/mesh_mirror.cc
View File

@@ -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);

47
source/blender/blenkernel/intern/mesh_normals.cc
View File

@@ -700,7 +700,6 @@ struct LoopSplitTaskDataCommon {
* different elements in the arrays. */
CornerNormalSpaceArray *lnors_spacearr;
MutableSpan<float3> loop_normals;
MutableSpan<short2> clnors_data;
/* Read-only. */
Span<float3> positions;
@@ -712,6 +711,7 @@ struct LoopSplitTaskDataCommon {
Span<int> loop_to_face;
Span<float3> face_normals;
Span<float3> vert_normals;
Span<short2> 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<float3> loop_normals = common_data->loop_normals;
MutableSpan<short2> clnors_data = common_data->clnors_data;
const Span<float3> positions = common_data->positions;
const Span<int2> edges = common_data->edges;
@@ -926,6 +925,7 @@ static void split_loop_nor_fan_do(LoopSplitTaskDataCommon *common_data,
const Span<int2> edge_to_loops = common_data->edge_to_loops;
const Span<int> loop_to_face = common_data->loop_to_face;
const Span<float3> face_normals = common_data->face_normals;
const Span<short2> 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<int, 8> 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<float3> vert_positions,
const Span<float3> 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<float3> r_loop_normals)
{
@@ -1314,8 +1290,7 @@ void normals_calc_loop(const Span<float3> 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<short2 *>(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<float3> 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<float3> 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);
}

6
source/blender/blenkernel/intern/mesh_remap.cc
View File

@@ -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<blender::short2 *>(
CustomData_get_layer_for_write(ldata_dst, CD_CUSTOMLOOPNORMAL, numloops_dst));
const blender::short2 *custom_nors_dst = static_cast<const blender::short2 *>(
CustomData_get_layer(ldata_dst, CD_CUSTOMLOOPNORMAL));
/* Cache loop normals into a temporary custom data layer. */
loop_normals_dst = static_cast<blender::float3 *>(
@@ -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});
}

6
source/blender/draw/intern/draw_cache_extract_mesh_render_data.cc
View File

@@ -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<blender::short2 *>(CustomData_get_layer_for_write(
&mr->me->loop_data, CD_CUSTOMLOOPNORMAL, mr->me->totloop));
const blender::short2 *clnors = static_cast<const blender::short2 *>(
CustomData_get_layer(&mr->me->loop_data, CD_CUSTOMLOOPNORMAL));
const bool *sharp_edges = static_cast<const bool *>(
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);
}

2
source/blender/modifiers/intern/MOD_normal_edit.cc
View File

@@ -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);
}

4
source/blender/modifiers/intern/MOD_weighted_normal.cc
View File

@@ -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);