Page MenuHome

Sculpt: Pose Brush with Inverse Kinematics
ClosedPublic

Authored by Pablo Dobarro (pablodp606) on Dec 11 2019, 5:26 AM.
Tokens
"Like" token, awarded by Constantina32."Love" token, awarded by monio."Like" token, awarded by Maged_afra."Love" token, awarded by sebbyling."100" token, awarded by womanonfire."Love" token, awarded by Alumx."Love" token, awarded by threedslider."100" token, awarded by Frozen_Death_Knight."Love" token, awarded by ruthwikrao."Love" token, awarded by NestorJimenezArt."Burninate" token, awarded by MetinSeven."Love" token, awarded by Noss."Love" token, awarded by jmztn."Love" token, awarded by Brandon777."Yellow Medal" token, awarded by franMarz."Like" token, awarded by Loner."Burninate" token, awarded by Alrob."Love" token, awarded by ILJI.

Diff Detail

Repository
rB Blender

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
source/blender/editors/sculpt_paint/sculpt.c
3582

Split this function in two separate functions. We only call displacement or rotation at a single time.
this will remove many false-if statements and makes the code more readable.

3609

add const where needed. (this needs to be done for the whole patch). adding more keywords makes code more readable.

3609

remove the i_ and just use a different name for the local copy.

3609

use lowercase names for function names. Except when indicating a module public function

3678–3679

is val modified at all or is this to fix potential threading issues?

3681

Possible index out of bound

3682

Don't use char

3748

copy_qt_qt?

3748

unneeded quat_to_mat4 writes a complete mat4

3767–3790

unneeded copies_m4_m4.

3827–3828

use MAX2

4053–4054

0.0f

4064

unneeded float conversion. better to let the compiler decide. Not sure if FIDIV instruction will be selected by all compilers :-)

4069–4070

tot_segments

4137

Can we use .orig directly and not use the tmp variable.

5967

add TODO(pdobarro): if something is a todo item

source/blender/editors/sculpt_paint/sculpt_intern.h
85

tot_segments

391

put pose_ik_chain_len and pose_ik_chain in a PoseIKChain struct.
and update the naming of the fields.

source/blender/makesrna/intern/rna_brush.c
1934

Add a description

This revision now requires changes to proceed.Dec 12 2019, 11:26 AM
Pablo Dobarro (pablodp606) marked 29 inline comments as done.
  • Rebase
  • Review Update

Think we are almost done, just some small clean up/code readability.

source/blender/blenkernel/BKE_paint.h
106

This is codewise not part of the enum, You could add it as a static const.
Leaving it here could render confusing data in a debug session. During debugging the debugger will print AREA_SYMM_DEFAULT, when user hasn't selected anything).

source/blender/editors/sculpt_paint/sculpt.c
1338

the flip_qt_qt uses ePaintSymmetryFlags we should clean up replace all const char symm parameters with a const ePaintSymmetryFlags symm.

3724

don't use char

3752

Doesn't need to be casted to char.

10371

unneeded cast to char in this function. Would it make sense to change int i to ePaintSymmetryAreas symm. Or convert the i inside the brackets to ePaintSymmetryAreas symm.

The problem with casting to char is that when we want to add more stuff to the ePaintSymmetryAreas, the char might trigger undesired results that are hard to track.

Jeroen Bakker (jbakker) requested changes to this revision.Dec 19 2019, 9:11 AM
This revision now requires changes to proceed.Dec 19 2019, 9:11 AM
Pablo Dobarro (pablodp606) marked 5 inline comments as done.
  • Review update
Jeroen Bakker (jbakker) requested changes to this revision.Dec 20 2019, 2:01 PM

Compilation Warning

[422/799] Building C object source/blender/editors/sculpt_paint/CMakeFiles/bf_editor_sculpt_paint.dir/sculpt.c.o
/home/jeroen/blender-git/blender/source/blender/editors/sculpt_paint/sculpt.c: In function ‘sculpt_pose_ik_chain_init’:
/home/jeroen/blender-git/blender/source/blender/editors/sculpt_paint/sculpt.c:4092:64: warning: passing argument 3 of ‘sculpt_nearest_vertex_get’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
   int nearest_vertex_index = sculpt_nearest_vertex_get(sd, ob, initial_location, FLT_MAX, true);
                                                                ^~~~~~~~~~~~~~~~
