blender
/
blender
136
406
Fork 588
Code Issues 6.2k Pull Requests 751 Projects 19 Wiki Activity

Mesh: Reduce custom normal calculation memory usage #107592

Merged
Hans Goudey merged 32 commits from HooglyBoogly/blender:corner-normals-refactor-storage into main 2023-05-10 14:41:16 +02:00
Conversation 5 Commits 32 Files Changed 4 +2 -2
1 changed files with 2 additions and 2 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit f1f92faa77 - Show all commits

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

@ -114,8 +114,8 @@ struct CornerNormalSpaceArray {
Array<CornerNormalSpace> spaces;
Campbell Barton commented 2023-05-05 09:01:16 +02:00
Review
Copy Link

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.

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 2023-05-07 05:43:51 +02:00
Review
Copy Link

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 2023-05-08 02:24:33 +02:00
Review
Copy Link

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).

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 2023-05-08 02:24:33 +02:00
Review
Copy Link

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.

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 2023-05-08 03:23:33 +02:00
Review
Copy Link

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.

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 2023-05-10 06:33:49 +02:00
Review
Copy Link

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 2023-05-10 06:40:57 +02:00
Review
Copy Link

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.

> 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.
👍 1
/**
* The index of the data in the #spaces array for each face corner.
* Rare -1 values define face corners without a coordinate space.
* The index of the data in the #spaces array for each face corner (the array size is the
* same as #Mesh::totloop). Rare -1 values define face corners without a coordinate space.
Campbell Barton commented 2023-05-05 09:34:38 +02:00
Outdated
Review
Copy Link

Prefer naming is based on the public function BKE_lnor_space_custom_normal_to_data (e.g. lnor_space_custom_normal_to_data_impl).

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 2023-05-07 05:44:34 +02:00
Outdated
Review
Copy Link

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.

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.
*/
Array<int> corner_space_indices;
};