Sculpt mesh abstraction API
ClosedPublic
Actions

Authored by Pablo Dobarro (pablodp606) on Jul 27 2019, 12:21 AM.

Details

Reviewers

Brecht Van Lommel (brecht)

Maniphest Tasks

T67518: New sculpting tools

Commits

rB037cf920b42d: Sculpt: mesh abstraction API

Summary

These functions make possible porting the tools from the sculpt branch, making them compatible with PBVH_FACES and #PBVH_BMESH## without duplicating the code. They can also help to simplify some existing code.

I ported some operators in the sculpt branch and they are working fine with dyntopo enabled. I think those are the most problematic operators, but maybe I'll need to add more functions to this API in the future. This should be enough to start making the final patches for most of the tools.

Diff Detail

Repository: rB Blender

Event Timeline

Pablo Dobarro (pablodp606) created this revision.Jul 27 2019, 12:21 AM

Harbormaster completed remote builds in B4183: Diff 16633.Jul 27 2019, 12:21 AM

Pablo Dobarro (pablodp606) added a reviewer: Brecht Van Lommel (brecht).Jul 27 2019, 12:21 AM

Pablo Dobarro (pablodp606) added a task: T67518: New sculpting tools.

Julian Mohr (Jules) awarded a token.Jul 27 2019, 10:15 AM

Juanfran Matheu (jfmatheu) awarded a token.Jul 27 2019, 4:11 PM

This patch relies on the multires memory optimization being removed, otherwise there will be uninitialized variables and crashes. We can probably do this, but just nothing we can't really merge this without that.

Overall approach seems fine, some comments on the implementation.

source/blender/editors/sculpt_paint/sculpt.c
97	All these functions should be `static` since they are not exposed in any header file.
222	Creating a gset and the associated memory allocations here is too slow. Since the typical number of neighbors is low, you can just store them in an array and loop over the array every time to check for duplicates. To avoid the memory allocation, `SculptVertexNeighbourIter` can contain a fixed size array of e.g. `256`. `int *neighbours` would initially point to that. You can then use `MEM_malloc` to double the capacity when that number is exceeded, which would be almost never. The code to do that can also be shared between the bmesh and faces case, if you make a common `sculpt_vertex_neighbours_add` function for both to call.

This revision now requires changes to proceed.Jul 30 2019, 1:20 AM

Replace gset with static arrays
Make sculpt mesh API functions static

Harbormaster completed remote builds in B4219: Diff 16695.Jul 30 2019, 6:25 PM

Ok, if this is fine we can wait to remove the multires optimization for merging, but this should already allow me to start making the patches for most of the new tools

Here's how I think it should work (did not test the code though):

P1058 (An Untitled Masterwork)

1	#define SCULPT_VERTEX_FIXED_CAPACITY 256
2
3	typedef struct SculptVertexNeighbourIter {
4	int *neighbours;
5	int size;
6	int capacity;
7
8	int neighbours_fixed[SCULPT_VERTEX_FIXED_CAPACITY];
9
10	int index;
11	int i;
12	} SculptVertexNeighbourIter;
13
14	static void sculpt_vertex_neighbour_add(SculptVertexNeighbourIter *iter, int neighbour_index)
15	{
16	for (int i = 0; i < iter->size; i++) {
17	if (iter->neighbours[i] == neighbour_index) {
18	return;
19	}
20	}
21
22	if (iter->count >= iter->capacity) {
23	iter->capacity += SCULPT_VERTEX_FIXED_CAPACITY;
24
25	if (iter->neighbours == iter->neighbours_fixed) {
26	iter->neighbours = MEM_mallocN(iter->capacity * sizeof(int), "neighbour array");
27	memcpy(iter->neighbours, iter->neighbours_fixed, sizeof(int) * iter->count);
28	}
29	else {
30	iter->neighbours = MEM_reallocN_id(iter->neighbours,
31	iter->capacity * sizeof(int),
32	"neighbour array");
33	}
34	}
35
36	iter->neighbours[iter->size] = neighbour_index;
37	iter->size++;
38	}
39
40	static void sculpt_vertex_neighbours_get_bmesh(SculptSession *ss,
41	int index,
42	SculptVertexNeighbourIter *iter)
43	{
44	BMVert *v = BM_vert_at_index(ss->bm, index);
45	BMIter liter;
46	BMLoop *l;
47	iter->size = 0;
48	iter->capacity = SCULPT_VERTEX_FIXED_CAPACITY;
49	iter->neighbours = iter->neighbours_fixed;
50
51	...
52
53	#define sculpt_vertex_neighbours_iter_end(neighbour_iterator) \
54	} \
55	if (neighbour_iterator.neighbours != neighbour_iterator.neighbours_fixed) { \
56	MEM_freeN(neighbour_iterator.neighbours); \
57	} \

source/blender/editors/sculpt_paint/sculpt.c
218	This will not reallocate when `iter->count` reaches `2 * SCULPT_VERTEX_NEIGHBOUR_MAX`.
243	Doing one allocation per vertex is still relatively slow, these things tend to work poorly especially with multithreading.

This revision now requires changes to proceed.Jul 31 2019, 1:32 AM

Avoid allocating memory on neighbour iterator

Harbormaster completed remote builds in B4235: Diff 16731.Jul 31 2019, 4:41 PM

Brecht Van Lommel (brecht) accepted this revision.Jul 31 2019, 4:43 PM

This revision is now accepted and ready to land.Jul 31 2019, 4:43 PM

Pablo, as we talked, I think the patch is fine to commit as well as long as you:

Document in the code that the API is not to be used with multi-res
Mention this also in the commit message
Do not commit to master any code that uses this API that do not poll for multi-res usage

Closed by commit rB037cf920b42d: Sculpt: mesh abstraction API (authored by Pablo Dobarro (pablodp606)). · Explain WhyAug 14 2019, 10:59 PM

This revision was automatically updated to reflect the committed changes.

Pablo Dobarro (pablodp606) added a commit: rB037cf920b42d: Sculpt: mesh abstraction API.