/home/jeroen/blender-git/blender/source/blender/editors/sculpt_paint/sculpt.c:445:12: note: expected ‘float *’ but argument is of type ‘const float *’
 static int sculpt_nearest_vertex_get(
            ^~~~~~~~~~~~~~~~~~~~~~~~~

Proposed fix:

1diff --git a/source/blender/editors/sculpt_paint/sculpt.c b/source/blender/editors/sculpt_paint/sculpt.c
2index 3b825c47100..00d201375e3 100644
3--- a/source/blender/editors/sculpt_paint/sculpt.c
4+++ b/source/blender/editors/sculpt_paint/sculpt.c
5@@ -443,7 +443,7 @@ static void nearest_vertex_get_reduce(const void *__restrict UNUSED(userdata),
6 }
7
8 static int sculpt_nearest_vertex_get(
9- Sculpt *sd, Object *ob, float co[3], float max_distance, bool use_original)
10+ Sculpt *sd, Object *ob, const float co[3], float max_distance, bool use_original)
11 {
12 SculptSession *ss = ob->sculpt;
13 PBVHNode **nodes = NULL;
14@@ -1926,7 +1926,8 @@ float tex_strength(SculptSession *ss,
15 bool sculpt_search_sphere_cb(PBVHNode *node, void *data_v)
16 {
17 SculptSearchSphereData *data = data_v;
18- float *center, nearest[3];
19+ const float *center;
20+ float nearest[3];
21 if (data->center) {
22 center = data->center;
23 }
24diff --git a/source/blender/editors/sculpt_paint/sculpt_intern.h b/source/blender/editors/sculpt_paint/sculpt_intern.h
25index 5ef0479069f..e8a6369c3b5 100644
26--- a/source/blender/editors/sculpt_paint/sculpt_intern.h
27+++ b/source/blender/editors/sculpt_paint/sculpt_intern.h
28@@ -259,7 +259,7 @@ typedef struct {
29 struct Sculpt *sd;
30 struct SculptSession *ss;
31 float radius_squared;
32- float *center;
33+ const float *center;
34 bool original;
35 bool ignore_fully_masked;
36 } SculptSearchSphereData;

source/blender/editors/sculpt_paint/sculpt.c
4092

I see this variable, but still a few calls to the same function. Could that be improved?
You can also add a const qualifier.

4134

I got a segmentation error here. weights are not always initialized. Root cause: br->pose_ik_segments == 0

This revision now requires changes to proceed.Dec 20 2019, 2:01 PM
Pablo Dobarro (pablodp606) marked 2 inline comments as done.
  • Review update
Jeroen Bakker (jbakker) requested changes to this revision.Mon, Dec 30, 8:52 AM

Yes you fixed the crash, but not the root cause :-)

when starting blender the pose brush is set to 0 segments. Check with --factory-startup fixing this should be the right solution.

source/blender/blenloader/intern/versioning_280.c
4321

this code seems not be correct. When starting blender with a factory settings the pose brush still has 0 pose_ik_segments

source/blender/editors/sculpt_paint/sculpt.c
4196

this seems to be a patch, not a fix. pose_ik_segments must never be zero.

This revision now requires changes to proceed.Mon, Dec 30, 8:52 AM
Pablo Dobarro (pablodp606) marked 2 inline comments as done.
  • Fix versioning code
Jeroen Bakker (jbakker) requested changes to this revision.Fri, Jan 3, 11:05 AM
Jeroen Bakker (jbakker) added inline comments.
release/scripts/startup/bl_ui/properties_paint_common.py
107

Is this change part of this patch or should it be added to a different patch?

source/blender/editors/sculpt_paint/sculpt.c
4072

At this point it is not clear why [0].weights isn't freed.
better set it to NULL here and add a comment to the segments fields about [0] ownership.
Other devs can look at the code and make a wrong decision on the current code.

4197

this should never happen, so could be removed?

This revision now requires changes to proceed.Fri, Jan 3, 11:05 AM
Pablo Dobarro (pablodp606) marked 3 inline comments as done.
  • Rebase
  • Review Update
release/scripts/startup/bl_ui/properties_paint_common.py
107

These properties are needed for this patch. pose_smooth_iterations was introduced in a previous patch, but I think it was accidentally removed in the refactor.

We're close.

I did see some additional changes were added to this patch that also seemed to be unrelated (alpha of the add_col and sub_col). best to add that to a separate patch.

Error: Not freed memory blocks: 3, total unfreed memory 0.007507 MB
Pose IK Chain len: 16 0x7f13402ec728
Pose IK Chain Segments len: 1616 0x7f133fbf4338
Pose IK weights len: 2304 0x7f1338499e38

If we can fix this it is good to commit.

Jeroen Bakker (jbakker) requested changes to this revision.Tue, Jan 7, 8:45 AM
This revision now requires changes to proceed.Tue, Jan 7, 8:45 AM
  • Free the IK chain preview whith the SculptSession

When committing can you separate the brush alpha and the rest of the patch?

This revision is now accepted and ready to land.Tue, Jan 7, 4:01 PM