Mesh: Replace MPoly struct with offset indices #105938

Merged
Hans Goudey merged 423 commits from refactor-mesh-face-generic into main 2023-04-04 20:39:42 +02:00
Member

Implements #95967.

Currently the MPoly struct is 12 bytes, and stores the index of a
face's first corner and the number of corners/verts/edges. Polygons
and corners are always created in order by Blender, meaning each
face's corners will be after the previous face's corners. We can take
advantage of this fact and eliminate the redundancy in mesh face
storage by only storing a single integer corner offset for each face.
The size of the face is then encoded by the offset of the next face.
The size of a single integer is 4 bytes, so this reduces memory
usage by 3 times.

The same method is used for CurvesGeometry, so Blender already has
an abstraction to simplify using these offsets called OffsetIndices.
This class is used to easily retrieve a range of corner indices for
each face. This also gives the opportunity for sharing some logic with
curves.

Another benefit of the change is that the offsets and sizes stored in
MPoly can no longer disagree with each other. Storing faces in the
order of their corners can simplify some code too.

Face/polygon variables now use the IndexRange type, which comes with
quite a few utilities that can simplify code.

Implementation notes:

  • The offset integer array has to be one longer than the face count to
    avoid a branch for every face, which means the data is no longer part
    of the mesh's CustomData.
  • We lose the ability to "reference" an original mesh's offset array
    until more reusable CoW from #104478 is committed. That will be added
    in a separate commit.
  • Since they aren't part of CustomData, poly offsets often have to be
    copied manually.
  • To simplify using OffsetIndices in many places, some functions and
    structs in headers were moved to only compile in C++.
  • All meshes created by Blender use the same order for faces and face
    corners, but just in case, meshes with mismatched order are fixed by
    versioning code.
  • MeshPolygon.totloop is no longer editable in RNA. Some breakage is
    necessary here unfortunately, and it should be worth it in 3.6, since
    that's the best way to allow loading meshes from 4.0.
Implements #95967. Currently the `MPoly` struct is 12 bytes, and stores the index of a face's first corner and the number of corners/verts/edges. Polygons and corners are always created in order by Blender, meaning each face's corners will be after the previous face's corners. We can take advantage of this fact and eliminate the redundancy in mesh face storage by only storing a single integer corner offset for each face. The size of the face is then encoded by the offset of the next face. The size of a single integer is 4 bytes, so this reduces memory usage by 3 times. The same method is used for `CurvesGeometry`, so Blender already has an abstraction to simplify using these offsets called `OffsetIndices`. This class is used to easily retrieve a range of corner indices for each face. This also gives the opportunity for sharing some logic with curves. Another benefit of the change is that the offsets and sizes stored in `MPoly` can no longer disagree with each other. Storing faces in the order of their corners can simplify some code too. Face/polygon variables now use the `IndexRange` type, which comes with quite a few utilities that can simplify code. Implementation notes: - The offset integer array has to be one longer than the face count to avoid a branch for every face, which means the data is no longer part of the mesh's `CustomData`. - We lose the ability to "reference" an original mesh's offset array until more reusable CoW from #104478 is committed. That will be added in a separate commit. - Since they aren't part of `CustomData`, poly offsets often have to be copied manually. - To simplify using `OffsetIndices` in many places, some functions and structs in headers were moved to only compile in C++. - All meshes created by Blender use the same order for faces and face corners, but just in case, meshes with mismatched order are fixed by versioning code. - `MeshPolygon.totloop` is no longer editable in RNA. Some breakage is necessary here unfortunately, and it should be worth it in 3.6, since that's the best way to allow loading meshes from 4.0.
Hans Goudey added 392 commits 2023-03-20 20:44:50 +01:00
9726bb4e51 Initial progress splitting MLoop in two arrays
About halfway through removing all uses of `MLoop`. See T102359
d300fe01ac Cleanup: Make naming more consistent
Avoid _i prefix which doesn't really help
c94f259fe4 Start refactor of MPoly
TODO:
- multires_reshape.h to C++
- `MeshRenderData` nontrivial
- BKE_mesh_tangent.h to C++
- Remove `ps_tri_index_to_mpoly`
- Remove `BKE_mesh_polys_flip`
- All sculpt and pbvh files to C++ (SculptSession) (PBVH)
c573a1fcf9 Return an empty span for poly offsets when there are no polys
Usually the size is increased by one for the offsets, but to avoid
returning invalid spans, return an empty span. This is generally
more convenient too. It's different from curves because curves
with points but no curves are already invalid
buildbot/vexp-code-patch-coordinator Build done. Details
b7071bdaa5
Merge branch 'main' into refactor-mesh-corners-generic
buildbot/vexp-code-patch-coordinator Build done. Details
9959b431dc
Small cleanups and variable renames
buildbot/vexp-code-patch-coordinator Build done. Details
806c304666
Cleanup: Use poly_verts helper variable
buildbot/vexp-code-patch-coordinator Build done. Details
1e217a1083
Merge branch 'main' into refactor-mesh-corners-generic
buildbot/vexp-code-patch-coordinator Build done. Details
bbb16acd62
Add info to comment
Hans Goudey added the
Module
Nodes & Physics
Interest
Modeling
labels 2023-03-20 20:45:43 +01:00
Hans Goudey added this to the Modeling project 2023-03-20 20:45:45 +01:00
Hans Goudey added 1 commit 2023-03-20 21:05:32 +01:00
buildbot/vexp-code-patch-coordinator Build done. Details
3598ee1f8e
Fix build error from missing include
Hans Goudey added 1 commit 2023-03-21 01:07:19 +01:00
Hans Goudey added 1 commit 2023-03-22 18:38:06 +01:00
buildbot/vexp-code-patch-coordinator Build done. Details
04ae5967f9
Attempt to fix build error
Hans Goudey added 1 commit 2023-03-22 19:53:40 +01:00
Hans Goudey added 4 commits 2023-03-22 23:01:26 +01:00
Hans Goudey requested review from Campbell Barton 2023-03-22 23:01:55 +01:00
Hans Goudey requested review from Jacques Lucke 2023-03-22 23:01:55 +01:00
Hans Goudey added 2 commits 2023-03-24 13:20:26 +01:00
Hans Goudey added this to the 3.6 LTS milestone 2023-03-27 16:13:16 +02:00
Hans Goudey added 2 commits 2023-03-28 01:34:40 +02:00
Hans Goudey added 1 commit 2023-03-29 17:11:48 +02:00
Hans Goudey added 1 commit 2023-03-29 19:39:39 +02:00
Hans Goudey added 2 commits 2023-03-30 14:03:01 +02:00
Hans Goudey added 1 commit 2023-03-30 14:08:57 +02:00
Member

