Mesh: Reduce custom normal calculation memory usage #107592
No reviewers
Labels
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
3 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#107592
Loading…
Reference in New Issue
No description provided.
Delete Branch "HooglyBoogly/blender:corner-normals-refactor-storage"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Avoid storing redundant and unnecessary data in temporary arrays during
face corner normal calculation with custom normals. Previously we used
96 bytes per normal space (
MLoopNorSpace
(64),MLoopNorSpace *
(8),LinkData
(24)), now we use 36 (CornerNormalSpace
(32),int
(4)).This is achieved with a few changes:
Reducing memory usage gives a significant performance improvement in
my test files, which is consistent with findings from previous commits
(see
9fcfba4aae
). In my test, the time used to calculatenormals for a character model with 196 thousand faces reduced from
20.2 ms to 14.0 ms, a 44% improvement.
Edit mode isn't affected by this change.
A note about the
reverse_index_array
function added here: this is thesame as the mesh mapping functions (for example for mapping from
vertices to face corners). I'm planning on working this area and hoping
to generalize/reuse the implementation in the near future.
@mont29 I added you as a reviewer since I'm adding a new storage type for the normal fan spaces-- the changes are a bit more "public" than my past ones in this area. If you don't have an opinion or the time to check it out, that's fine too.
@blender-bot build
@HooglyBoogly thanks for the heads up, from the patch description it looks like a nice improvement!
But indeed am not in touch with this area of code anymore, so would rather let the Modeling module handle the review.
In general I trust this is working properly, but I find the new naming confusing and would prefer to stick to the existing names.
A "normal fan" is ambiguous, "fan space" too - while reading the code I'm wondering a normal for what? a fan around what... and what is it aligned to?
If it were a generic type which we would use in different situations, that could work, but this is a spesific type for calculating loop-normals. While
lnor
is an odd name it's terse and unambiguous. So I'd prefer to keep it unless a new convention is proposed (including renaming DNA MLoopNor ... etc), although that can be separate from this patch.lnor_
prefix for functions that deal with loop normals.MLoopNorSpaceArray
&MLoopNorSpace
names.@ -80,0 +99,4 @@
float ref_beta;
};
/**
Worth mentioning some things that might seem obvious, that this stores normal data for an entire mesh.
@ -80,0 +102,4 @@
/**
* Storage for coordinate spaces and connection information during normal calculation.
*/
struct NormalFanSpaces {
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.@ -80,0 +109,4 @@
* size of each fan), there may be many fewer spaces than face corners, so they are stored in a
* separate array.
*/
Array<NormalFanSpace> spaces;
Worth mentioning this array isn't aligned to geometry and must be accessed via
corner_space_indices
.@ -80,0 +112,4 @@
Array<NormalFanSpace> spaces;
/** The index of the data in the #spaces array for each face corner. */
Array<int> corner_space_indices;
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.Moving away from the term "loop" in general applies here too IMO, the values here are per face corner.
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 termloop index
is currently a meaningful term which communicates alignment).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 onceCornerNormalSpaceArray
is initialized? If so mention under what condition that happens.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.
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.
The exact working I'm not so fussed about although I think it's reasonable to say the
MDeformVert
array is aligned with thevert_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.
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.
@ -80,0 +115,4 @@
Array<int> corner_space_indices;
};
void fan_space_custom_normal_to_data(const NormalFanSpace *lnor_space,
Prefer naming is based on the public function
BKE_lnor_space_custom_normal_to_data
(e.g.lnor_space_custom_normal_to_data_impl
).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.@ -572,2 +587,2 @@
const short clnor_data[2],
float r_custom_lnor[3])
namespace blender::bke::mesh {
static void fan_space_custom_data_to_normal(const NormalFanSpace *lnor_space,
Prefer old name (can remove the BKE prefix).
Thanks for the feedback. I did the changes you mentioned but went in a slightly different direction. Using the old struct names is confusing, since their contents is purposely different than the old versions (which remain used by BMesh, to be clear). I went with
CornerNormalSpaceArray
andCornerNormalSpace
, since these are new structs, it doesn't make sense to useMLoop
instead ofCorner
, andNormal
is just nicer thanNor
. I'm trying to get consistent naming in theBKE_mesh.hh
header, and while the "loop" to "corner" transition is a big one that isn't finished in full, I think it makes sense to do in these new structs.@ -82,0 +120,4 @@
Array<int> corner_space_indices;
};
void fan_space_custom_normal_to_data(const CornerNormalSpace *lnor_space,
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
toCD_CUSTOMLOOPNORMAL
.e.g.
corner_normal_space_to_clnor
.Ah, sorry, I meant to rename that and keep the same name as before like you suggested.
@ -1367,0 +1408,4 @@
* item. Group each item's indices together consecutively, encoding the grouping in #r_offsets,
* which is meant to be used by #OffsetIndices.
*/
static void reverse_index_array(const Span<int> item_indices,
It's not obvious what
r_offsets
&r_reverse_indices
represent, which should be stated in plan text (in this doc-string).Or, wrap
reverse_index_array
in a function which takeslnors_spacearr
and include a description of what the values represent there.Either way noting that
fan_corners_data
&fan_corner_offset_indices
represent inline isn't practical as they're declared twice.@ -1546,3 +1617,3 @@
}
Vector<int> processed_corners;
Array<int> fan_corner_offset_indices;
These names read as generic face-corner information when they are quite spesific and only useful when used together.
That these values are referenced by the space index would make this clearer, e.g.
fan_corners_from_space_data
fan_corners_from_space_offset_indices
@blender-bot build