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.
Details
Diff Detail
- Repository
- rB Blender
- Branch
- fix_self_collision_with_sewing (branched from master)
- Build Status
Buildable 6727 Build 6727: 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 | ||
|---|---|---|
| 1187 | This can be simplified to: bool sewing_active = (clmd->sim_parms->flags & CLOTH_SIMSETTINGS_FLAG_SEW); | |
| 1601 | 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.
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)
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 | ||
|---|---|---|
| 848 | I'm not sure if ghash needs to be created here. | |
| 1669 | There is no need to add two pairs, just add one as long as the first index is less than the second. | |
| source/blender/blenkernel/intern/collision.c | ||
| 1187–1192 | sewing_active is only used in the DEBUG build. | |
