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
Member

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:

  • Avoid sharing the data storage with the BMesh implementation
  • Use indices to refer to normal fan spaces rather than pointers
  • Only calculate indices of all corners in each fan when necessary
  • Don't duplicate automatic normal in space storage
  • Avoid storing redundant flags in space struct

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 calculate
normals 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 the
same 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.

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: - Avoid sharing the data storage with the BMesh implementation - Use indices to refer to normal fan spaces rather than pointers - Only calculate indices of all corners in each fan when necessary - Don't duplicate automatic normal in space storage - Avoid storing redundant flags in space struct Reducing memory usage gives a significant performance improvement in my test files, which is consistent with findings from previous commits (see 9fcfba4aae7982327072). In my test, the time used to calculate normals 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 the same 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.
Hans Goudey added 20 commits 2023-05-03 23:45:23 +02:00
Hans Goudey requested review from Bastien Montagne 2023-05-03 23:45:41 +02:00
Hans Goudey added this to the Modeling project 2023-05-03 23:45:46 +02:00
Hans Goudey added the
Interest
Performance
label 2023-05-03 23:45:51 +02:00
Author
Member

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

@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.
Hans Goudey added 1 commit 2023-05-04 00:16:50 +02:00
Author
Member

@blender-bot build

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

@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.
Bastien Montagne refused to review 2023-05-04 10:01:11 +02:00
Hans Goudey requested review from Campbell Barton 2023-05-04 14:30:23 +02:00
Campbell Barton requested changes 2023-05-05 09:39:32 +02:00
Campbell Barton left a comment
Owner

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.

  • Keep lnor_ prefix for functions that deal with loop normals.
  • Keep MLoopNorSpaceArray & MLoopNorSpace names.
  • Other changes mentioned inline.
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. - Keep `lnor_` prefix for functions that deal with loop normals. - Keep `MLoopNorSpaceArray` & `MLoopNorSpace` names. - Other changes mentioned inline.
@ -80,0 +99,4 @@
float ref_beta;
};
/**

Worth mentioning some things that might seem obvious, that this stores normal data for an entire mesh.

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.

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.

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.

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.
Author
Member

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.

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

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.
Author
Member

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.

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.

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

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

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

Prefer old name (can remove the BKE prefix).
Hans Goudey added 4 commits 2023-05-07 05:47:38 +02:00
Author
Member

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 and CornerNormalSpace, since these are new structs, it doesn't make sense to use MLoop instead of Corner, and Normal is just nicer than Nor. I'm trying to get consistent naming in the BKE_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.

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` and `CornerNormalSpace`, since these are new structs, it doesn't make sense to use `MLoop` instead of `Corner`, and `Normal` is just nicer than `Nor`. I'm trying to get consistent naming in the `BKE_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.
Hans Goudey requested review from Campbell Barton 2023-05-07 15:19:15 +02:00
Campbell Barton requested changes 2023-05-08 03:17:53 +02:00
@ -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 to CD_CUSTOMLOOPNORMAL.

e.g. corner_normal_space_to_clnor.

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`.
Author
Member

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.
@ -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 takes lnors_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.

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 takes `lnors_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

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`
Hans Goudey added 5 commits 2023-05-08 03:40:44 +02:00
Hans Goudey requested review from Campbell Barton 2023-05-08 03:40:51 +02:00
Hans Goudey added 1 commit 2023-05-08 03:42:48 +02:00
Hans Goudey added 1 commit 2023-05-09 01:51:34 +02:00
Campbell Barton approved these changes 2023-05-10 06:41:58 +02:00
Author
Member

@blender-bot build

@blender-bot build
Hans Goudey merged commit 17d161f565 into main 2023-05-10 14:41:16 +02:00
Hans Goudey deleted branch corner-normals-refactor-storage 2023-05-10 14:41:17 +02:00
Howard Trickey referenced this issue from a commit 2023-05-29 02:51:41 +02:00
Sign in to join this conversation.
No reviewers
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
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#107592
No description provided.