Page MenuHome
Differential D5788

Add the ability to create internal springs to the cloth sim
ClosedPublic
Actions

Authored by Sebastian Parborg (zeddb) on Sep 13 2019, 3:53 PM.

Details

Reviewers
Brecht Van Lommel (brecht)
Jacques Lucke (JacquesLucke)
Sergey Sharybin (sergey)
Commits
rB62ef59aa0cca: Add the ability to create internal springs to the cloth sim
Summary

This will make closed surfaces behave more like a soft body.

As always, I'm a bit uncertain with the naming of options. So I'm open to alternative naming if there is something more proper.

On of the things that is left to do is to decide what to do when dynamic base mesh is on. Having a dynamic base mesh will force recalculation of springs and will currently not work with this. I'm guessing I could simply disable internal springs if dynamic base mesh is on?

@Campbell Barton (campbellbarton) @Germano Cavalcante (mano-wii) sometimes (with suzanne for example) this triggers and asset in blender/blenlib/intern/BLI_kdopbvh.c:1292
BLI_assert(!((flag & BVH_OVERLAP_RETURN_PAIRS) && (flag & ~BVH_OVERLAP_BREAK_ON_FIRST)));

I do not set these flags with my code, so I'm guessing this is an issue with defaults?

Video:

Left: No internal springs
Middle: Internal springs
Right: Internal springs with vertex groups with animated properties

Diff Detail

Event Timeline

Sebastian Parborg (zeddb) updated this revision to Diff 18204.Sep 13 2019, 3:53 PM
Sebastian Parborg (zeddb) created this revision.
Jacques Lucke (JacquesLucke) requested changes to this revision.Sep 13 2019, 4:27 PM
Jacques Lucke (JacquesLucke) added inline comments.
release/scripts/startup/bl_ui/properties_physics_cloth.py
186

I think title case should be used here, i.e. Max Spring Length. Same below.

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

Should maybe named something like find_internal_spring_target_vertex.

1391

Should this be r_rayhit and be the last parameter?

1394

max_distance?

1396

Are there cases when we want this to be false?

1459

Looks like rayhit->index is two different things in different cases here. Sometimes it is an index into the looptri array, and sometimes a vertex index. You should avoid using the same variable for that.

In fact, I don't see why BVHTreeRayHit should be passed from the outside at all.

1513

use_internal_springs

1529

Can you just use a separate code path for the case when internal springs are built? It feels unnecessary to squeeze this into this loop and then have a condition in every iteration.

1545

Wait, every spring is separately allocated? Can you use some mempool?

1547

I'm surprised to see that you handled failed allocation here. That's rarely done in Blender, but cannot hurt to do. Just wonder if it is worth the additional code path.

source/blender/makesdna/DNA_cloth_types.h
151

Comments should end with a dot.

152

Just "internal" does not tell me that this is about springs. So either put this into a separate struct that only contains information about the (internal) springs, or call it something like internal_spring_max_length.
Same below and for the RNA property names.

This revision now requires changes to proceed.Sep 13 2019, 4:27 PM
Germano Cavalcante (mano-wii) added a comment.Sep 13 2019, 5:22 PM

Here the commit to fix the assert rBf7e8b580989e

Sebastian Parborg (zeddb) added inline comments.Sep 16 2019, 12:22 PM
source/blender/blenkernel/intern/cloth.c
1391

I'm not sure what you mean here?

1396

Yes, there are certain setups where you do not what it to check for normals.
When using meshes like Suzanne this can be useful to make sure that the eyes are mounted to the eyelids.

1529

But then we would have to iterate over the mesh two times. But perhaps looping over two times in not that big of an issue?

1545

I simply copied the spring creation code. This is how it is done in the later part of this file.

1547

Same as above, this is simply how the other code does it too.

I guess we could rewrite it if you feel that it would be better.

Jacques Lucke (JacquesLucke) added inline comments.Sep 16 2019, 1:24 PM
source/blender/blenkernel/intern/cloth.c
1391

Usually parameters that return stuff to the caller should have the r_ prefix and be at the end of the parameter list.
However, BVHTreeRayHit shouldn't be a parameter in the first place, so it doesn't matter here.

1529

Doing the raycast is far more expensive than having a loop over all vertices. So an additional loop should not make the performance worse in any noticeable way.

Sebastian Parborg (zeddb) updated this revision to Diff 19876.Nov 27 2019, 12:35 PM
Sebastian Parborg (zeddb) marked 12 inline comments as done.

Updated the patch based on the feedback given.

Sergey Sharybin (sergey) added a comment.Nov 28 2019, 11:40 AM

@Sebastian Parborg (zeddb) for communicating improvements like that ios always great to show some simple simulation side-by-side with and without internal springs.

Currently the descriptions are quite low-level. Is it possible to add some more natural explanations of their effect (just speculating: "Higher values makes cloth more rigid") ?

Sebastian Parborg (zeddb) updated this revision to Diff 20077.Dec 4 2019, 5:50 PM

