Details

Reviewers

Maniphest Tasks

T95966: Struct of Arrays Refactor for Mesh Edges

Commits

rB1ea169d90e39: Mesh: Move loose edge flag to a separate cache

Summary

As part of T95966, this patch moves loose edge information out of the
flag on each edge and into a new lazily calculated cache in mesh
runtime data. The number of loose edges is also cached, so further
processing can be skipped completely when there are no loose edges.

Previously the ME_LOOSEEDGE flag was updated on a "best effort"
basis. In order to be sure that it was correct, you had to be sure
to call BKE_mesh_calc_edges_loose first. Now the loose edge tag
is always correct. It also doesn't have to be calculated eagerly
in various places like the screw modifier where the complexity
wasn't worth the theoretical performance benefit.

There is also an API function to eagerly set the number of loose
edges to zero to avoid building the cache. This is used by various
primitive nodes, mainly with the goal of improving drawing performance.
This results in a few ms shaved off extracting draw data for some
large meshes in my tests.

In the Python API, MeshEdge.is_loose is no longer an editable
property. No built-in addons set its value anyway. The upside is that
addons can be sure the data is correct based on the mesh when they
retrieve it.

After this, there are only three more flags to move before we can save
1/3 of the memory usage for edges in most meshes.

Note: This patch has similarities with D16530. Some cleanup can be done if that is committed first.

Tests
There is one test failure in the Python OBJ exporter: export_obj_cube.
However, that happens because of incorrect versioning. Opening the
file, all the edges are set to "loose". This patch fixes that issue.

Diff Detail

Repository: rB Blender

Event Timeline

Hans Goudey (HooglyBoogly) requested review of this revision.Nov 14 2022, 11:51 PM

Hans Goudey (HooglyBoogly) created this revision.

Add comments to API functions and cache struct

The export_obj_cube test seems to fail for me.

https://builder.blender.org/admin/#/builders/136/builds/227

source/blender/blenkernel/BKE_mesh_types.h
71	less or equal to zero
source/blender/blenkernel/intern/mesh_runtime.cc
70	Looks like this might decrement the same edge multiple times.

This revision now requires changes to proceed.Nov 15 2022, 12:00 PM

Improve comment
Fix decrementing count for edge multiple times

Jacques Lucke (JacquesLucke) added inline comments.Nov 16 2022, 5:14 PM

source/blender/blenkernel/BKE_mesh_types.h
70	Maybe `is_loose_bits`?
source/blender/blenkernel/intern/mesh_runtime.cc
60	Will be interesting to optimize this later.
64	Might be more efficient to resize to 0 and then use `->resize(this->totedge, true)`. That's because right now the new bits are all first initialized to zero and then again to one.
81	Should probably tag dirty first to make sure that the cache is actually updated.
source/blender/nodes/geometry/nodes/node_geo_duplicate_elements.cc
748	Could probably tag all edges as loose here similar to how we tag none as loose in some places.

Hans Goudey (HooglyBoogly) marked 2 inline comments as done.Nov 16 2022, 10:51 PM

Hans Goudey (HooglyBoogly) added inline comments.

source/blender/blenkernel/intern/mesh_runtime.cc
60	Do you have any ideas? I'm sure there are some possibilities, I don't know much about dealing with bitmaps though. It should be easier after T102359 too.
64	Good idea, thanks.
81	I think I'd like to separate "un-sharing" the cache from setting its value explicitly. That's more aligned to how normals are handled, and it's possible that you'd end up calculating the value explicitly without change the mesh. In that case you'd still want to benefit from the sharing.
source/blender/nodes/geometry/nodes/node_geo_duplicate_elements.cc
748	It could be done, but only at the cost of creating the whole bitmap. Unless I'm actually observing an improvement, I'd like to avoid doing it that eagerly right now. OTOH, in the future I could see there being three states in the loose edge cache: count=0, bitmap size=0 count=N, bitmap size=mesh.totedge count=mesh.totedge, bitmap size=0 For now I'd like to avoid the complexity though.

Use shared cache
Update BitVector name
Optimize resizing
Mention tagging dirty in comment

Jacques Lucke (JacquesLucke) accepted this revision.Nov 17 2022, 12:31 PM

