Page MenuHome

Cloth: Fix T65568: sewing and self collision issue
ClosedPublic

Authored by Ish Bosamiya (ish_bosamiya) on Feb 22 2020, 8:45 PM.

Details

Summary

As explained in the Task T65568 by @Luca Rood (LucaRood), the self collision system should exclude triangles that are connected by sewing springs. This patch does the same.

Diff Detail

Repository
rB Blender
Branch
fix_self_collision_with_sewing (branched from master)
Build Status
Buildable 6738
Build 6738: arc lint + arc unit

Event Timeline

Thanks for the quick patch!

This disables collisions between any two triangles that have a sewing spring at all, not only triangles connected by the same sewing spring. I'm not sure how important this is... It seems pretty unlikely to be an issue in practice, but I'd like to hear the thoughts of the other reviewers on this.
Basically, the question is if this is good enough, or if we actually want to spend the extra time checking if the two triangles are connected by one sewing spring. This would only need to be checked for a small percentage of collisions, so maybe the cost is not too high. Is it worth maybe adding a sewing spring index to ClothVertex? What about verts with multiple sewing springs?

Other than this more conceptual issue, see my inline comments.

source/blender/blenkernel/intern/collision.c
1192–1194

This can be simplified to:

bool sewing_active = (clmd->sim_parms->flags & CLOTH_SIMSETTINGS_FLAG_SEW);
1603–1605

Same as above.

This solution is efficient. But it is not the most correct since it makes the self collision of cloths like this fail completely:

I would like to see other alternatives.

Perhaps we can use a BLI_BITMAP to construct a lookup matrix to tell if two verts are connected with a sewing edge?
(see blender/blenkernel/intern/cloth.c:1579 for how I used this in kinda the same manner when creating internal springs)

So this bitmap would be created once and then used to see if vertex A is connected to vertex B with a sewing edge. Or we could even simplify it down to faces instead of vertices.

Improve sewing edges check by using bitmap

  • simplify boolean check for sewing_active
  • use bitmap for sewing edges test

This implementation seems to be the best of both worlds, its fast and it doesn't take up too much extra space.

Ish Bosamiya (ish_bosamiya) marked 2 inline comments as done.Feb 24 2020, 12:51 PM

Although it is a bitmap, mvert_num * mvert_num worries me a little.
I wonder if making a second array of vertices with the sewing edges index would consume less. (The problem with this is that a vertex can connect to more than one swinging edge)

For 20,000 vertices with say 10% having sewing edges,

BLI_bitmap takes:
50000004 bytes = 47.68 MB

BLI_array takes:
20000 * 8 bytes (for the array pointer) + 2000 * 4 bytes (for the vertices with sewing edges) which is 168000 bytes = 0.16 MB

With this rough calculation, BLI_array is much more space efficient, so you are right @Germano Cavalcante (mano-wii) . The reallocations would be minimal (if any) since most simulations with sewing have only one sewing edge per vertex, so BLI_array would be just as efficient in terms of time as the bitmap alternative.

So should I rewrite the patch to use BLI_array?

The difference shows that the BLI_bitmap (at least as it is now) should be avoided.
But BLI_array would not solve the problem of more than one sewing edge per vertex.
This could be a known limitation, but I would prefer a solution to be found.

Perhaps we can use a set (gset in blender) to specify all the vertex indexes that form a sewing edge vertex pair?
This way, we should in theory only allocate memory for the edge pairs that are sewing edges and we will be able to handle multiple sewing edges per vertex.

So we only store the cloth->mvert_num * tri_a->tri[i] + tri_b->tri[j] keys in the set and then simply query the set to see if the current vertex pair is in there.

So just some pseudo code:

sew_edge_graph = BLI_gset_ptr_new("cloth_sewing_edges_graph")

for all sewing edge vertex pairs:
  BLI_gset_insert(sew_edge_graph,  cloth->mvert_num * tri_a->tri[i] + tri_b->tri[j]))
  BLI_gset_insert(sew_edge_graph,  cloth->mvert_num * tri_a->tri[j] + tri_b->tri[i]))


During collide check ->

if BLI_gset_haskey(sew_edge_graph, cloth->mvert_num * tri_a->tri[i] + tri_b->tri[j]))
  Don't collide
else:
  collide

BLI_gset_free(sew_edge_graph)

@Sebastian Parborg (zeddb), good idea. We can also use int[2] as a key. Something similar is done with edgetri_cache in bmesh_intersect.c (but there is GHash and int[4]).

@Germano Cavalcante (mano-wii) Ah, that works too. Perhaps it is for the better also to use an array pair. (Might be more obvious what is going on and we don't have to worry about overflowing with the int addition)

Now using GHash with inthash_v2

Looks good. I'm not sure about the changes to the GHash code. Maybe @Campbell Barton (campbellbarton) could take a look at that part?

source/blender/blenkernel/intern/cloth.c
858

I'm not sure if ghash needs to be created here.
Maybe just on demand? (In the code below at /* Structural springs. */)

1686

There is no need to add two pairs, just add one as long as the first index is less than the second.
Overlaps already work that way (index1 < index2).
Otherwise you could change the order when testing.

source/blender/blenkernel/intern/collision.c
1192–1194

sewing_active is only used in the DEBUG build.
Use a #ifdef DEBUG that includes this and the code inside BLI_assert below.

Updated with the requested changes.

Ish Bosamiya (ish_bosamiya) marked 3 inline comments as done.Feb 26 2020, 3:23 PM

LGFM, but it might be good to wait another day to see what other reviewers think.

This revision is now accepted and ready to land.Feb 26 2020, 3:33 PM

LGFM, but it might be good to wait another day to see what other reviewers think.

Ok