Page MenuHome

Sculpt: add pose origin preview to the pose brush
ClosedPublic

Authored by Pablo Dobarro (pablodp606) on Wed, Sep 11, 4:17 PM.
Tags
None
Subscribers
None
Tokens
"Love" token, awarded by jfmatheu."Like" token, awarded by TheAngerSpecialist."Love" token, awarded by brilliant_ape."Like" token, awarded by Way."Love" token, awarded by billreynish."Love" token, awarded by belich."Love" token, awarded by Rusculleda.

Details

Summary

This makes the pose brush easier to control

Diff Detail

Repository
rB Blender

Event Timeline

Jeroen Bakker (jbakker) requested changes to this revision.Thu, Sep 12, 12:42 PM
Jeroen Bakker (jbakker) added inline comments.
source/blender/editors/sculpt_paint/paint_cursor.c
1368

This introduces a memory leak as it never frees the allocated memory, not something you want to do during drawing.

As we only need the pose_origin, can we extract that to a function and call it in both places.

Calculating the pose_origin is heavy. Is it an option to calculate it only when it changes. This will improve the performance when TAA is enabled for example.

1418

Reset depth testing after use. On my VEGA it shows artifacts when used.

This revision now requires changes to proceed.Thu, Sep 12, 12:42 PM
Pablo Dobarro (pablodp606) marked 2 inline comments as done.Thu, Sep 12, 3:17 PM
Pablo Dobarro (pablodp606) updated this revision to Diff 18156.
  • Update pose origin preview only on active vertex changes
  • Don't allocate memory to update the preview
  • Reset depth test after drawing preview
Way awarded a token.Thu, Sep 12, 11:38 PM

Not related to these changes, just a note current master also has a memory leak. Looking at the code I cannot find the freeing of the pose_factor. We should fix this in master.

Error: Not freed memory blocks: 2, total unfreed memory 1.531639 MB
Pose factor len: 801996 0x7fdf38716038
BKE_pbvh_search_gather len: 1024 0x7fdf39d84b38

Also a general note is (totally not related to this patch) but we could think about making the brush drawing relative to the screen DPI. This way it will look the same in low and high DPI displays.

source/blender/editors/sculpt_paint/paint_cursor.c
1460

GPU_depth_test resets the GPU's what should be minimalized. One (or both) statements seems to be incorrect.

source/blender/editors/sculpt_paint/sculpt.c
3612–3613

I would still suggest to split this function as it is also called outside the initialization of the brush. It makes the code more readable.

Something like this.