Jacques Lucke (JacquesLucke) added inline comments.

source/blender/blenkernel/intern/mesh_runtime.cc
60	Ideas yes, not sure yet if they will actually make it faster though, that makes it interesting. For example: Each thread gets its own bitmap and they are combined in the end. First store a byte per edge to allow multi-threaded writing, and then convert it into a bitmap afterwards. Use atomic compare-exchange to write to the bitmap from multiple threads.
81	Not sure I fully understand. Can you provide a concrete example? To me it seems like this is likely only called when you just changed the mesh, which also means that it is not shared yet. If you still want to keep tagging dirty separate, there should be a comment explaining why.

This revision is now accepted and ready to land.Nov 17 2022, 12:31 PM

Closed by commit rB1ea169d90e39: Mesh: Move loose edge flag to a separate cache (authored by Hans Goudey (HooglyBoogly)). · Explain WhyNov 18 2022, 11:05 PM

This revision was automatically updated to reflect the committed changes.

Hans Goudey (HooglyBoogly) added a commit: rB1ea169d90e39: Mesh: Move loose edge flag to a separate cache.

Mesh: Move loose edge flag to a separate cacheClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 57625

source/blender/blenkernel/BKE_mesh.h

source/blender/blenkernel/BKE_mesh_types.h

source/blender/blenkernel/intern/bvhutils.cc

source/blender/blenkernel/intern/cloth.cc

source/blender/blenkernel/intern/curve_to_mesh_convert.cc

source/blender/blenkernel/intern/geometry_component_mesh.cc

source/blender/blenkernel/intern/mesh_calc_edges.cc

source/blender/blenkernel/intern/mesh_convert.cc

source/blender/blenkernel/intern/mesh_legacy_convert.cc

source/blender/blenkernel/intern/mesh_remesh_voxel.cc

source/blender/blenkernel/intern/mesh_runtime.cc

source/blender/blenkernel/intern/mesh_validate.cc

source/blender/blenkernel/intern/subsurf_ccg.c

source/blender/blenloader/intern/versioning_280.c

source/blender/bmesh/intern/bmesh_construct.c

source/blender/draw/intern/draw_cache_extract_mesh_render_data.cc

source/blender/editors/include/ED_mesh.h

source/blender/editors/mesh/mesh_data.cc

source/blender/editors/object/object_modifier.cc

source/blender/geometry/intern/mesh_merge_by_distance.cc

source/blender/geometry/intern/mesh_primitive_cuboid.cc

source/blender/gpencil_modifiers/intern/lineart/lineart_cpu.cc

source/blender/io/collada/GeometryExporter.cpp

source/blender/io/collada/MeshImporter.cpp

source/blender/io/wavefront_obj/exporter/obj_export_file_writer.cc

source/blender/io/wavefront_obj/exporter/obj_export_mesh.hh

source/blender/io/wavefront_obj/exporter/obj_export_mesh.cc

source/blender/io/wavefront_obj/exporter/obj_exporter.cc

source/blender/io/wavefront_obj/importer/obj_import_mesh.cc

source/blender/makesdna/DNA_mesh_types.h

source/blender/makesdna/DNA_meshdata_types.h

source/blender/makesrna/intern/rna_mesh.c

source/blender/modifiers/intern/MOD_mask.cc

source/blender/modifiers/intern/MOD_screw.c

source/blender/nodes/geometry/nodes/node_geo_convex_hull.cc

source/blender/nodes/geometry/nodes/node_geo_delete_geometry.cc

source/blender/nodes/geometry/nodes/node_geo_duplicate_elements.cc

source/blender/nodes/geometry/nodes/node_geo_extrude_mesh.cc

source/blender/nodes/geometry/nodes/node_geo_mesh_primitive_circle.cc

source/blender/nodes/geometry/nodes/node_geo_mesh_primitive_cone.cc

source/blender/nodes/geometry/nodes/node_geo_mesh_primitive_grid.cc

source/blender/nodes/geometry/nodes/node_geo_mesh_primitive_line.cc

source/blender/nodes/geometry/nodes/node_geo_mesh_primitive_uv_sphere.cc

Mesh: Move loose edge flag to a separate cache
ClosedPublic
Actions

Revision Contents
Changeset List