I found one issue related to undo. To reproduce the crash, open Blender, duplicate the default cube and undo. I couldn't figure out the root cause yet. The crash happens because mesh->poly_offsets_data is null after the undo. For some reason BLO_read_int32_array sets the pointer to null, even though the array seems to be written correctly in mesh_blend_write.

What do you think about renaming poly_offsets_data to poly_offset_indices?

I found one issue related to undo. To reproduce the crash, open Blender, duplicate the default cube and undo. I couldn't figure out the root cause yet. The crash happens because `mesh->poly_offsets_data` is null after the undo. For some reason `BLO_read_int32_array` sets the pointer to null, even though the array seems to be written correctly in `mesh_blend_write`. What do you think about renaming `poly_offsets_data` to `poly_offset_indices`?
Hans Goudey added 2 commits 2023-03-30 17:32:29 +02:00
Hans Goudey added 1 commit 2023-03-30 18:04:39 +02:00
Hans Goudey added 1 commit 2023-03-30 19:13:42 +02:00
025dbfde2d Fix issue with writng in non-legacy format
I have not much clue why (this thing is really confusing), but we need
to write the actual geometry data after writing the ID struct. Otherwise
the write seems to fail (the data is not readable later).
Author
Member

I found one issue related to undo.

I finally found the issue. For some reason the order of operations matters a lot when writing a mesh. I think I understood why last year at some point but now I forget, it's pretty weird.

What do you think about renaming poly_offsets_data to poly_offset_indices?

Good idea, done.

>I found one issue related to undo. I finally found the issue. For some reason the order of operations matters a lot when writing a mesh. I think I understood why last year at some point but now I forget, it's pretty weird. >What do you think about renaming poly_offsets_data to poly_offset_indices? Good idea, done.
Member

Ah good find, for some reason I thought that we always write the id struct at the beginning of the function, but didn't check here.
I believe the reason is that the old-new-map is created per ID. Everything that comes after an ID block in the file belongs to that ID, until the next one starts.

Ah good find, for some reason I thought that we always write the id struct at the beginning of the function, but didn't check here. I believe the reason is that the old-new-map is created per ID. Everything that comes after an ID block in the file belongs to that ID, until the next one starts.
Hans Goudey added 1 commit 2023-03-30 19:24:27 +02:00
Hans Goudey added 2 commits 2023-04-03 17:31:07 +02:00
Member

@blender-bot build

@blender-bot build
Jacques Lucke approved these changes 2023-04-03 18:37:14 +02:00
Jacques Lucke left a comment
Member

Could not find another issue in my tests.

Could not find another issue in my tests.
Hans Goudey added 1 commit 2023-04-03 19:02:42 +02:00
Campbell Barton approved these changes 2023-04-04 14:39:10 +02:00
Campbell Barton left a comment
Owner

Noted some issues inline, otherwise LGTM, no need for an extra review cycle.

Noted some issues inline, otherwise LGTM, no need for an extra review cycle.
@ -944,6 +957,23 @@ Mesh *BKE_mesh_add(Main *bmain, const char *name)
return me;
}
void BKE_mesh_poly_offsets_ensure(Mesh *mesh)

Suggestion: "Ensure" functions typically create the data and fill it with valid values.

This function just ensures the memory is allocated. Perhaps this could be called BKE_mesh_poly_offsets_ensure_alloc or BKE_mesh_poly_offsets_ensure_uninitialized.

Suggestion: "Ensure" functions typically create the data and fill it with valid values. This function just ensures the memory is allocated. Perhaps this could be called `BKE_mesh_poly_offsets_ensure_alloc` or `BKE_mesh_poly_offsets_ensure_uninitialized`.
HooglyBoogly marked this conversation as resolved
@ -2199,0 +2261,4 @@
});
CustomData old_poly_data = mesh->pdata;
CustomData_reset(&mesh->pdata);
CustomData_copy(&old_poly_data, &mesh->pdata, CD_MASK_MESH.lmask, CD_CONSTRUCT, mesh->totloop);

Shouldn't pmask and totpoly be used here?

Shouldn't `pmask` and `totpoly` be used here?
HooglyBoogly marked this conversation as resolved
Hans Goudey added 4 commits 2023-04-04 20:35:00 +02:00
Hans Goudey added 2 commits 2023-04-04 20:35:36 +02:00
Hans Goudey merged commit 7966cd16d6 into main 2023-04-04 20:39:42 +02:00
Hans Goudey deleted branch refactor-mesh-face-generic 2023-04-04 20:39:43 +02:00
Sign in to join this conversation.
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#105938
No description provided.