Updated diff to latest master.

No need to review this now, I want to do some changes to the code. I just wanted to have a version that at least applies to the current master here.

Sebastian Parborg (zeddb) updated this revision to Diff 20127.Dec 6 2019, 2:34 PM
Sebastian Parborg (zeddb) edited the summary of this revision. (Show Details)

Updated with vertex group support and made the internal springs its own spring type with separate stiffness settings.

I'm guessing there should be something that groups together the "Max compression" settings with the vertex group. Though sort of how it is currently done with the other springs. So at least is as confusing as the existing spring settings for the cloth sim.

@Sergey Sharybin (sergey) I've added a video:

Left: No internal springs
Middle: Internal springs
Right: Internal springs with vertex groups with animated properties

Fin O'Riordan (fin.eskimo) awarded a token.Dec 6 2019, 5:14 PM
Fin O'Riordan (fin.eskimo) added a subscriber: Fin O'Riordan (fin.eskimo).
Sebastian Parborg (zeddb) updated this revision to Diff 20131.Dec 6 2019, 5:40 PM

Made the patch apply with latest master...

Jacques Lucke (JacquesLucke) added a comment.Dec 7 2019, 10:29 AM

I assume graph_edit.c should not be part of this patch?

Jacques Lucke (JacquesLucke) accepted this revision.Dec 7 2019, 11:01 AM

All requested changes are smaller style issues that should be addressed before merging. The main functionality seems fine and works in my test.

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

Copy-pasted comment. Should probably be changed.

Also comments should end with .. This is missing in most of your comments.

1431

The declarations of vert_idx, mloop and lt can be moved further down into a smaller scope.

source/blender/makesdna/DNA_cloth_types.h
165

length an

source/blender/makesrna/intern/rna_cloth.c
825

an

839

surface point normal -> vertex normal?

source/blender/physics/intern/BPH_mass_spring.cpp
476 ↗(On Diff #20131)

In cases like this I'd usually do one of the following:

  1. use a switch statement with BLI_assert(false) in the default case.
  2. do else if (s->type & CLOT_SPRING_TYPE_INTERNAL)
  3. put BLI_assert(s->type & CLOT_SPRING_TYPE_INTERNAL) as first line after the else.

Pick one. The comment is not necessary anymore then.

477 ↗(On Diff #20131)

Declare and initialize in the same line when possible.

492 ↗(On Diff #20131)

This comment is exactly the same as the one below. This makes it more confusing than without the comment imo. The comment suggests that both lines have the same effect, which is not the case (I think).

Btw, what does the k_ stand for?

This revision is now accepted and ready to land.Dec 7 2019, 11:01 AM
Sebastian Parborg (zeddb) added a comment.Dec 7 2019, 11:36 AM
In D5788#148937, @Jacques Lucke (JacquesLucke) wrote:

I assume graph_edit.c should not be part of this patch?

Yes, my bad. I included an other patch by mistake (which is now commited to master).

Sebastian Parborg (zeddb) added inline comments.Dec 7 2019, 11:49 AM
source/blender/physics/intern/BPH_mass_spring.cpp
492 ↗(On Diff #20131)

k_ stands for the spring constant k. In physics this is used to calculate the force the spring will exert when stretched or compressed.

I guess for artistic freedom, the compression and tension has been split into two variables as the k constant should in theory be the same for both directions.

Or well, in pure spring systems you do not have any damping. But to prevent oscillations you attach a dampener as well as a spring. Then you have a spring constant k and a dampening variable d. So instead of 2 variables (k and d), we now have 4.

You are right that the comments are not really correct. Both needs to be set to zero for it to actually act as if no spring was there.

However if it is only compressed or stretched, only one of them needs to be set to zero for it to act as if no spring was there.
(Because no damping is applied from the tension when compressing etc)

Closed by commit rB62ef59aa0cca: Add the ability to create internal springs to the cloth sim (authored by Sebastian Parborg (zeddb)). · Explain WhyDec 9 2019, 7:16 PM
This revision was automatically updated to reflect the committed changes.
Sebastian Parborg (zeddb) marked 10 inline comments as done.
Sebastian Parborg (zeddb) added a commit: rB62ef59aa0cca: Add the ability to create internal springs to the cloth sim.

Revision Contents
Changeset List

PathSize
release/
scripts/
startup/
bl_ui/
29 lines
source/
blender/
blenkernel/
4 lines
intern/
154 lines
makesdna/
5 lines
makesrna/
intern/
44 lines

Diff 20077

View Options

release/scripts/startup/bl_ui/properties_physics_cloth.py

Loading...
View Options

source/blender/blenkernel/BKE_cloth.h

Loading...
View Options

source/blender/blenkernel/intern/cloth.c

Loading...
View Options

source/blender/makesdna/DNA_cloth_types.h

Loading...
View Options

source/blender/makesrna/intern/rna_cloth.c

Loading...