Details

Reviewers

Campbell Barton (campbellbarton)
Jacques Lucke (JacquesLucke)

Maniphest Tasks

T95966: Struct of Arrays Refactor for Mesh Edges
T95967: Struct of Arrays Refactor for Mesh Polygons
T93602: Struct of Arrays Refactor for Mesh Vertices

Commits

rB2480b55f216c: Mesh: Move hide flags to generic attributes

Summary

Using the attribute name semantics proposed in T97452, this patch moves
the hide status of mesh elements from the ME_FLAG of vertices, edges,
and faces to generic boolean attribute layers. Storing this data as
generic attributes can significantly simplify and improve code,
as described in T95965.

The attributes are called .hide_vert, .hide_edge, and .hide_face.
The . prefix means they are "UI attributes", so they still contain
original data edited by users, but they aren't meant to be accessed
procedurally by the user in arbitrary situations. They will also be
hidden in the spreadsheet and the attribute list by default (I left
that change out for now to help the review process).

The attributes are still written to and read from the mesh in the old way,
so neither forward nor backward compatibility are affected.

Branch: refactor-mesh-hide-generic

Diff Detail

Repository: rB Blender

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Hans Goudey (HooglyBoogly) mentioned this in rB450a19009569: Cleanup: Remove unused subsurf ccg functions.May 15 2022, 12:12 PM

Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)May 15 2022, 12:28 PM

Hans Goudey (HooglyBoogly) mentioned this in rBe46a38942a87: Cleanup: Simplify loop syntax, decrease variable scope.May 15 2022, 3:43 PM

Hans Goudey (HooglyBoogly) mentioned this in rB1c70402c62ad: Cleanup: Move three mesh editors files to C++.May 15 2022, 8:41 PM

Hans Goudey (HooglyBoogly) mentioned this in rB84a764156340: Cleanup: Simplify loop syntax, make function static.May 15 2022, 8:59 PM

Hans Goudey (HooglyBoogly) mentioned this in D15011: Cleanup: Simplify custom data file writing process.May 22 2022, 8:49 PM

Hans Goudey (HooglyBoogly) planned changes to this revision.May 22 2022, 9:02 PM

Hans Goudey (HooglyBoogly) added parent revisions: D15011: Cleanup: Simplify custom data file writing process, D14583: Blend Write: Add option for legacy mesh format.

Hans Goudey (HooglyBoogly) mentioned this in rB3152d68b7006: Cleanup: Simplify custom data file writing process.May 29 2022, 11:02 AM

Hans Goudey (HooglyBoogly) mentioned this in D15069: Attributes: Hide internal UI attributes and disallow procedural access.May 30 2022, 6:59 PM

Hans Goudey (HooglyBoogly) added a parent revision: D15069: Attributes: Hide internal UI attributes and disallow procedural access.

Hans Goudey (HooglyBoogly) mentioned this in rB4669178fc378: Attributes: Hide internal UI attributes and disallow procedural access.May 31 2022, 1:21 PM

Hans Goudey (HooglyBoogly) updated this revision to Diff 52086.May 31 2022, 6:23 PM

Many updates and fixes
Working edit mode conversion
Working "legacy mesh format" option

Hans Goudey (HooglyBoogly) mentioned this in rB7974d2bff6f3: CustomData: Add function to free a named layer.Jun 7 2022, 6:01 PM

Cleanup to naming
Changes to use the new attribute API
Fixes to paint hide flushing

Cleanup: Remove unrelated change

@Campbell Barton (campbellbarton), @Jacques Lucke (JacquesLucke), I'm not sure how much either of you want to review this.
Feel free to look into it as much or as little as you want. You're the two people I thought of though.

Paul Larson (GeorgiaPacific) added a subscriber: Paul Larson (GeorgiaPacific).Jul 11 2022, 10:19 PM

Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)Jul 13 2022, 3:46 AM

Hans Goudey (HooglyBoogly) added a child revision: D15488: Mesh: Remove redundant custom data pointers.Jul 19 2022, 3:51 AM

Hans Goudey (HooglyBoogly) mentioned this in D14583: Blend Write: Add option for legacy mesh format.Jul 20 2022, 12:30 AM

Merge master
Cleanup: Rename function

Cleanup: Clang format

Hans Goudey (HooglyBoogly) planned changes to this revision.Jul 20 2022, 7:04 PM

Always save with legacy mesh format

Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)Jul 20 2022, 7:52 PM

Looks like some tests are failing. Some due to changes in attribute layer count, but some seem to fail for different reasons.

source/blender/blenkernel/BKE_mesh_legacy_convert.h
21	Better don't call it "new", because it's only new now.
source/blender/blenkernel/intern/mesh_evaluate.cc
755	Isn't this changing behavior?
source/blender/blenkernel/intern/mesh_legacy_convert.cc
893	Looks like an easy target for multi threading and possibly other optimizations, since this is run every time a file is saved and when an undo step is created (the latter of which could potentially be skipped, not sure).
921	Call `finish`

