Patch to fix T84651
Previously vertex Weights were not normalizing on using gradient tool for weight paint (Considering auto normalize is checked).
This patch will fix the normalizing issue regarding gradient tool for weight paint .
Details
Details
- Reviewers
Campbell Barton (campbellbarton) - Maniphest Tasks
- T84651: Weight Paint Gradient Tool doesn't consider Auto-Normalize
Diff Detail
Diff Detail
- Repository
- rB Blender
Event Timeline
Comment Actions
Didn't verify the correctness of the code, just readability issues.
| source/blender/blenkernel/intern/deform.c | ||
|---|---|---|
| 450 ↗ | (On Diff #33266) | Follow the naming style same as other variables in this module. tot is generally a prefix. |
| 451 ↗ | (On Diff #33266) | def_nr_lock can be const too |
| 454 ↗ | (On Diff #33266) | Might want to early return here ? |
| 460 ↗ | (On Diff #33266) | Early return here, and then remove the else (and the scope & indent it created). |
| 464 ↗ | (On Diff #33266) | Keep the scope of i and other variables to the minimum. |
| 466 ↗ | (On Diff #33266) | Is the first i in lock_iweight a typo? I'm not familiar with the code here, so ignore if I'm wrong here. |
| 482 ↗ | (On Diff #33266) | Comments should start with capital letter, and end with period. Same elsewhere. |
| 484 ↗ | (On Diff #33266) | |
| source/blender/editors/sculpt_paint/paint_vertex_weight_ops.c | ||
| 772 | Probably a mistake to add an empty line | |
| 858 | Remove "fix". Comment to concisely describe what the code below does is fine. | |
| 860–863 | Don't declare uninitialized variables. | |
| 867 | const bool is_normalize | |
| 872–880 | 4 empty lines seem overdone here | |
Comment Actions
Tested! Thanks for the patch! I'm not a C coder, but I have some feedback on what it's doing:
Although doing what it should, there is a difference between this and how brushes do their normalization. The way this works is actually not necessarily better nor worse, but different:
- When a vertex only has a weight for one group, you shouldn't be able to subtract anything from it. This was always slightly controversial, but that's how brushes currently work with Auto-Normalize.
- Even when a vertex has weights from multiple groups, when you subtract all of one group's weight from a vertex, it should result in a weight of 0.0 still being assigned to that vert. Instead, it seems to un-assign that vert completely. I actually prefer this, but this is inconsistent with what brushes do. (Adding this as a feature would be totally nice though, but that's a discussion for another patch c:)
Let me know if I'm not making sense.
Comment Actions
Checking BKE_defvert_normalize_lock_single, it looks like this could be used instead of adding a new normalize function.
(other vertex groups may be used for physics or painting particle systems).
To match painting functions, this could take the vgroup_subset argument too, otherwise there is no way for vertex groups to exclude themselves from normalizing.
| source/blender/blenkernel/intern/deform.c | ||
|---|---|---|
| 448 ↗ | (On Diff #33303) | Regarding naming BKE_devert_normalize_gradient_paint isn't very informative, as it doesn't let developers know whats different. Also, if this is used in other situations, the name is likely to become meaningless. |
| 448–449 ↗ | (On Diff #33303) | No reason to pass both dv and dw, as you can access both with dv. |
| source/blender/editors/sculpt_paint/paint_vertex_weight_ops.c | ||
| 860–862 | vc is only used to access scene use: Scene *scene = CTX_data_scene(C); instead. | |
This comment was removed by Pratik Borhade (PratikPB2123).
| source/blender/blenkernel/intern/deform.c | ||
|---|---|---|
| 466 ↗ | (On Diff #33266) | I keep code block related to calculation of vertex weight similar to other function (eg. see BKE_defvert_normalize_lock_single ,BKE_defvert_normalize_lock_map) . |
| 482 ↗ | (On Diff #33266) | see BKE_defvert_normalize_lock_single or BKE_defvert_normalize_lock_map |
This comment was removed by Pratik Borhade (PratikPB2123).
Comment Actions
hey @Campbell Barton (campbellbarton) , do you want me to remove new function and use `BKE_defvert_normalize_lock_single ??
Comment Actions
Yes, unless there is any important feature in your own function which works differently, from a quick look it seems its not needed.
Comment Actions
This is much simpler, and I don't see any down-sides to using the existing function.
Currently this is running on all deform groups (even unselected geometry users wont expect to edit).
Suggest to use WPGradient_userData.vert_visit, only normalizing weights that have been edited.
Note, we should only operating on the vertices modified on the final update, this might need some more investigation.
Note, ideally this would normalize while drawing the gradient, it's possible some modifiers/effects use the absolute value of some groups.
This is more involved, so it could be done as a separate patch - since it would likely involve a full copy of all vertex groups.
| source/blender/editors/sculpt_paint/paint_vertex_weight_ops.c | ||
|---|---|---|
| 866 | This should be initialized from BKE_object_defgroup_validmap_get. | |
| source/blender/editors/sculpt_paint/paint_vertex_weight_ops.c | ||
|---|---|---|
| 866–879 | This is unnecessary data.vert_visit is already created and freed above (line 840 without this patch applied). | |
| source/blender/editors/sculpt_paint/paint_vertex_weight_ops.c | ||
|---|---|---|
| 610 | This can be moved under the VGRAD_STORE_DW_EXIST check below. Since that situation won't ever cause the vertices to be painted onto. | |
| 866 | My mistake, that functions checks bones, this should be BKE_object_defgroup_lock_flags_get - to respect the groups lock icon. | |
Comment Actions
Noticed ,weight subtraction was not occurring well for smaller values ( < 0.001) . that is why value assignment for lock_iweight changed . Rest all suggested changes implemented .
Comment Actions
Can you provide an example of this?
Changes to normalize behavior should be kept separate from improvements to the gradient tool.
Comment Actions
Removed change in BKE_defvert_normalize_lock_single . As it was condition specific ( for smaller value of vertex weight ), new bug report can be consider for it .