1/* Calculate the pose origin and (Optionaly the pose factor) that is used when using the pose brush
2 *
3 * r_pose_origin must be a valid pointer. the r_pose_factor is optional. When set to NULL it won't
4 * be calculated. */
5void sculpt_pose_calc_pose_data(Sculpt *sd,
6 Object *ob,
7 SculptSession *ss,
8 float initial_location[3],
9 float radius,
10 float *r_pose_origin,
11 float *r_pose_factor)
12{
13 const bool calc_pose_factor = (r_pose_factor != NULL);
14
15 sculpt_vertex_random_access_init(ss);
16
17 float pose_origin[3];
18 float pose_initial_co[3];
19
20 copy_v3_v3(pose_initial_co, initial_location);
21
22 char *visited_vertices = MEM_callocN(sculpt_vertex_count_get(ss) * sizeof(char),
23 "Visited vertices");
24 BLI_Stack *not_visited_vertices = BLI_stack_new(sizeof(VertexTopologyIterator),
25 "not visited vertices stack");
26
27 float tot_co = 0;
28 zero_v3(pose_origin);
29
30 VertexTopologyIterator mevit;
31
32 /* Add active vertex and symmetric vertices to the stack. */
33 const char symm = sd->paint.symmetry_flags & PAINT_SYMM_AXIS_ALL;
34 for (char i = 0; i <= symm; ++i) {
35 if (is_symmetry_iteration_valid(i, symm)) {
36 float location[3];
37 flip_v3_v3(location, sculpt_vertex_co_get(ss, sculpt_active_vertex_get(ss)), (char)i);
38 mevit.v = -1;
39 if (i == 0) {
40 mevit.v = sculpt_active_vertex_get(ss);
41 }
42 else {
43 if (calc_pose_factor) {
44 mevit.v = sculpt_nearest_vertex_get(sd, ob, location, radius * radius, false);
45 }
46 }
47 if (mevit.v != -1) {
48 mevit.it = 1;
49 BLI_stack_push(not_visited_vertices, &mevit);
50 }
51 }
52 }
53
54 /* Flood fill the internal pose brush factor. Calculate the pose rotation point based on the
55 * boundaries of the brush factor*/
56 while (!BLI_stack_is_empty(not_visited_vertices)) {
57 VertexTopologyIterator c_mevit;
58 BLI_stack_pop(not_visited_vertices, &c_mevit);
59 SculptVertexNeighborIter ni;
60 sculpt_vertex_neighbors_iter_begin(ss, c_mevit.v, ni)
61 {
62 if (visited_vertices[(int)ni.index] == 0) {
63 VertexTopologyIterator new_entry;
64 new_entry.v = ni.index;
65 new_entry.it = c_mevit.it + 1;
66 if (calc_pose_factor) {
67 r_pose_factor[new_entry.v] = 1.0f;
68 }
69 visited_vertices[(int)ni.index] = 1;
70 if (sculpt_pose_brush_is_vertex_inside_brush_radius(
71 sculpt_vertex_co_get(ss, new_entry.v), pose_initial_co, radius, symm)) {
72 BLI_stack_push(not_visited_vertices, &new_entry);
73 }
74 else {
75 if (check_vertex_pivot_symmetry(
76 sculpt_vertex_co_get(ss, new_entry.v), pose_initial_co, symm)) {
77 tot_co++;
78 add_v3_v3(pose_origin, sculpt_vertex_co_get(ss, new_entry.v));
79 }
80 }
81 }
82 }
83 sculpt_vertex_neighbors_iter_end(ni)
84 }
85
86 BLI_stack_free(not_visited_vertices);
87 MEM_freeN(visited_vertices);
88
89 if (tot_co > 0) {
90 mul_v3_fl(pose_origin, 1.0f / (float)tot_co);
91 }
92 copy_v3_v3(r_pose_origin, pose_origin);
93}
94
95static void sculpt_pose_brush_init(
96 Sculpt *sd, Object *ob, SculptSession *ss, Brush *br, float initial_location[3], float radius)
97{
98 float *pose_factor = MEM_callocN(sculpt_vertex_count_get(ss) * sizeof(float), "Pose factor");
99
100 sculpt_pose_calc_pose_data(
101 sd, ob, ss, initial_location, radius, ss->cache->pose_origin, pose_factor);
102
103 copy_v3_v3(ss->cache->pose_initial_co, initial_location);
104 ss->cache->pose_factor = pose_factor;
105
106 /* Smooth the pose brush factor for cleaner deformation */
107 PBVHNode **nodes;
108 PBVH *pbvh = ob->sculpt->pbvh;
109 int totnode;
110
111 BKE_pbvh_search_gather(pbvh, NULL, NULL, &nodes, &totnode);
112
113 SculptThreadedTaskData data = {
114 .sd = sd,
115 .ob = ob,
116 .brush = br,
117 .nodes = nodes,
118 };
119
120 for (int i = 0; i < 4; i++) {
121 TaskParallelSettings settings;
122 BLI_parallel_range_settings_defaults(&settings);
123 settings.use_threading = ((sd->flags & SCULPT_USE_OPENMP) && totnode > SCULPT_THREADED_LIMIT);
124 BLI_task_parallel_range(0, totnode, &data, pose_brush_init_task_cb_ex, &settings);
125 }
126}

3648

mevit.v is not set when is_symmetry_iteration_valid returns false, and not reset in the next loop. So it can be 0.

Seems to be code-wise incorrect, but I am not up to speed with the exact details here. Best that you decide if this implementation is correct.

If it is correct, we should add a comment why.

Jeroen Bakker (jbakker) requested changes to this revision.Fri, Sep 13, 10:02 AM
This revision now requires changes to proceed.Fri, Sep 13, 10:02 AM
source/blender/editors/sculpt_paint/paint_cursor.c
1413

This behavior seems to be different then shown in the video. Is this intentionally. Now the line intersects with mesh very easily and you don't see it.

You could also draw the line twice. one in back (with a higher alpha) and one in front. This makes it more clear to the user where the intersection takes place.

Pablo Dobarro (pablodp606) marked 2 inline comments as done.Fri, Sep 13, 4:17 PM
Pablo Dobarro (pablodp606) updated this revision to Diff 18205.
  • Fix memory leak
  • Refactor init pose
  • Disable depth test

I disabled the depth test in the pose preview and the dynamic mesh preview. The idea is to draw this lines on top of everything in the scene, but interacting with another preview elements (symmetry plane previews, brush textures...).
Do you think that the depth test is also the cause of T69728?

This revision is now accepted and ready to land.Tue, Sep 17, 8:16 AM