Fix BMesh conversion
Remove bad assert
Don't create layers when unnecessary
Parallelize flag copying
Cleanup variables names, add "finish" calls
Remove "new" in comment
Merge master

Hans Goudey (HooglyBoogly) marked an inline comment as done.Jul 22 2022, 2:27 AM

Hans Goudey (HooglyBoogly) added inline comments.

source/blender/blenkernel/intern/mesh_evaluate.cc
755	Good catch, thanks. The hidden propagation here is definitely "aggressive". Not so say that's wrong, but it might be better if it propagated in the same way as other boolean attributes in the future. I won't change the behavior now though.
source/blender/blenkernel/intern/mesh_legacy_convert.cc
893	I parallelized the copying of flags to and from mesh elements and layers. I didn't parallelize the `std::any_of`, because I didn't want to re-implement that logic, and some searching told me that apparently compilers can parallelize it themselves (not sure if they actually do though).

Merge master

https://builder.blender.org/admin/#/builders/18/builds/578

Test builds are failing when applying the patch. Not sure why, maybe updating the diff will help

https://builder.blender.org/admin/#/builders/57/builds/1447

Merge master, fix sculpt undo retrieval of hide values

Skip legacy conversion when reading and writing undo steps

Merge master

Fix compile warning

Merge master

Cleanup: Remove unused variable

Generally seems fine, some minor notes only.

The convention to use "." + snake-case for built-in custom-data should be documented, suggest: CustomDataLayer.name although it could be elsewhere.
Prefer the name .hide_poly for the polygon layer, as there is face data which is separate (and the term poly in general when assigning variables etc).
Commit message should note the file size increase from this change (roughly).

source/blender/makesrna/intern/rna_mesh.c
2186	use `rna_MeshPoly_*` in keeping w/ existing names.
source/blender/modifiers/intern/MOD_decimate.c
206–207	Why is this change needed? Logically decimate shouldn't need to build a lookup table (AFAICS).

This revision now requires changes to proceed.Aug 10 2022, 5:47 AM

Campbell Barton (campbellbarton) added inline comments.Aug 10 2022, 5:56 AM

source/blender/bmesh/intern/bmesh_mesh_convert.cc
948	Is there much advantage in having these separate functions compared with setting these values in the existing loop that copies values from/to? This requires a separate loop over the geometry and lookup table, we could lazily create this data inline - instead.
957–958	`.edge_vert`, `.face_vert` typos?

Campbell Barton (campbellbarton) added inline comments.Aug 10 2022, 5:58 AM

source/blender/modifiers/intern/MOD_decimate.c
206–207	I see these tables could be created for maintaining hidden geometry, I don't think `BKE_mesh_from_bmesh_for_eval_nomain` needs to support hide at all, either it could be removed - of if there are some cases where it's useful, have an argument not to support hide layers - which can be enabled for this caller (probably other modifier evaluation too)

The convention to use "." + snake-case for built-in custom-data should be documented, suggest: CustomDataLayer.name although it could be elsewhere.

I'll do that separately, if you don't mind. The convention is already used (see allow_procedural_attribute_access).

Prefer the name .hide_poly for the polygon layer, as there is face data which is separate (and the term poly in general when assigning variables etc).

The current name is meant to be consistent with the name of the attribute domain. Personally I prefer the longer term consistency with the attribute API rather than the short term consistency with the legacy MFace, but I'm okay compromising here.
I hope we can replace the particle system by 4.0 so this stops being a problem.

Commit message should note the file size increase from this change (roughly).

File sizes shouldn't need to increase with this patch, since we still write and read with the legacy format (until 4.0).

source/blender/bmesh/intern/bmesh_mesh_convert.cc
948	I think it makes sense, for a few reasons. Avoids making the basic loop more complicated and longer and avoids adding more constant checks into the existing loops-- smaller hot loops are usually better. This can be parallelized easily (thought I had done that already) We don't want to add the layers unless necessary , which requires a separate loop to check if the hide flags are used anyway It's in-line with natural parallelizations to the rest of the copying process (i.e. fast loops to build lookup tables first, then everything else can be parallelized).
957–958	Oops, thanks
source/blender/makesrna/intern/rna_mesh.c
2186	Hmm, I see 12 uses of `rna_MeshPoly_` and 24 uses of `rna_MeshPolygon`. The RNA struct name is also `MeshPolygon` rather than `MeshPoly`.
source/blender/modifiers/intern/MOD_decimate.c
206–207	The table is used during `BKE_mesh_from_bmesh_for_eval_nomain` to fill the hide layers if necessary. See `convert_bmesh_hide_flags_to_mesh_attributes`.

Don't save hide attributes in files at all (since they're still written in the old format anyway)
Parallelize layer copying
Fix attribute names in assert
Disallow procedural access
Rename .hide_face to .hide_poly
Merge branch 'master' into refactor-mesh-hide-generic

The patch LGTM, still not so keen on modifiers having to create hide data even when it's not needed (object mode display for e.g.). But this could be handled as part of a separate patch (if at all - the additional memory use is fairly minor).

In D14685#425323, @Hans Goudey (HooglyBoogly) wrote:

The convention to use "." + snake-case for built-in custom-data should be documented, suggest: CustomDataLayer.name although it could be elsewhere.

I'll do that separately, if you don't mind. The convention is already used (see allow_procedural_attribute_access).

Seems fine.

Commit message should note the file size increase from this change (roughly).

File sizes shouldn't need to increase with this patch, since we still write and read with the legacy format (until 4.0).

In that case the run-time increase should be noted, it's good to note increased memory use (not just when it's optimized).
And that this will apply to the file size for 4.0.

*Edit* noticed since instances of hide_face that should be named hide_poly remaining in the patch.

source/blender/modifiers/intern/MOD_decimate.c
206–207	In this case the assert can be moved under `updateFaceCount` since it's expected decimate wont create this data.

This revision is now accepted and ready to land.Aug 10 2022, 7:19 AM

Campbell Barton (campbellbarton) added inline comments.Aug 10 2022, 7:21 AM

source/blender/blenkernel/BKE_mesh_mapping.h
88	Should be named `hide_poly`.
source/blender/blenkernel/intern/bvhutils.cc
1186	Should be named `hide_poly`.
source/blender/blenkernel/intern/mesh_evaluate.cc
765	Should be named `hide_poly`.

Closed by commit rB2480b55f216c: Mesh: Move hide flags to generic attributes (authored by Hans Goudey (HooglyBoogly)). · Explain WhyAug 11 2022, 7:00 PM

This revision was automatically updated to reflect the committed changes.

Hans Goudey (HooglyBoogly) added a commit: rB2480b55f216c: Mesh: Move hide flags to generic attributes.

Refactor: Move mesh hide flags to generic attributesClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 54568

source/blender/blenkernel/BKE_customdata.h

source/blender/blenkernel/BKE_mesh.h

source/blender/blenkernel/BKE_mesh_legacy_convert.h

source/blender/blenkernel/BKE_mesh_mapping.h

source/blender/blenkernel/BKE_paint.h

source/blender/blenkernel/BKE_pbvh.h

source/blender/blenkernel/intern/attribute_access.cc

source/blender/blenkernel/intern/bvhutils.cc

source/blender/blenkernel/intern/customdata.cc

source/blender/blenkernel/intern/mesh.cc

source/blender/blenkernel/intern/mesh_evaluate.cc

source/blender/blenkernel/intern/mesh_legacy_convert.cc

source/blender/blenkernel/intern/mesh_mapping.c

source/blender/blenkernel/intern/paint.c

source/blender/blenkernel/intern/pbvh.c

source/blender/blenkernel/intern/pbvh_intern.h

source/blender/blenkernel/intern/subdiv_converter_mesh.c

source/blender/blenkernel/intern/subsurf_ccg.c

source/blender/bmesh/intern/bmesh_construct.c

source/blender/bmesh/intern/bmesh_mesh_convert.cc

source/blender/draw/intern/draw_cache_extract_mesh_render_data.cc

source/blender/draw/intern/draw_cache_impl_subdivision.cc

source/blender/draw/intern/mesh_extractors/extract_mesh.hh

source/blender/draw/intern/mesh_extractors/extract_mesh_ibo_edituv.cc

source/blender/draw/intern/mesh_extractors/extract_mesh_ibo_fdots.cc

source/blender/draw/intern/mesh_extractors/extract_mesh_ibo_lines.cc

source/blender/draw/intern/mesh_extractors/extract_mesh_ibo_lines_adjacency.cc

source/blender/draw/intern/mesh_extractors/extract_mesh_ibo_lines_paint_mask.cc

source/blender/draw/intern/mesh_extractors/extract_mesh_ibo_points.cc

source/blender/draw/intern/mesh_extractors/extract_mesh_ibo_tris.cc

source/blender/draw/intern/mesh_extractors/extract_mesh_vbo_lnor.cc

source/blender/draw/intern/mesh_extractors/extract_mesh_vbo_pos_nor.cc

source/blender/editors/include/ED_mesh.h

source/blender/editors/mesh/editface.cc

source/blender/editors/mesh/editmesh_undo.c

source/blender/editors/mesh/meshtools.cc

source/blender/editors/object/object_vgroup.c

source/blender/editors/sculpt_paint/paint_hide.c

source/blender/editors/sculpt_paint/sculpt.c

source/blender/editors/sculpt_paint/sculpt_dyntopo.c

source/blender/editors/sculpt_paint/sculpt_undo.c

source/blender/editors/space_view3d/view3d_iterators.c

source/blender/editors/space_view3d/view3d_select.c

source/blender/gpu/GPU_buffers.h

source/blender/gpu/intern/gpu_buffers.c

source/blender/gpu/intern/gpu_shader_builder_stubs.cc

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

source/blender/makesdna/DNA_meshdata_types.h

source/blender/makesrna/intern/rna_mesh.c

source/blender/modifiers/intern/MOD_decimate.c

Refactor: Move mesh hide flags to generic attributes
ClosedPublic
Actions

Revision Contents
Changeset List