Page MenuHome

Refactor: Move vertex group names to object data
ClosedPublic

Authored by Hans Goudey (HooglyBoogly) on Jun 24 2021, 12:42 AM.

Details

Summary

This commit moves the storage of bDeformGroup and the active index
to Mesh, Lattice, and bGPdata instead of Object.

As explained in T88951, the list of vertex group names is currently
stored separately per object, even though vertex group data is stored
on the geometry. This tends to complicate code and cause bugs,
especially as geometry is created procedurally and tied less closely
to objects.

The "Copy Vertex Groups to Linked" operator is removed, since they
are stored on the geometry anyway.

This patch leaves the object-level python API for vertex groups in place.
Creating a geometry-level RNA API can be a separate step, the changes
here are invasive enough as it is.

Diff Detail

Repository
rB Blender
Branch
refactor-vertex-group-names (branched from master)
Build Status
Buildable 15620
Build 15620: arc lint + arc unit

Event Timeline

Hans Goudey (HooglyBoogly) requested review of this revision.Jun 24 2021, 12:42 AM
Hans Goudey (HooglyBoogly) created this revision.
  • Add comments, minor cleanup, remove debug statements
  • Fix unused variable warning
  • Move active index to geometry
  • Add small API to retrieve vertex group indices and lists from an ID
  • Merge branch 'master' into refactor-vertex-group-names
  • More fixes and cleanup
  • Fix issue with modifier vertex group utility-- tests pass
  • Merge branch 'master' into refactor-vertex-group-names
Hans Goudey (HooglyBoogly) retitled this revision from Refactor: Move vertex group names to object data (WIP) to Refactor: Move vertex group names to object data.Jul 5 2021, 3:29 PM
Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)
  • Share implementation of vertex group retrieval
  • Rename functions "for_read" -> "" and "for_write" -> "mutable"

Looks all very straight forward, good job!
Got a few minor comments, but those should be easy to fix.

Ideally at least one other dev should have a quick look at this. Also maybe @Demeter Dzadik (Mets) can check if this breaks production files?

source/blender/blenkernel/intern/deform.c
537

Implement this in terms of BKE_id_defgroup_list_get.

584

This and the function above can share code by extracting a int *BKE_object_defgroup_active_index_p(...) function.

source/blender/blenkernel/intern/lattice_deform.c
367

Can just cast to ID * directly.

source/blender/blenkernel/intern/mesh.c
935

Under what circumstances is this copied already? Would it make sense to always clear and copy the list?

source/blender/editors/gpencil/gpencil_fill.c
1526

Could you use tgpf->gpd->... directly?

source/blender/editors/object/object_vgroup.c
2846

Don't call BKE_object_defgroup_active_index_get twice.

source/blender/makesdna/DNA_object_types.h
61

Reminder that there is this todo comment. I don't think it's super urgent to move this now. If you wanted to move it, you'd probably have to move it to a new file.

283

Leave a comment saying this has been moved to the object data.

This revision is now accepted and ready to land.Jul 5 2021, 6:21 PM
Hans Goudey (HooglyBoogly) marked 8 inline comments as done.Jul 5 2021, 8:58 PM
Hans Goudey (HooglyBoogly) added inline comments.
source/blender/blenkernel/intern/mesh.c
935

The newly added BKE_id_eval_properties_copy function does this. The correct thing to do is free the list and then replace it though, I think. I've updated this function to do that.

source/blender/makesdna/DNA_object_types.h
61

Yeah, probably doesn't matter enough to move it.

Hans Goudey (HooglyBoogly) marked 2 inline comments as done.
  • Fixes from review comments

check forward compatibility

I couldn't find this important change in release notes, so I added it to the Animation & Rigging section.