source/blender/editors/sculpt_paint/paint_cursor.c
1348	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.
1398	Reset depth testing after use. On my VEGA it shows artifacts when used.

This revision now requires changes to proceed.Sep 12 2019, 12:42 PM

Update pose origin preview only on active vertex changes
Don't allocate memory to update the preview
Reset depth test after drawing preview

Harbormaster completed remote builds in B4931: Diff 18156.Sep 12 2019, 3:17 PM

Nahuel Belich (belich) awarded a token.Sep 12 2019, 10:57 PM

William Reynish (billreynish) awarded a token.Sep 12 2019, 11:33 PM

Way awarded a token.Sep 12 2019, 11:38 PM

Ted Nielsen (brilliant_ape) awarded a token.Sep 13 2019, 12:41 AM

Joseph Brandenburg (TheAngerSpecialist) awarded a token.Sep 13 2019, 4:06 AM

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

1440

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

3611–3612

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.

P1098 Sculpt mode pose brush

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. */
5	void 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
95	static 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	}

3647

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.Sep 13 2019, 10:02 AM

This revision now requires changes to proceed.Sep 13 2019, 10:02 AM

Jeroen Bakker (jbakker) added inline comments.Sep 13 2019, 10:08 AM

source/blender/editors/sculpt_paint/paint_cursor.c
1393	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.

Fix memory leak
Refactor init pose
Disable depth test

Harbormaster completed remote builds in B4952: Diff 18205.Sep 13 2019, 4:17 PM

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?

Juanfran Matheu (jfmatheu) awarded a token.Sep 13 2019, 7:44 PM

Jeroen Bakker (jbakker) accepted this revision.Sep 17 2019, 8:16 AM

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

Closed by commit rB58214ab52a62: Sculpt: Add pose origin preview to the Pose Brush (authored by Pablo Dobarro (pablodp606)). · Explain WhySep 17 2019, 2:46 PM

This revision was automatically updated to reflect the committed changes.

Pablo Dobarro (pablodp606) added a commit: rB58214ab52a62: Sculpt: Add pose origin preview to the Pose Brush.