Page MenuHome

Sculpt face iterators
ClosedPublic

Authored by Joseph Eagar (joeedh) on Oct 11 2022, 11:14 PM.

Details

Summary

This patch adds basic face iterators to the sculpt API. The interface is similar to the existing vertex iterators. It's not C++ (though it does mark private fields in PBVHFaceIter as private if compiling under C++).

Example:

PBVHFaceIter fd;

BKE_pbvh_face_iter_begin(pbvh, node, fd) {

  /* Face reference and face index */
  PBVHFaceRef face = fd->face;
  int face_index = fd->index;
  
  /* Can read and modify hide flag if it exist (it may not) */
  if (fd->hide) {
    *fd->hide ^= true; /* toggle hide */
  }

  /* Can read and modify face set if it exists */
  if (fd->face_set) {
    *fd->face_set = something;
  }

  /*Can read vertices*/
  for (int i=0; i<fd.verts_num; i++) {
    float *co = SCULPT_vertex_co_get(ss, fd.verts[i]);
  }
} 
BKE_pbvh_face_iter_end(fd);

Diff Detail

Event Timeline

Joseph Eagar (joeedh) requested review of this revision.Oct 11 2022, 11:14 PM
Joseph Eagar (joeedh) created this revision.
Joseph Eagar (joeedh) edited the summary of this revision. (Show Details)
Joseph Eagar (joeedh) edited the summary of this revision. (Show Details)
Joseph Eagar (joeedh) edited the summary of this revision. (Show Details)Oct 11 2022, 11:18 PM
Joseph Eagar (joeedh) edited the summary of this revision. (Show Details)

Don't require deferencing.

Can you say more about the semantics of this for grids, does it visit base mesh or tessellated faces?

What is the purpose of the hash? And every visited face seems to be inserted into it, so SmallHash may not be appropriate?

Can you say more about the semantics of this for grids, does it visit base mesh or tessellated faces?

What is the purpose of the hash? And every visited face seems to be inserted into it, so SmallHash may not be appropriate?

It iterates over the base mesh faces (sculpt mode has no concept of grid faces; it always uses the base mesh faces). SmallHash is used because I wanted a lightweight open addressing hash table. I'm actually thinking of using blender::Set instead. The reason I used a hash table at all was to avoid having to refactor PBVH so that all three modes assign base mesh faces into only one PBVHNode (only PBVH_BMESH guarantees this at the moment).

Can you then add a comment explaining this iterates over base mesh faces?

I'd expect the primitives in the PBVHNode that belong to the same poly to be stored consecutively, so that you can avoid the hash? At least I can't think of an obvious reason why they should get out of order in the PBVH building.

Can you then add a comment explaining this iterates over base mesh faces?

I can do that. I should probably add a comment explaining this to PBVHFaceRef as well.

I'd expect the primitives in the PBVHNode that belong to the same poly to be stored consecutively, so that you can avoid the hash? At least I can't think of an obvious reason why they should get out of order in the PBVH building.

The node builders for PBVH_GRIDS and PBVH_FACES work on 'primitives' (MLoopTris for PBVH_FACES and grids for PBVH_GRIDS). As far as I could tell the code doesn't force primitives from the same face to belong to single nodes.

Fix multires crash.

Brecht Van Lommel (brecht) requested changes to this revision.Oct 17 2022, 12:14 PM

The node builders for PBVH_GRIDS and PBVH_FACES work on 'primitives' (MLoopTris for PBVH_FACES and grids for PBVH_GRIDS). As far as I could tell the code doesn't force primitives from the same face to belong to single nodes.

The hash was removed now, but this comment seems to indicate that doesn't work?

I can see that if you are iterating all faces in a subset of PBVH nodes, you need some way to keep track of which faces you have already visited. Although on the other hand if you were using multithreading with a different iterator for each thread, you'd need a different solution anyway. So maybe the problem does not need to be solved at the level of the iterator anyway.

source/blender/blenkernel/BKE_pbvh.h
43

No longer needed.

source/blender/blenkernel/intern/pbvh.c
15

No longer needed.

3516

This leaks memory if fd->verts was heap allocated.

This revision now requires changes to proceed.Oct 17 2022, 12:14 PM

The node builders for PBVH_GRIDS and PBVH_FACES work on 'primitives' (MLoopTris for PBVH_FACES and grids for PBVH_GRIDS). As far as I could tell the code doesn't force primitives from the same face to belong to single nodes.

The hash was removed now, but this comment seems to indicate that doesn't work?

I changed the node builders to respect original face boundaries in master.

Joseph Eagar (joeedh) marked 3 inline comments as done.
Brecht Van Lommel (brecht) requested changes to this revision.Nov 7 2022, 7:34 PM
Brecht Van Lommel (brecht) added inline comments.
source/blender/blenkernel/intern/pbvh.c
3518–3523

I think this still leaks? Here it's overwriting the fd->verts pointer which may have been heap allocated.

Maybe simpler logic would be to set fd->verts = fd->verts_reserved_ and fd->verts_size_ = PBVH_FACE_ITER_VERTS_RESERVED when the iterator is initialized. And then above 5 lines of code can be eliminated entirely.

This revision now requires changes to proceed.Nov 7 2022, 7:34 PM
Joseph Eagar (joeedh) marked an inline comment as done.

Move initialization of PBVHFaceIter->verts to BKE_pbvh_face_iter_init.

source/blender/blenkernel/intern/pbvh.c
3518–3523

How would the pointer be heap allocated? Verts_num increases monotonically.

Anyway I moved the initialization as suggested.

This revision is now accepted and ready to land.Nov 10 2022, 10:41 PM

Sync with latest master

This revision was automatically updated to reflect the committed changes.