Page MenuHome

Unify all XYZ symmetry options using Mesh Symmetry
ClosedPublic

Authored by Pablo Dobarro (pablodp606) on Aug 16 2020, 6:21 PM.

Details

Summary

This adds XYZ symmetry as a property of meshes and updates all modes to
use the mesh symmetry by default to have a consistent tool behavior
between all modes and when switching objects.

Diff Detail

Repository
rB Blender

Event Timeline

Pablo Dobarro (pablodp606) requested review of this revision.Aug 16 2020, 6:21 PM
Pablo Dobarro (pablodp606) created this revision.

This updates all paint modes to use the new symmetry flags. @Germano Cavalcante (mano-wii) Is it ok to replace all instances of ME_EDIT_MIRROR_X with the new flags?

Good to see this property being unified.
If the default value of the mesh edit remains the same (without symmetry), I see no problem.

Pablo Dobarro (pablodp606) edited the summary of this revision. (Show Details)
  • Use ME symmetry flags in Edit Mesh code
  • Fix UI in object mode

Ok, now XYZ symmetry options are shared between vetex paint, texture paint, weight paint, sculpt and edit mode. If everything works correctly, do we want to keep the old flags and RNA properties for something or should I remove and deprecate them?

For compatibility the property could just keep the name use_mirror_x and add use_mirror_y and use_mirror_z? If we want to change the UI as well to symmetry that's possible for consistency, but the Python API doesn't need to break for this I think.

This will also need version patching to keep the existing flag from edit mode, or it could keep using editflag.

  • Rename use_symmetry_x to use_mirror_x
  • Add versioning

Overall it looks good to me.

release/scripts/startup/bl_ui/space_view3d_toolbar.py
1100–1101

This ipaint seems to be no longer used here

source/blender/makesdna/DNA_mesh_types.h
270

I think it would be nice to remove (plus change the value of others and versioning) or rename these ME_EDIT_MIRROR_ flags to ME_FLAG_UNUSED_ since they are no longer used.

This revision is now accepted and ready to land.Aug 17 2020, 10:16 PM
Pablo Dobarro (pablodp606) marked 2 inline comments as done.
  • Review Update
Brecht Van Lommel (brecht) requested changes to this revision.Aug 18 2020, 5:35 PM

Looking at the patch, what is the purpose of moving the flags to a new symmetry member, rather than just leaving it in editflag? It seems simpler to just leave things as they were and make sculpting use those flags.

release/scripts/startup/bl_ui/space_view3d_toolbar.py
1004

The same mirror X option is now in two places. Previously it was already confusing, but the tooltip for the old option explained that one was for mirroring the brushes.

The best solution would be to support Y and Z mirroring everywhere. I haven't checked how complicated that is. If it's too complicated, is there something better we can do here?

This revision now requires changes to proceed.Aug 18 2020, 5:35 PM

Looking at the patch, what is the purpose of moving the flags to a new symmetry member, rather than just leaving it in editflag?

I thought that it may be less confusing to have a separate variable and flags for symmetry options that are intended to be supported in all modes. Right now editflag contains flags that are mode specific, like ME_EDIT_PAINT_VERT_SEL.

release/scripts/startup/bl_ui/space_view3d_toolbar.py
1004

I'm not familiar with the weight paint tools and with the transform tools, so I'm not sure why it is not supported. I've only seen the Y and Z symmetry flags being used once in the transform code.
Maybe we can leave the current editmesh flag for X symmetry where Y and Z is not supported and use the new flags for the rest of cases.

I personally don't see much issue in having the flags in editflag, but either way is fine.

This revision is now accepted and ready to land.Aug 19 2020, 7:43 PM
Campbell Barton (campbellbarton) requested changes to this revision.Aug 20 2020, 7:08 AM

Initial code review.

release/scripts/startup/bl_ui/space_view3d_toolbar.py
1110–1114

The mesh = context.object.data should be assigned.

source/blender/blenloader/intern/versioning_290.c
605–606

These flags could be cleared at the same time, so we can re-use them later. (their declaration should then have /* cleared */ after it, so we know reusing is safe).

source/blender/makesdna/DNA_mesh_types.h
272–273

These should be named ME_EDITFLAG_UNUSED_*, as we already have ME_FLAG_UNUSED_* for Mesh.flag.

This revision now requires changes to proceed.Aug 20 2020, 7:08 AM

This causes changed behavior in weight paint mode where both sculpt symmetry and weight mirror are enabled at once, this makes it impossible to paint onto symmetrically named groups.

Attached video, behavior from master on the left, this patch is on the right.


We could have an option for weight paint to make symmetry apply to Symmetry: [Brush | Vertex Group] since it seems highly unlikely we want to enable both at once.


Also noticed weight-paint will use Y and Z symmetry, if they have been set in other mode.

Pablo Dobarro (pablodp606) marked 3 inline comments as done.
  • Review Update

@Campbell Barton (campbellbarton) In that case, wouldn't it make more sense to add a separate flag for using vertex group symmetry and keep these flags only for brush symmetry to be consistent with the rest of the modes? Should that change be part of this patch?

Having a separate flag seems fine, I think that should be part of this patch.

  • Use a separate property for Vertex Group L/R mirroring

@Campbell Barton (campbellbarton) Ok, now weight paint should work as expected. There are separate options to control the brush symmetry (using the unified symmetry flags like in any other mode) and the L/R vertex groups symmetry

Seems fine, only minor issues noted.

source/blender/makesdna/DNA_mesh_types.h
273–274

We could leave these for future support of Y/Z vertex-group mirror (leave them commented out).

source/blender/makesrna/intern/rna_mesh.c
3290–3294

Naming is a bit odd here, why not call: use_mirror_vertex_group_x ?

This revision is now accepted and ready to land.Sep 18 2020, 3:27 AM
This revision was automatically updated to reflect the committed changes.
Pablo Dobarro (pablodp606) marked an inline comment as done.