Unifying sculpt, weight paint & vertex paint symmetry options under mesh properties #110156

dgkf · 2023-07-16T03:42:47+02:00

dgkf commented

2023-07-16 03:42:47 +02:00

First-time contributor

Closes #108107, tagging @JosephEagar and @JulienKaspar for review as suggested

Summary

Migrates sculpt (and weight paint, vertex paint) mode symmetry settings (including symmetry, tiling and axis locking) to mesh properties, which allows them to be configured per-mesh instead of defined for all objects. A few properties already worked this way, this work expands on previous work to migrate the reset of the sculpt symmetry panel settings.

Changes

feat: lock_{x,y,z}, tile_{x,y,z}, use_mirror_{x,y,z}, symmetry, symmetry_feather, symmetrize_direction, radial_symmetry and tile_offset were moved to be mesh properties (previously sculpt mode properties)
feat: Toolbars and menus were migrated to update these mesh data properties
feat: Respective operators in sculpt, weight painting and vertex painting were migrated to use these mesh data properties

chore: Replaced this old enum magic with a pattern I spotted in the sculpt code internals. At least for me, it made me a lot more comfortable making edits. Hopefully this helps to make the code's purpose more obvious and more consistent throughout.

- for (int i = 0; i <= symm; i++) {
-   if (i == 0 || (symm & i && (symm != 5 || i != 3) && (symm != 6 || !ELEM(i, 3, 5)))) {
+ for (ePaintSymmetryFlags symmpass = PAINT_SYMM_NONE; symmpass <= symm; symmpass++) {
+   if (SCULPT_is_symmetry_iteration_valid(symmpass, symm)) {

Needs Feedback

This is my first attempt at contributing to Blender, and there was a lot I had to learn to put this together. I'm sure the initial issue is clear to veterans, but I felt like I had to still make a lot of educated guesses about implementation details. I'll try to document areas of uncertainty below:

I have a hard time telling if the symmetry feathering is working, so I'd appreciate a close look at my changes to that feature.
How to set default values? The tile offset in sculpt mode defaults to 0, when the original default was 1, and I couldn't figure out how to change this behavior.
How to manage deprecation/version updates in versioning_400.cc? this felt beyond my capabilities right now. It wasn't obvious to me how I should go about propagating that data to the meshes.
To keep the scope manageable, I didn't go through all the trouble of stripping out the old symmetry enums (PAINT_SYMM_{X,Y,Z}, SCULPT_LOCK_{X,Y,Z}) and their use throughout many sculpting operators. Instead, eMeshSymmetryType is a 1-to-1 replacement for ePaintSymmetryFlags and I simply cast to the old paint enum to leverage these existing functions without a complete refactor.
Two fields, tile (axis tiling flags) and symmetry (symmetry axis flags) were listed in a follow-up comment in the original issue (#108107 (comment)), but not in the initial issue body (#108107 (comment)). I didn't realize they weren't part of the original scope until after I had already migrated them. Hopefully it's still valuable work, but if not I can revert those changes.
I'm not very familiar with the python API, so extra attention to those changes would be appreciated - especially since these changes might require some lifecycle management to deprecate old fields.
If testing is expected, I could use some pointers.

Closes #108107, tagging @JosephEagar and @JulienKaspar for review as suggested ### Summary Migrates sculpt (and weight paint, vertex paint) mode symmetry settings (including symmetry, tiling and axis locking) to mesh properties, which allows them to be configured per-mesh instead of defined for all objects. A few properties already worked this way, this work expands on previous work to migrate the reset of the sculpt symmetry panel settings. ### Changes * **feat:** `lock_{x,y,z}`, `tile_{x,y,z}`, `use_mirror_{x,y,z}`, `symmetry`, `symmetry_feather`, `symmetrize_direction`, `radial_symmetry` and `tile_offset` were moved to be mesh properties (previously sculpt mode properties) * **feat:** Toolbars and menus were migrated to update these mesh data properties * **feat:** Respective operators in sculpt, weight painting and vertex painting were migrated to use these mesh data properties * **chore:** Replaced this old enum magic with a pattern I spotted in the sculpt code internals. At least for me, it made me a lot more comfortable making edits. Hopefully this helps to make the code's purpose more obvious and more consistent throughout. ```diff - for (int i = 0; i <= symm; i++) { - if (i == 0 || (symm & i && (symm != 5 || i != 3) && (symm != 6 || !ELEM(i, 3, 5)))) { + for (ePaintSymmetryFlags symmpass = PAINT_SYMM_NONE; symmpass <= symm; symmpass++) { + if (SCULPT_is_symmetry_iteration_valid(symmpass, symm)) { ``` ### Needs Feedback > This is my first attempt at contributing to Blender, and there was a lot I had to learn to put this together. I'm sure the initial issue is clear to veterans, but I felt like I had to still make a lot of educated guesses about implementation details. I'll try to document areas of uncertainty below: - [ ] I have a hard time telling if the symmetry feathering is working, so I'd appreciate a close look at my changes to that feature. - [ ] How to set default values? The tile offset in sculpt mode defaults to `0`, when the original default was `1`, and I couldn't figure out how to change this behavior. - [ ] How to manage deprecation/version updates in `versioning_400.cc`? this felt beyond my capabilities right now. It wasn't obvious to me how I should go about propagating that data to the meshes. - [ ] To keep the scope manageable, I didn't go through all the trouble of stripping out the old symmetry enums (`PAINT_SYMM_{X,Y,Z}`, `SCULPT_LOCK_{X,Y,Z}`) and their use throughout many sculpting operators. Instead, `eMeshSymmetryType` is a 1-to-1 replacement for `ePaintSymmetryFlags` and I simply cast to the old paint enum to leverage these existing functions without a complete refactor. - [ ] Two fields, `tile` (axis tiling flags) and `symmetry` (symmetry axis flags) were listed in a follow-up comment in the original issue (https://projects.blender.org/blender/blender/issues/108107#issuecomment-953674), but not in the initial issue body (https://projects.blender.org/blender/blender/issues/108107#issue-97702). I didn't realize they weren't part of the original scope until after I had already migrated them. Hopefully it's still valuable work, but if not I can revert those changes. - [ ] I'm not very familiar with the python API, so extra attention to those changes would be appreciated - especially since these changes might require some lifecycle management to deprecate old fields. - [ ] If testing is expected, I could use some pointers.

dgkf added 3 commits 2023-07-16 03:42:59 +02:00

6e1457e857 unifying deform lock settings in sculpt symmetry menu under mesh data

f2cb21de6e moving tile, tile_offset, symmetry, symmetry feathering, radial_symmetry to mesh data

d6047d6fff port toolbar mapping to use mesh attribute

dgkf changed title from ~~Unifying sculpt, weight paint & vertex paint symmetry options under mesh properties~~ to WIP: Unifying sculpt, weight paint & vertex paint symmetry options under mesh properties

2023-07-16 03:43:39 +02:00

dgkf added 1 commit 2023-07-16 03:45:41 +02:00

89a3b1086b format; remove commented code and TODO notes

dgkf added 1 commit 2023-07-16 05:20:22 +02:00

buildbot/vexp-code-patch-lint Build done. Details

buildbot/vexp-code-patch-linux-x86_64 Build done. Details

buildbot/vexp-code-patch-darwin-arm64 Build done. Details

buildbot/vexp-code-patch-darwin-x86_64 Build done. Details

buildbot/vexp-code-patch-windows-amd64 Build done. Details

buildbot/vexp-code-patch-coordinator Build done. Details

05a691777d merging main; removing unused variables

dgkf changed title from ~~WIP: Unifying sculpt, weight paint & vertex paint symmetry options under mesh properties~~ to Unifying sculpt, weight paint & vertex paint symmetry options under mesh properties

2023-07-16 05:25:14 +02:00

Iliya Katushenock added this to the Sculpt, Paint & Texture project 2023-07-16 10:30:43 +02:00

dgkf commented

2023-10-03 19:20:42 +02:00

First-time contributor

Pinging @JosephEagar and @JulienKaspar

I didn't want to pester you during Wing It! production (which was amazing 🥰), but this PR is still looking for feedback. I know 4.0 is getting closer to release and just wanted to be sure I wasn't a bottleneck if you wanted this code to be part of it. If you have other priorities during this time, I understand that too.

I'm here to wrap up this work when you need me to.

Pinging @JosephEagar and @JulienKaspar I didn't want to pester you during Wing It! production (which was _amazing_ 🥰), but this PR is still looking for feedback. I know 4.0 is getting closer to release and just wanted to be sure I wasn't a bottleneck if you wanted this code to be part of it. If you have other priorities during this time, I understand that too. I'm here to wrap up this work when you need me to.

Julien Kaspar added the

Module

Sculpt, Paint & Texture

label 2023-10-06 12:14:15 +02:00

Julien Kaspar requested review from Sergey Sharybin 2023-10-06 12:14:40 +02:00

Julien Kaspar requested review from Julien Kaspar 2023-10-06 12:14:40 +02:00

Julien Kaspar requested review from Hans Goudey 2023-10-06 12:14:40 +02:00

Julien Kaspar commented

2023-10-06 12:15:53 +02:00

Thanks for working on this @dgkf
I added some reviewers who can help out with the technical implementation.

Thanks for working on this @dgkf I added some reviewers who can help out with the technical implementation.

🚀 1

Sergey Sharybin commented

2023-10-19 11:19:23 +02:00

@JulienKaspar Can you help with the artist-side testing? I.e., the symmetry feathering.

How to set default values? The tile offset in sculpt mode defaults to 0, when the original default was 1, and I couldn't figure out how to change this behavior.

For the new meshes it is to be done in source/blender/makesdna/DNA_mesh_defaults.h.

For the existing meshes you'd need to write versioning code:

Increment BLENDER_FILE_SUBVERSION in BKE_blender_version.h
Add corresponding if (!MAIN_VERSION_FILE_ATLEAST(...)) block
Move code from Versioning code until next subversion bump goes here. to that block
Add a loop over meshes and initialize the new fields to their desired values.

How to manage deprecation/version updates in versioning_400.cc? this felt beyond my capabilities right now. It wasn't obvious to me how I should go about propagating that data to the meshes.

Add DNA_DEPRECATED qualifier to the DNA fields which are deprecated.

To keep the scope manageable, I didn't go through all the trouble of stripping out the old symmetry enums (PAINT_SYMM_{X,Y,Z}, SCULPT_LOCK_{X,Y,Z}) and their use throughout many sculpting operators.

We should not have two enums which we need to keep in sync. That is just a recipe for a non-trivial bug to hunt down. If we deprecate something, only do-versioning code is allowed to access it.

I'm not very familiar with the python API, so extra attention to those changes would be appreciated - especially since these changes might require some lifecycle management to deprecate old fields.

We do not have mechanism to deprecate Python API. So from the code side the approach is to remove fields which are no longer used.

@JulienKaspar Can you help with the artist-side testing? I.e., the symmetry feathering. > How to set default values? The tile offset in sculpt mode defaults to 0, when the original default was 1, and I couldn't figure out how to change this behavior. For the new meshes it is to be done in `source/blender/makesdna/DNA_mesh_defaults.h`. For the existing meshes you'd need to write versioning code: - Increment `BLENDER_FILE_SUBVERSION` in `BKE_blender_version.h` - Add corresponding `if (!MAIN_VERSION_FILE_ATLEAST(...))` block - Move code from `Versioning code until next subversion bump goes here.` to that block - Add a loop over meshes and initialize the new fields to their desired values. > How to manage deprecation/version updates in versioning_400.cc? this felt beyond my capabilities right now. It wasn't obvious to me how I should go about propagating that data to the meshes. Add `DNA_DEPRECATED` qualifier to the DNA fields which are deprecated. > To keep the scope manageable, I didn't go through all the trouble of stripping out the old symmetry enums (PAINT_SYMM_{X,Y,Z}, SCULPT_LOCK_{X,Y,Z}) and their use throughout many sculpting operators. We should not have two enums which we need to keep in sync. That is just a recipe for a non-trivial bug to hunt down. If we deprecate something, only do-versioning code is allowed to access it. - I'm not very familiar with the python API, so extra attention to those changes would be appreciated - especially since these changes might require some lifecycle management to deprecate old fields. We do not have mechanism to deprecate Python API. So from the code side the approach is to remove fields which are no longer used.

Sergey Sharybin reviewed 2023-10-19 11:21:16 +02:00

source/blender/makesrna/intern/rna_object.cc Outdated

						
				@ -3784,1 +3840,4 @@

				  /* Mesh Lock Settings */

				  prop = RNA_def_property(srna, "use_mesh_lock_x", PROP_BOOLEAN, PROP_NONE);

Sergey Sharybin commented

2023-10-19 11:21:08 +02:00

Surely this follows the same pattern of use_mesh_mirror_{x,y,z} but I don't think we should be doing so. We should not have such "shortcuts". If the source of proeprty is a mesh, it needs to be accessed via mesh: ob.data. use_lock_x, not ob. use_mesh_lock_x.

Surely this follows the same pattern of `use_mesh_mirror_{x,y,z}` but I don't think we should be doing so. We should not have such "shortcuts". If the source of proeprty is a mesh, it needs to be accessed via mesh: `ob.data. use_lock_x`, not `ob. use_mesh_lock_x`.

👍 1

dgkf commented

2023-11-02 18:11:53 +01:00

First-time contributor

Sounds good! I'm happy to make this change - it also jumped out at me as a bit odd, but as a first time contributor it's not always clear how much license I should take with these types of refactoring decisions.

dgkf commented

2023-11-02 17:19:38 +01:00

First-time contributor

For the new meshes it is to be done in source/blender/makesdna/DNA_mesh_defaults.h.

For the existing meshes you'd need to write versioning code:

[...]

Add a loop over meshes and initialize the new fields to their desired values.

@Sergey - What default do you think should be used when a object has different settings in different modes? For example, symmetry lock is set in sculpt, but not in vertex paint. In such a case, what should be the migrated setting for the mesh?

I think it makes sense to enable each flag in a situation where any mode has the flag enabled. I imagine it's common for an object to be edited in only one mode (sculpted, but never vertex painted), in which case it might have sculpt x symmetry, but vertex painting was never touched. Does that sound like a reasonable way of porting old settings?

And just to give an update, I'm still definitely planning to come back to this but have been a bit preoccupied for a bit. I need to find a free weekend(s) to incorporate your feedback.

> For the new meshes it is to be done in `source/blender/makesdna/DNA_mesh_defaults.h`. > > For the existing meshes you'd need to write versioning code: > - [...] > - Add a loop over meshes and initialize the new fields to their desired values. @Sergey - What default do you think should be used when a object has different settings in different modes? For example, symmetry lock is set in sculpt, but not in vertex paint. In such a case, what should be the migrated setting for the mesh? I think it makes sense to enable each flag in a situation where _any_ mode has the flag enabled. I imagine it's common for an object to be edited in only one mode (sculpted, but never vertex painted), in which case it might have sculpt x symmetry, but vertex painting was never touched. Does that sound like a reasonable way of porting old settings? --- And just to give an update, I'm still definitely planning to come back to this but have been a bit preoccupied for a bit. I need to find a free weekend(s) to incorporate your feedback.

Sergey Sharybin commented

2024-02-27 16:28:19 +01:00

@dgkf Hey! We are finally getting to the backlog of the sculpt module. Do you mind merging the main branch into this PR, to help testing etc?

dgkf commented

2024-02-27 16:37:04 +01:00

First-time contributor

@Sergey - Sure will do. Sorry that work on this has stalled a bit. Frankly I'm a bit intimidated by some of the suggestions (handling the version migrations to the new data format and migrating all the instances of the previous variables).

I'll get the merge conflicts sorted out, but I think to keep this moving forward I might need some help with some of the aspects that require more familiarity.

@Sergey - Sure will do. Sorry that work on this has stalled a bit. Frankly I'm a bit intimidated by some of the suggestions (handling the version migrations to the new data format and migrating all the instances of the previous variables). I'll get the merge conflicts sorted out, but I think to keep this moving forward I might need some help with some of the aspects that require more familiarity.

Sergey Sharybin commented

2024-02-27 16:56:04 +01:00

@dgkf Surely we can help with the versioning and data format. Don't let these topics scare you! :)

P.S. If you're not there yet, you're welcome in the #sculpt-paint-texture-module room of blender.chat. Some discussions/questions/suggestions can be done more easily in the chat form.

@dgkf Surely we can help with the versioning and data format. Don't let these topics scare you! :) P.S. If you're not there yet, you're welcome in the `#sculpt-paint-texture-module` room of `blender.chat`. Some discussions/questions/suggestions can be done more easily in the chat form.

❤️ 1

Daniel Bystedt commented

2024-02-29 20:03:51 +01:00

@dgkf @Sergey please ping me when there is a a build available that I can test.

Sergey Sharybin commented

2024-03-04 14:54:41 +01:00

@blender-bot package

Blender Bot commented

2024-03-04 14:54:44 +01:00

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR110156) when ready.

Daniel Bystedt commented

2024-03-04 22:46:10 +01:00

@Sergey seems like the build failed. I'll hold off until I hear something new

dgkf commented

2024-03-04 22:47:48 +01:00

First-time contributor

@DanielBystedt I started with a rebase last night and it looks like it should be pretty manageable. I can tag you when there's a rebased version available.

❤️ 1 🎉 1

dgkf added 1 commit 2024-03-05 04:46:00 +01:00

buildbot/vexp-code-patch-lint Build done. Details

buildbot/vexp-code-patch-darwin-x86_64 Build done. Details

buildbot/vexp-code-patch-darwin-arm64 Build done. Details

buildbot/vexp-code-patch-linux-x86_64 Build done. Details

buildbot/vexp-code-patch-windows-amd64 Build done. Details

buildbot/vexp-code-patch-coordinator Build done. Details

611c6c7d78 merging upstream main

dgkf commented

2024-03-05 04:53:18 +01:00

First-time contributor

@DanielBystedt main has been merged in. Conflicts should all be sorted out now. I've built the latest version locally and things look like they've mostly carried over, but the code base looks like its changed pretty considerably since I started this work so I'm watching for hiccups.

One thing that I've noticed already is that I can induce a crash by linked-duplicating an object and then unlinking it.

Otherwise, I'm able to configure all the symmetry options on a per-mesh basis as was the focus of this PR.

Based on the attention this PR has gotten in the last couple weeks, it seems like this might be gating some further work, so if it's holding anyone up please feel free to carry this forward without me. I don't want to slow you all down and right now my bandwidth is limited to just a bit of time on weekends.

@DanielBystedt `main` has been merged in. Conflicts should all be sorted out now. I've built the latest version locally and things look like they've mostly carried over, but the code base looks like its changed pretty considerably since I started this work so I'm watching for hiccups. One thing that I've noticed already is that I can induce a crash by linked-duplicating an object and then unlinking it. Otherwise, I'm able to configure all the symmetry options on a per-mesh basis as was the focus of this PR. Based on the attention this PR has gotten in the last couple weeks, it seems like this might be gating some further work, so if it's holding anyone up please feel free to carry this forward without me. I don't want to slow you all down and right now my bandwidth is limited to just a bit of time on weekends.

Sergey Sharybin commented

2024-03-05 10:37:00 +01:00

@blender-bot package

Blender Bot commented

2024-03-05 10:37:04 +01:00

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR110156) when ready.

Daniel Bystedt commented

2024-03-09 17:16:47 +01:00

This PR works perfectly fine in my opinion. I can't replicate a crash by duplicete link an object and then unlink it as described above.

I tried the PR with

sculpt
weight paint
vertex paint
texture paint

All works. Approved from my end

This PR works perfectly fine in my opinion. I can't replicate a crash by duplicete link an object and then unlink it as described above. I tried the PR with - sculpt - weight paint - vertex paint - texture paint All works. Approved from my end

🎉 1

Daniel Bystedt approved these changes 2024-03-09 17:17:02 +01:00

dgkf commented

2024-03-11 00:14:24 +01:00

First-time contributor

I'd be thrilled if this was accepted, but I'd also suggest reading through the feedback @Sergey gave as many of those remain unresolved. If you do merge as-is, you might consider these as next steps:

There is currently no legacy file format mapping for the old mode-based setting data structures into mesh-based data.
To merge with master with minimal changes, there are a few _legacy variables that are used as this work started before they were deprecated.
There are still some old enum values used from when these were mode-based.

I can tackle these, but it will probably take me a couple weekends. If you'd like things to move faster, I'd be fine with someone swooping in and putting a final pass on everything.

I'd be thrilled if this was accepted, but I'd also suggest reading through the feedback @Sergey gave as many of those remain unresolved. If you do merge as-is, you might consider these as next steps: - There is currently no legacy file format mapping for the old mode-based setting data structures into mesh-based data. - To merge with master with minimal changes, there are a few `_legacy` variables that are used as this work started before they were deprecated. - There are still some old enum values used from when these were mode-based. I can tackle these, but it will probably take me a couple weekends. If you'd like things to move faster, I'd be fine with someone swooping in and putting a final pass on everything.

Sergey Sharybin commented

2024-03-13 11:22:16 +01:00

@DanielBystedt Thanks for testing!

@dgkf Daniel is more on an art-side of review, for the code either me or Hans still need to have a look. Knowing that on the behavior level Daniel is happy makes it more exciting to move forward with the review though!

I am not sure if I'll immediately have time to help on the code side, unfortunately. I am also not sure if we are in a huge hurry, so no need to feel pressured!

Let me know if you'd like to have some code side review already, or whether you have some changes planned already and we'd better wait for those?

@DanielBystedt Thanks for testing! @dgkf Daniel is more on an art-side of review, for the code either me or Hans still need to have a look. Knowing that on the behavior level Daniel is happy makes it more exciting to move forward with the review though! I am not sure if I'll immediately have time to help on the code side, unfortunately. I am also not sure if we are in a huge hurry, so no need to feel pressured! Let me know if you'd like to have some code side review already, or whether you have some changes planned already and we'd better wait for those?

buildbot/vexp-code-patch-lint Build done.

Details

buildbot/vexp-code-patch-darwin-x86_64 Build done.

Details

buildbot/vexp-code-patch-darwin-arm64 Build done.

Details

buildbot/vexp-code-patch-linux-x86_64 Build done.

Details

buildbot/vexp-code-patch-windows-amd64 Build done.

Details

buildbot/vexp-code-patch-coordinator Build done.

Details

This pull request has changes conflicting with the target branch.

scripts/startup/bl_ui/space_view3d_toolbar.py
source/blender/editors/sculpt_paint/sculpt_ops.cc

View command line instructions.

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u 108107-unified-mesh-props:dgkf-108107-unified-mesh-props

git checkout dgkf-108107-unified-mesh-props

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Unifying sculpt, weight paint & vertex paint symmetry options under mesh properties #110156

Summary

Changes

Needs Feedback

Checkout