Curves: initial surface collision for curves sculpt mode #104469

Jacques Lucke · 2023-02-08T14:47:59+01:00

Jacques Lucke commented

2023-02-08 14:47:59 +01:00

During hair grooming in curves sculpt mode, it is very useful when hair strands are prevented from intersecting with the surface mesh. Unfortunately, it also decreases performance significantly so we don't want it to be turned on all the time.

The surface collision is used by the Comb, Pinch and Puff brushes currently.

This was originally based on D15451 but I changed it quite significantly do make it more feasible to integrate in master without introducing new big abstractions that we don't know we'll need. The intersection prevention quality of this patch is worse than the original patch, it's a bit faster though. Overall, perfect collision detection at the cost of bad performance is not necessary for interactive sculpting, because the user can fix small mistakes very quickly. Nevertheless, the quality can probably still be improved significantly without too big slow-downs. This can be done separately from this patch.

Possible future improvements:

"Proper" solver that can handle more complex cases without resulting in intersections.
Support a separate collision collection.

During hair grooming in curves sculpt mode, it is very useful when hair strands are prevented from intersecting with the surface mesh. Unfortunately, it also decreases performance significantly so we don't want it to be turned on all the time. The surface collision is used by the Comb, Pinch and Puff brushes currently. This was originally based on [D15451](https://ro.developer.blender.org/D15451) but I changed it quite significantly do make it more feasible to integrate in master without introducing new big abstractions that we don't know we'll need. The intersection prevention quality of this patch is worse than the original patch, it's a bit faster though. Overall, perfect collision detection at the cost of bad performance is not necessary for interactive sculpting, because the user can fix small mistakes very quickly. Nevertheless, the quality can probably still be improved significantly without too big slow-downs. This can be done separately from this patch. ![image](/attachments/9f31b730-ec5d-424e-a35a-13649b46c35a) Possible future improvements: * "Proper" solver that can handle more complex cases without resulting in intersections. * Support a separate collision collection.

2023-02-06 16-08-09.mp4

2.5 MiB

image.png

402 KiB

❤️ 4 🎉 2

Jacques Lucke added 17 commits 2023-02-08 14:48:01 +01:00

8226dcc72c initial patch from Lukas

ec59d78c34 cleanup

f3bf7b4cd1 fix

220503e7b6 progress

569ee4a9c3 Merge branch 'master' into temp-curves-surface-collision

9945f1f1a5 progress

78c89f9e68 Merge branch 'master' into temp-curves-surface-collision

c58b362a6b cleanup

d4db7dc82d fix

d359dc4648 fix

1f0fe9a8e9 Merge branch 'master' into temp-curves-surface-collision

43261bebae progress

e8991adbfb Curves: Initial surface collision for curves sculpt mode.

Differential Revision: https://developer.blender.org/D17217

e6cd70ca6f Merge branch 'master' into temp-curves-surface-collision

3c130ee5dd fix

3f3364d118 Merge branch 'main' into temp-curves-surface-collision

buildbot/vexp-code-patch-coordinator Build done. Details

b18e5c9cac Merge branch 'main' into temp-curves-surface-collision

Jacques Lucke commented

2023-02-08 14:48:15 +01:00

@blender-bot build

Jacques Lucke added 5 commits 2023-02-09 14:06:43 +01:00

6514ea7aee cleanup

46384860f5 cleanup

23fe08f5a4 cleanup

25c2417d96 Merge branch 'main' into temp-curves-surface-collision

buildbot/vexp-code-patch-coordinator Build done. Details

8bcb13f610 cleanup

Jacques Lucke changed title from ~~WIP: Curves: initial surface collision for curves sculpt mode~~ to Curves: initial surface collision for curves sculpt mode

2023-02-09 14:28:26 +01:00

Jacques Lucke requested review from Hans Goudey 2023-02-09 14:29:25 +01:00

Jacques Lucke requested review from Simon Thommes 2023-02-09 14:29:32 +01:00

Jacques Lucke commented

2023-02-09 14:30:26 +01:00

@blender-bot build

Simon Thommes approved these changes 2023-02-10 12:24:08 +01:00

Simon Thommes left a comment

Seems good to me!

Would of course be great if performance/accuracy could be tweaked more in the future but this is great for a first iteration!

Seems good to me! Would of course be great if performance/accuracy could be tweaked more in the future but this is great for a first iteration!

Hans Goudey approved these changes 2023-02-10 18:24:16 +01:00

Hans Goudey left a comment

Just a couple suggestions about the index mask stuff inline, and a couple other small things. Since it worked in my testing and for Simon and it will probably be changed more in the future, I didn't think too much about the specifics of the collision algorithm. What's there seems reasonable, but I didn't really think about alternatives.

At least for the guide use cases where there are very few initial curves, the performance difference isn't noticeable. It's obviously much different when there are lots of curves though.

Just a couple suggestions about the index mask stuff inline, and a couple other small things. Since it worked in my testing and for Simon and it will probably be changed more in the future, I didn't think too much about the specifics of the collision algorithm. What's there seems reasonable, but I didn't really think about alternatives. At least for the guide use cases where there are very few initial curves, the performance difference isn't noticeable. It's obviously much different when there are lots of curves though.

