Mesh: Reduce custom normal calculation memory usage #107592
|
|
@ -90,7 +90,7 @@ void normals_calc_poly_vert(Span<float3> vert_positions,
|
|||
* coordinate space used to convert normals between the "custom normal" #short2 representation and
|
||||
* a regular #float3 format.
|
||||
*/
|
||||
struct NormalFanSpace {
|
||||
struct CornerNormalSpace {
|
||||
/** Reference vector, orthogonal to corner normal. */
|
||||
float3 vec_ref;
|
||||
/** Third vector, orthogonal to corner normal and #vec_ref. */
|
||||
|
|
@ -102,22 +102,22 @@ struct NormalFanSpace {
|
|||
};
|
||||
|
|
||||
|
||||
/**
|
||||
* Storage for coordinate spaces and connection information during normal calculation.
|
||||
* Storage for corner fan coordinate spaces for an entire mesh.
|
||||
Campbell Barton
commented
The names The names `NormalFanSpaces` & `NormalFanSpace` are too similar, the old suffix "Array" wasn't exactly accurate either (as it's not just an array) but at least it was easily distinguishable.
|
||||
*/
|
||||
struct NormalFanSpaces {
|
||||
struct CornerNormalSpaceArray {
|
||||
/**
|
||||
* The normal coordinate spaces, potentially shared between multiple face corners in a smooth fan
|
||||
* connected to a vertex. Depending on the mesh (the amount of sharing / number of sharp edges /
|
||||
* size of each fan), there may be many fewer spaces than face corners, so they are stored in a
|
||||
* separate array.
|
||||
* connected to a vertex (and not per face corner). Depending on the mesh (the amount of sharing
|
||||
* / number of sharp edges / size of each fan), there may be many fewer spaces than face corners,
|
||||
* so they are stored in a separate array.
|
||||
|
Campbell Barton
commented
Worth mentioning this array isn't aligned to geometry and must be accessed via Worth mentioning this array isn't aligned to geometry and must be accessed via `corner_space_indices`.
|
||||
*/
|
||||
Array<NormalFanSpace> spaces;
|
||||
Array<CornerNormalSpace> spaces;
|
||||
|
||||
|
Campbell Barton
commented
Prefer to keep the term Doc-string should mention Prefer to keep the term `loop` here as a corner isn't obviously aligned with the a mesh loops. e.g. `loop_space_indices`
Doc-string should mention `-1` are used to references loops without a reference to a space-array.
Hans Goudey
commented
Moving away from the term "loop" in general applies here too IMO, the values here are per face corner. Moving away from the term "loop" in general applies here too IMO, the values here are per face corner.
Campbell Barton
commented
In this case the doc-string should note that this array is aligned with a meshes loops (even if logically it's a face corner the index is a loop index. When reading new code with fresh eyes - lookup tables should not clearly what indexes them and what the they index into ( In this case the doc-string should note that this array is aligned with a meshes loops (even if logically it's a face corner the index is a loop index. When reading new code with fresh eyes - lookup tables should not clearly what indexes them and what the they index into (`CornerNormalSpaceArray::spaces` in this case). The term `loop index` is currently a meaningful term which communicates alignment).
Campbell Barton
commented
Noting that these values are for each face corner isn't explicitly stating this is aligned with mesh loops which I think its worth noting. That -1 is used to define values as not yet having an entry in Noting that these values are for each face corner isn't explicitly stating this is aligned with mesh loops which I think its worth noting.
That -1 is used to define values as not yet having an entry in `spaces` (worth noting too), are there actually cases these values will be left at -1 once `CornerNormalSpaceArray` is initialized? If so mention under what condition that happens.
Hans Goudey
commented
To be honest, your use of "aligned with" is a bit confusing to me. It makes me think of memory address alignment, which is a different thing. But I'll make it more clear.
I don't know to be honest. Some existing comments mention that, but it seems a bit paranoid and maybe worth fixing if it happens? Either way, I'm still working on understanding the whole algorithm, so I'm not able to add more details yet. To be honest, your use of "aligned with" is a bit confusing to me. It makes me think of memory address alignment, which is a different thing. But I'll make it more clear.
>That -1 is used to define values as uninitialized (worth noting too), are there actually cases these values will be left at -1
I don't know to be honest. Some existing comments mention that, but it seems a bit paranoid and maybe worth fixing if it happens? Either way, I'm still working on understanding the whole algorithm, so I'm not able to add more details yet.
Campbell Barton
commented
It seems likely they're never -1 and this is just used to check initialization, since this is updating the existing comment, seems fine to keep as-is. It seems likely they're never -1 and this is just used to check initialization, since this is updating the existing comment, seems fine to keep as-is.
Campbell Barton
commented
The exact working I'm not so fussed about although I think it's reasonable to say the
Ah, in that case removing the comment or expanding on it is outside the scope of the patch. It seems possible they're never -1 and this is just used to check initialization. > To be honest, your use of "aligned with" is a bit confusing to me. It makes me think of memory address alignment, which is a different thing. But I'll make it more clear.
The exact working I'm not so fussed about although I think it's reasonable to say the `MDeformVert` array is aligned with the `vert_positions` (for example), in this case I think it's not talking about memory alignment.
Whatever the case, geometry indices might apply to vertices, edges, polygons, loops, looptris... I think it's reasonable that this is always clear if not from the name then the doc-string.
> >That -1 is used to define values as uninitialized (worth noting too), are there actually cases these values will be left at -1
>
> I don't know to be honest. Some existing comments mention that, but it seems a bit paranoid and maybe worth fixing if it happens? Either way, I'm still working on understanding the whole algorithm, so I'm not able to add more details yet.
Ah, in that case removing the comment or expanding on it is outside the scope of the patch. It seems possible they're never -1 and this is just used to check initialization.
|
||||
/** The index of the data in the #spaces array for each face corner. */
|
||||
Array<int> corner_space_indices;
|
||||
};
|
||||
|
Campbell Barton
commented
Prefer naming is based on the public function Prefer naming is based on the public function `BKE_lnor_space_custom_normal_to_data` (e.g. `lnor_space_custom_normal_to_data_impl`).
Hans Goudey
commented
Longer term the public functions would be replaced, there was just no need to expose these in Longer term the public functions would be replaced, there was just no need to expose these in `BKE_mesh.hh` yet. But that's fine with me for now.
|
||||
|
||||
void fan_space_custom_normal_to_data(const NormalFanSpace *lnor_space,
|
||||
void fan_space_custom_normal_to_data(const CornerNormalSpace *lnor_space,
|
||||
const float3 lnor_no_custom,
|
||||
const float custom_lnor[3],
|
||||
short r_clnor_data[2]);
|
||||
|
Campbell Barton
commented
The naming here introduces "fan_space_custom_normal" which reads as if it might be different from This could be more clearly names that it's a direct conversion from e.g. The naming here introduces "fan_space_custom_normal" which reads as if it might be different from `CornerNormalSpace` (as if there might be a _"fan space"_ and a _"corner normal space"_).
This could be more clearly names that it's a direct conversion from `CornerNormalSpace` to `CD_CUSTOMLOOPNORMAL`.
e.g. `corner_normal_space_to_clnor`.
Hans Goudey
commented
Ah, sorry, I meant to rename that and keep the same name as before like you suggested. Ah, sorry, I meant to rename that and keep the same name as before like you suggested.
|
||||
|
|
@ -146,7 +146,7 @@ void normals_calc_loop(Span<float3> vert_positions,
|
|||
bool use_split_normals,
|
||||
float split_angle,
|
||||
short2 *clnors_data,
|
||||
NormalFanSpaces *r_lnors_spacearr,
|
||||
CornerNormalSpaceArray *r_lnors_spacearr,
|
||||
MutableSpan<float3> r_loop_normals);
|
||||
|
||||
void normals_loop_custom_set(Span<float3> vert_positions,
|
||||
|
|
|
|||
|
|
@ -390,7 +390,7 @@ Mesh *BKE_mesh_mirror_apply_mirror_on_axis_for_modifier(MirrorModifierData *mmd,
|
|||
CustomData *ldata = &result->ldata;
|
||||
blender::short2 *clnors = static_cast<blender::short2 *>(
|
||||
CustomData_get_layer_for_write(ldata, CD_CUSTOMLOOPNORMAL, result->totloop));
|
||||
blender::bke::mesh::NormalFanSpaces lnors_spacearr;
|
||||
blender::bke::mesh::CornerNormalSpaceArray lnors_spacearr;
|
||||
|
||||
/* The transform matrix of a normal must be
|
||||
* the transpose of inverse of transform matrix of the geometry... */
|
||||
|
|
|
|||
|
|
@ -468,7 +468,7 @@ MLoopNorSpace *BKE_lnor_space_create(MLoopNorSpaceArray *lnors_spacearr)
|
|||
#define LNOR_SPACE_TRIGO_THRESHOLD (1.0f - 1e-4f)
|
||||
|
||||
namespace blender::bke::mesh {
|
||||
static void normal_fan_space_define(NormalFanSpace *lnor_space,
|
||||
static void normal_fan_space_define(CornerNormalSpace *lnor_space,
|
||||
const float lnor[3],
|
||||
float vec_ref[3],
|
||||
float vec_other[3],
|
||||
|
|
@ -540,7 +540,7 @@ void BKE_lnor_space_define(MLoopNorSpace *lnor_space,
|
|||
const blender::Span<blender::float3> edge_vectors)
|
||||
{
|
||||
using namespace blender::bke::mesh;
|
||||
NormalFanSpace space{};
|
||||
CornerNormalSpace space{};
|
||||
normal_fan_space_define(&space, lnor, vec_ref, vec_other, edge_vectors);
|
||||
copy_v3_v3(lnor_space->vec_lnor, lnor);
|
||||
copy_v3_v3(lnor_space->vec_ref, space.vec_ref);
|
||||
|
|
@ -585,7 +585,7 @@ MINLINE short unit_float_to_short(const float val)
|
|||
}
|
||||
|
||||
namespace blender::bke::mesh {
|
||||
static void fan_space_custom_data_to_normal(const NormalFanSpace *lnor_space,
|
||||
static void fan_space_custom_data_to_normal(const CornerNormalSpace *lnor_space,
|
||||
|
Campbell Barton
commented
Prefer old name (can remove the BKE prefix). Prefer old name (can remove the BKE prefix).
|
||||
const float3 lnor_no_custom,
|
||||
const short clnor_data[2],
|
||||
float r_custom_lnor[3])
|
||||
|
|
@ -626,7 +626,7 @@ void BKE_lnor_space_custom_data_to_normal(const MLoopNorSpace *lnor_space,
|
|||
float r_custom_lnor[3])
|
||||
{
|
||||
using namespace blender::bke::mesh;
|
||||
NormalFanSpace space;
|
||||
CornerNormalSpace space;
|
||||
space.vec_ref = lnor_space->vec_ref;
|
||||
space.vec_ortho = lnor_space->vec_ortho;
|
||||
space.ref_alpha = lnor_space->ref_alpha;
|
||||
|
|
@ -635,7 +635,7 @@ void BKE_lnor_space_custom_data_to_normal(const MLoopNorSpace *lnor_space,
|
|||
}
|
||||
|
||||
namespace blender::bke::mesh {
|
||||
void fan_space_custom_normal_to_data(const NormalFanSpace *lnor_space,
|
||||
void fan_space_custom_normal_to_data(const CornerNormalSpace *lnor_space,
|
||||
const float3 lnor_no_custom,
|
||||
const float custom_lnor[3],
|
||||
short r_clnor_data[2])
|
||||
|
|
@ -695,7 +695,7 @@ void BKE_lnor_space_custom_normal_to_data(const MLoopNorSpace *lnor_space,
|
|||
short r_clnor_data[2])
|
||||
{
|
||||
using namespace blender::bke::mesh;
|
||||
NormalFanSpace space;
|
||||
CornerNormalSpace space;
|
||||
space.vec_ref = lnor_space->vec_ref;
|
||||
space.vec_ortho = lnor_space->vec_ortho;
|
||||
space.ref_alpha = lnor_space->ref_alpha;
|
||||
|
|
@ -709,7 +709,7 @@ struct LoopSplitTaskDataCommon {
|
|||
/* Read/write.
|
||||
* Note we do not need to protect it, though, since two different tasks will *always* affect
|
||||
* different elements in the arrays. */
|
||||
NormalFanSpaces *lnors_spacearr;
|
||||
CornerNormalSpaceArray *lnors_spacearr;
|
||||
MutableSpan<float3> loop_normals;
|
||||
MutableSpan<short2> clnors_data;
|
||||
|
||||
|
|
@ -889,7 +889,7 @@ static void lnor_space_for_single_fan(LoopSplitTaskDataCommon *common_data,
|
|||
|
||||
loop_normals[ml_curr_index] = poly_normals[loop_to_poly[ml_curr_index]];
|
||||
|
||||
if (NormalFanSpaces *lnors_spacearr = common_data->lnors_spacearr) {
|
||||
if (CornerNormalSpaceArray *lnors_spacearr = common_data->lnors_spacearr) {
|
||||
const Span<float3> positions = common_data->positions;
|
||||
const Span<int2> edges = common_data->edges;
|
||||
const OffsetIndices polys = common_data->polys;
|
||||
|
|
@ -912,7 +912,7 @@ static void lnor_space_for_single_fan(LoopSplitTaskDataCommon *common_data,
|
|||
sub_v3_v3v3(vec_prev, positions[vert_3], positions[vert_pivot]);
|
||||
normalize_v3(vec_prev);
|
||||
|
||||
NormalFanSpace *lnor_space = &lnors_spacearr->spaces[space_index];
|
||||
CornerNormalSpace *lnor_space = &lnors_spacearr->spaces[space_index];
|
||||
normal_fan_space_define(lnor_space, loop_normals[ml_curr_index], vec_curr, vec_prev, {});
|
||||
lnors_spacearr->corner_space_indices[ml_curr_index] = space_index;
|
||||
|
||||
|
|
@ -930,7 +930,7 @@ static void split_loop_nor_fan_do(LoopSplitTaskDataCommon *common_data,
|
|||
const int space_index,
|
||||
Vector<float3> *edge_vectors)
|
||||
{
|
||||
NormalFanSpaces *lnors_spacearr = common_data->lnors_spacearr;
|
||||
CornerNormalSpaceArray *lnors_spacearr = common_data->lnors_spacearr;
|
||||
MutableSpan<float3> loop_normals = common_data->loop_normals;
|
||||
MutableSpan<short2> clnors_data = common_data->clnors_data;
|
||||
|
||||
|
|
@ -1072,7 +1072,7 @@ static void split_loop_nor_fan_do(LoopSplitTaskDataCommon *common_data,
|
|||
length = 1.0f;
|
||||
}
|
||||
|
||||
NormalFanSpace *lnor_space = &lnors_spacearr->spaces[space_index];
|
||||
CornerNormalSpace *lnor_space = &lnors_spacearr->spaces[space_index];
|
||||
normal_fan_space_define(lnor_space, lnor, vec_org, vec_curr, *edge_vectors);
|
||||
lnors_spacearr->corner_space_indices.as_mutable_span().fill_indices(
|
||||
processed_corners.as_span(), space_index);
|
||||
|
|
@ -1270,7 +1270,7 @@ void normals_calc_loop(const Span<float3> vert_positions,
|
|||
bool use_split_normals,
|
||||
float split_angle,
|
||||
short2 *clnors_data,
|
||||
NormalFanSpaces *r_lnors_spacearr,
|
||||
CornerNormalSpaceArray *r_lnors_spacearr,
|
||||
MutableSpan<float3> r_loop_normals)
|
||||
{
|
||||
/* For now this is not supported.
|
||||
|
|
@ -1328,7 +1328,7 @@ void normals_calc_loop(const Span<float3> vert_positions,
|
|||
/* When using custom loop normals, disable the angle feature! */
|
||||
const bool check_angle = (split_angle < float(M_PI)) && (clnors_data == nullptr);
|
||||
|
||||
NormalFanSpaces _lnors_spacearr;
|
||||
CornerNormalSpaceArray _lnors_spacearr;
|
||||
|
||||
#ifdef DEBUG_TIME
|
||||
SCOPED_TIMER_AVERAGED(__func__);
|
||||
|
|
@ -1457,7 +1457,7 @@ static void mesh_normals_loop_custom_set(Span<float3> positions,
|
|||
* function *is not* performance-critical, since it is mostly expected to be called by io add-ons
|
||||
* when importing custom normals, and modifier (and perhaps from some editing tools later?). So
|
||||
* better to keep some simplicity here, and just call #bke::mesh::normals_calc_loop() twice! */
|
||||
NormalFanSpaces lnors_spacearr;
|
||||
CornerNormalSpaceArray lnors_spacearr;
|
||||
BitVector<> done_loops(corner_verts.size(), false);
|
||||
Array<float3> loop_normals(corner_verts.size());
|
||||
const Array<int> loop_to_poly = mesh_topology::build_loop_to_poly_map(polys);
|
||||
|
|
|
|||
|
|
@ -205,7 +205,7 @@ static void apply_weights_vertex_normal(WeightedNormalModifierData *wnmd,
|
|||
|
||||
const bool has_clnors = wn_data->has_clnors;
|
||||
const float split_angle = wn_data->split_angle;
|
||||
bke::mesh::NormalFanSpaces lnors_spacearr;
|
||||
bke::mesh::CornerNormalSpaceArray lnors_spacearr;
|
||||
|
||||
const bool keep_sharp = (wnmd->flag & MOD_WEIGHTEDNORMAL_KEEP_SHARP) != 0;
|
||||
const bool use_face_influence = (wnmd->flag & MOD_WEIGHTEDNORMAL_FACE_INFLUENCE) != 0 &&
|
||||
|
|
|
|||
Worth mentioning some things that might seem obvious, that this stores normal data for an entire mesh.