Page MenuHome

Curves: New Grow/Shrink brush.
ClosedPublic

Authored by Jacques Lucke (JacquesLucke) on Mar 28 2022, 1:15 PM.

Details

Summary

This adds a new Grow/Shrink brush which is similar to the Length brush in the old hair system.

  • It's possible to switch between growing and shrinking by hold down ctrl and/or by changing the direction enum.
  • 3d brush is supported.
  • Different brush falloffs are supported.
  • Supports scaling curves uniformly or shrinking/extrapolating them. Extrapolation is linear only in this patch.
  • A minimum length settings helps to avoid creating zero-sized curves.

Ref T96447.

Diff Detail

Repository
rB Blender
Branch
grow-shrink-brush (branched from master)
Build Status
Buildable 21386
Build 21386: arc lint + arc unit

Event Timeline

Jacques Lucke (JacquesLucke) requested review of this revision.Mar 28 2022, 1:15 PM
Jacques Lucke (JacquesLucke) created this revision.
Jacques Lucke (JacquesLucke) retitled this revision from Curves: New Grow/Shrink brush. to Curves: New Grow/Shrink brush. (WIP).Mar 28 2022, 1:16 PM
Jacques Lucke (JacquesLucke) edited the summary of this revision. (Show Details)
  • 3d brush
  • allow switching between growing and shrinking with an enum
  • add minimum length setting
Jacques Lucke (JacquesLucke) retitled this revision from Curves: New Grow/Shrink brush. (WIP) to Curves: New Grow/Shrink brush..Mar 31 2022, 1:40 PM
Jacques Lucke (JacquesLucke) edited the summary of this revision. (Show Details)

I found some weird behavior:

  • Add default primitive
  • Grow the curves until they are long and mostly straight
  • Shrink the curves until the disappear
  • The bounding box is infinite/weird now, and sometimes curves pop out in a random direction.
source/blender/editors/sculpt_paint/curves_sculpt_grow_shrink.cc
41–47

Campbell recently did this in 9ae98f305a4c2ef226ad7, might as well do that now I guess.

96

This is very inefficient of course. I guess the next step is to add a utility to BLI_length_parameterize.hh to find indices and factors along lengths.

Something like this I suppose:

void create_arbitrary_samples(Span<float> lengths,
                              Span<float> sample_lengths,
                              bool cyclic,
                              MutableSpan<int> indices,
                              MutableSpan<float> factors);

That would also be used in the sample curves node.
I have a few ideas there, something like sorting the sample length arguments we discussed a while ago.

146

could be clarified with "rather than the end point"

222–228

I'd suggest extracting a utility that finds the total length from Span<float3>.

I'd also suggest slicing a span of positions for the curve into a variable and then using that instead of using curve_points over and over.
That makes it more like move_last_point_and_resample in structure.

333

If you disagree that's fine, I'd general expect "for" instead of "by" here though.

333–344

Could add something like /* Execute effect. */ on the second parallel block, to mirror the "Compute influences" comment.

I'm a bit curious about the organization here. For example, why not do something like this?

threading::parallel_for(curves_->curves_range(), 256, [&](IndexRange curves_range) {
  Influences &influences = influences_by_thread.local();
  influences.clear();
  if (falloff_shape_ == PAINT_FALLOFF_SHAPE_TUBE) {
    this->gather_influences_projected(influences);
  }
  else if (falloff_shape_ == PAINT_FALLOFF_SHAPE_SPHERE) {
    this->gather_influences_spherical(influences);
  }
  self_->effect_->execute(*curves_, influences.curve_indices, influences.move_distances_cu);
});

I'd imagine that fusing the two passes would keep more data in cache for each thread, unless the goal is to keep the data from the first influence gathering pass more local?
Or maybe you're thinking about something besides locality?

352–485

I guess this influence gathering logic is quite similar to existing code in other brushes, just improved and generalized a bit?

source/blender/blenlib/BLI_math_geom.h
338–341

I'd suggest a few changes here, even if it makes it less consistent with the other functions in this area.

  • Change r_close_a to r_closest_a
  • Document what "lambda" means in this situation.
  • I think return arguments are supposed to be last, in general? This area is quite inconsistent then.. I'm okay if you don't change that one :P

Also, I wonder about having something like BLI_math_geometry.hh using float2 instead. Maybe?

source/blender/makesrna/intern/rna_brush.c
1949

Suggestion for simplification of the tooltip:
"When shrinking curves, they shouldn't become shorter than this length" -> Avoid shrinking curves shorter than this length

  • Merge branch 'master' into grow-shrink-brush
  • fix float accuracy issue
  • comment cleanup
  • extract utility
  • improve name
  • add comment
  • cleanup
source/blender/blenlib/BLI_math_geom.h
338–341

I think return arguments are supposed to be last, in general?

Yeah not sure why all the functions here have the return parameter first.

source/blender/editors/sculpt_paint/curves_sculpt_grow_shrink.cc
96

Yes right, I'll refactor this code once we have the utility in BLI_length_parameterize.hh.
Would probably make sense to have two versions, one that expects a sorted array and one that doesn't. Of course one could be implemented in terms of the other for now.

333–344

That would probably improve cache usage. Think I'll keep the loops separate for now. Joining loops later on seems relatively easy if we notice that it is better in practice. There are arguments for keeping the loops separate:

  • Makes benchmarking easier because each loop can easily be measured separately.
  • self_->effect_->execute is called at most #threads times, while in your code it may be called much more often with shorter arrays.
  • Might make it easier to change how the influence is computed without changing the effect.
352–485

For the most part yes. I'm still not 100% sure this code is really doing what the user expects tbh. That probably needs another iteration after this patch.
The main thing that I tried that may be questionable is that the brush strength depends on the distance the brush was moved (more movement results in more growing/shrinking). I found that to work relatively well. Also it helps making sure that the brush does something useful at many different scales, because it automatically adapts to the zoom level.
I'd like to give this a try, but maybe something else is needed later.

Thanks for updates and the replies. This looks good to me now.

This revision is now accepted and ready to land.Apr 5 2022, 2:28 PM
This revision was automatically updated to reflect the committed changes.