source/blender/blenlib/intern/index_mask.cc

						
				@ -241,4 +241,10 @@ IndexMask find_indices_from_virtual_array(const IndexMask indices_to_check,

				  return detail::find_indices_based_on_predicate__merge(indices_to_check, sub_masks, r_indices);

				}

				IndexMask find_indices_from_array(const Span<bool> array, Vector<int64_t> &r_indices)

Hans Goudey commented

2023-02-10 17:34:49 +01:00

Can't seem to add a comment there, but this can replace

find_indices_based_on_predicate(indices_to_check, 4096, r_indices, [&](const int64_t i) { return span[i]; });

above.

Can't seem to add a comment there, but this can replace ``` find_indices_based_on_predicate(indices_to_check, 4096, r_indices, [&](const int64_t i) { return span[i]; }); ``` above.

JacquesLucke marked this conversation as resolved

source/blender/editors/sculpt_paint/curves_sculpt_pinch.cc

						
				@ -143,0 +147,4 @@

				    const IndexMask changed_curves_mask = index_mask_ops::find_indices_from_array(changed_curves,

				                                                                                  indices);

				    const Mesh *surface = curves_id_->surface && curves_id_->surface->type == OB_MESH ?

				                              static_cast<Mesh *>(curves_id_->surface->data) :

Hans Goudey commented

2023-02-10 17:40:06 +01:00

static_cast<Mesh *> -> static_cast<const Mesh *>

`static_cast<Mesh *>` -> `static_cast<const Mesh *>`

JacquesLucke marked this conversation as resolved

source/blender/editors/sculpt_paint/curves_sculpt_puff.cc

						
				@ -157,2 +160,3 @@

				    this->puff(curve_weights);

				    this->restore_segment_lengths();

				    Vector<int64_t> changed_curves_indices;

Hans Goudey commented

2023-02-10 17:44:27 +01:00

What about something like this?

    Vector<int64_t> changed_curves_indices;
    index_mask_ops::find_indices_based_on_predicate(
        curve_selection_, 4096, changed_curves_indices, [&](const int64_t i) {
          return curve_weights[i] > 0.0f;
        });

Also, since the mask has to be computed, would it be possible to use the mask in this->puff()?

What about something like this? ``` Vector<int64_t> changed_curves_indices; index_mask_ops::find_indices_based_on_predicate( curve_selection_, 4096, changed_curves_indices, [&](const int64_t i) { return curve_weights[i] > 0.0f; }); ``` Also, since the mask has to be computed, would it be possible to use the mask in `this->puff()`?

Jacques Lucke commented

2023-02-11 13:33:14 +01:00

I might be wrong, but I think that's not equivalent currently. We could refactor things separately to clean this up though.

Note that the index used in the predicate curve_weights[select_i] > 0.0f is select_i, but the index appended to changed_curves_indices is curve_selection_[select_i]. So it's a different one.

I might be wrong, but I think that's not equivalent currently. We could refactor things separately to clean this up though. Note that the index used in the predicate `curve_weights[select_i] > 0.0f` is `select_i`, but the index appended to `changed_curves_indices` is `curve_selection_[select_i]`. So it's a different one.

source/blender/geometry/GEO_curve_constraint_solver.hh

						
				@ -0,0 +4,4 @@

				#include "BKE_curves.hh"

				namespace blender::geometry::curve_constraint_solver {

Hans Goudey commented

2023-02-10 17:46:40 +01:00

Suggestion: curve_constraint_solver -> curve_constraint(s)

I'm guessing the current name is left over from when it was more object oriented

Suggestion: `curve_constraint_solver` -> `curve_constraint(s)` I'm guessing the current name is left over from when it was more object oriented

JacquesLucke marked this conversation as resolved

source/blender/geometry/intern/curve_constraint_solver.cc

						
				@ -0,0 +6,4 @@

				#include "GEO_curve_constraint_solver.hh"

				#include "BKE_bvhutils.h"

Hans Goudey commented

2023-02-10 18:16:07 +01:00

Should probably have the note about the coordinate spaces in this file too

JacquesLucke marked this conversation as resolved

source/blender/geometry/intern/curve_constraint_solver.cc

						
				@ -0,0 +140,4 @@

				          const float3 normalized_slide_direction_cu = math::normalize_and_get_length(

				              slide_direction_cu, slide_direction_length_cu);

				          /* Use pythagorian theory to determine how far to slide. */

Hans Goudey commented

2023-02-10 18:20:24 +01:00

theory -> theorem?

`theory` -> `theorem`?

JacquesLucke marked this conversation as resolved

source/blender/makesrna/intern/rna_curves.c

						
				@ -456,0 +456,4 @@

				  prop = RNA_def_property(srna, "use_sculpt_collision", PROP_BOOLEAN, PROP_NONE);

				  RNA_def_property_boolean_sdna(prop, NULL, "flag", CV_SCULPT_COLLISION_ENABLED);

				  RNA_def_property_ui_text(

				      prop, "Use Sculpt Collision", "Enable collision handling with the surface during sculpting");