Page MenuHome
Differential D8557

Add 'Capsule' target object shape to the Cast modifier
Needs ReviewPublic
Actions

Authored by Christian Friedrich (rbx775) on Aug 13 2020, 12:00 AM.

Details

Reviewers
Campbell Barton (campbellbarton)
Bastien Montagne (mont29)
Howard Trickey (howardt)
Summary

Useful for non-destructive modeling workflows in combination with the remesh modifier.

It pushes the capsules hemispheres out of the original bounding box to immediately distinguish itself from the 'Sphere' type for uniformly sized target objects or uniformly sized control objects.

I simply added the 'modifier' module owners as reviewers, hope thats ok.

Diff Detail

Event Timeline

Christian Friedrich (rbx775) requested review of this revision.Aug 13 2020, 12:00 AM
Christian Friedrich (rbx775) created this revision.
Bastien Montagne (mont29) requested changes to this revision.Oct 6 2020, 12:35 PM
Bastien Montagne (mont29) added inline comments.
source/blender/makesdna/DNA_modifier_types.h
675–676

never change those values, unless absolutely mandatory (those are written to file, so you'd need some versioning code, and would break forward compatibility).

Just add new enums with available unused value, order is irrelevant here.

source/blender/modifiers/intern/MOD_cast.c
127

This is un-initializing len (and vmin is not initialized either), which is most likely bug.

127

In fact, you need only one float here (call it bbox_z e.g., result of vmax-vmin).

vmin and vmax can then be defined inside their local usage area (first if (type == MOD_CAST_TYPE_CAPSULE) {).

194–201

vmin and vmax need to be initialized properly for this to work, respectively to FLT_MAX and -FLT_MAX

252–253

Styling: Space between comment opening/closing and text, and extra opening * at every new line.

254

Why FLT_EPSILON? Why not simply 0.0f?

This revision now requires changes to proceed.Oct 6 2020, 12:35 PM
Bastien Montagne (mont29) added inline comments.Oct 6 2020, 12:36 PM
source/blender/makesrna/intern/rna_modifier.c
3472

Can add a tooltip stating that this is applied along the Z axis

Christian Friedrich (rbx775) updated this revision to Diff 29763.Oct 9 2020, 1:15 AM

Thank you for your time Bastien, I learned alot! :)

FLT_EPSILON helps with precision errors for vertices that are on (local) 0.0 z.

Bastien Montagne (mont29) added a comment.Oct 9 2020, 10:26 AM
In D8557#226029, @Christian Friedrich (rbx775) wrote:

Thank you for your time Bastien, I learned alot! :)

FLT_EPSILON helps with precision errors for vertices that are on (local) 0.0 z.

I think this is only shifting the issue to vertices that lay on the FLT_EPSILON Z plane then…

What you could do is not move at all vertices that are whithin the [-FLT_EPSILON, FLT_EPSILON] z-value range maybe?

Bastien Montagne (mont29) added a comment.Oct 9 2020, 10:30 AM

On second though, I don't think that applying capsule only along Z axis is OK. I'd expect users to want to have the axis choice here.

Henrik Dick (weasel) added a subscriber: Henrik Dick (weasel).Oct 9 2020, 2:52 PM

@Bastien Montagne (mont29) The cylinder shape is doing the same. It is also only along the Z axis. You can change that with a control object, so there is no limitation here.

Bastien Montagne (mont29) added a project: BF Blender (2.92).Oct 9 2020, 3:22 PM

Ah right… Then LGTM, will wait a bit in case @Campbell Barton (campbellbarton) would like to check on it, also because this should wait for 2.92 now.

Bastien Montagne (mont29) accepted this revision.Oct 9 2020, 3:22 PM
This revision is now accepted and ready to land.Oct 9 2020, 3:22 PM
Christian Friedrich (rbx775) updated this revision to Diff 29796.EditedOct 9 2020, 5:22 PM
Christian Friedrich (rbx775) marked 6 inline comments as done.

@Bastien Montagne (mont29) Good point about only shifting the problem concerning the z centerline verts.
I tried fixing the issue in this revision:

Bastien Montagne (mont29) requested changes to this revision.Oct 9 2020, 6:09 PM
Bastien Montagne (mont29) added inline comments.
source/blender/modifiers/intern/MOD_cast.c
203

multiply by 0.5f here once, instead of for every vertex later in code.

256–260

In fact, I would rather keep a single if/else if pattern here, cleaner imho.

if (vec[2] < -FLT_EPSILON) {
  tmp_co[2] = fac * (vec[2] - bbox_z * 0.5f) * len + facm * tmp_co[2];
}
else if (vec[2] > FLT_EPSILON) {
  tmp_co[2] = fac * (vec[2] + bbox_z * 0.5f) * len + facm * tmp_co[2];
}
257

There is no need for FLT_EPSILON here anymore, you can just use < 0.0f I think?

258–259

No need to multiply by 0.5 here for every vertex

This revision now requires changes to proceed.Oct 9 2020, 6:09 PM
Christian Friedrich (rbx775) updated this revision to Diff 29801.Oct 9 2020, 6:52 PM
Christian Friedrich (rbx775) marked an inline comment as not done.

Thanks Bastien!

I will try to keep this in mind for future patches! :)
applied requested changes.

Bastien Montagne (mont29) accepted this revision.Oct 9 2020, 7:38 PM
This revision is now accepted and ready to land.Oct 9 2020, 7:38 PM
Henrik Dick (weasel) added a comment.Oct 10 2020, 10:43 AM

I did test this patch and found an inconsitency between capsule and cylinder, when the size parameter is used.
I feel that the left one with Size=1 is correct, but the right one should have a capsule that contains the cylinder.
At this point I am not entirely sure if I consider the problem to be in cylinder or in capsule, but since capsule is new,
it might as well match the existing behavior.

Christian Friedrich (rbx775) updated this revision to Diff 29811.EditedOct 10 2020, 9:38 PM
Christian Friedrich (rbx775) marked an inline comment as done.

Wops! Good catch @Henrik Dick (weasel) !

Fixed a bug where Capsule wasnt consistent with Cylinder regarding its 'size' parameter.

Christian Friedrich (rbx775) updated this revision to Diff 29812.Oct 10 2020, 10:04 PM
Christian Friedrich (rbx775) marked 3 inline comments as done.Nov 2 2020, 3:22 PM

Is this good to go @Bastien Montagne (mont29) ?

Dalai Felinto (dfelinto) edited reviewers, added: Howard fletcher (howard); removed: Howard Trickey (howardt). Dalai Felinto (dfelinto) added 1 blocking reviewer(s): Campbell Barton (campbellbarton).Mar 26 2021, 5:52 PM
Dalai Felinto (dfelinto) added a subscriber: Dalai Felinto (dfelinto).

This patch has an approved status however it was not committed yet. So I'm assuming the other reviewers are blocking. Updating the reviewers list to reflect this. This way the patch status still show as Need Review.

This revision now requires review to proceed.Mar 26 2021, 5:52 PM
Dalai Felinto (dfelinto) edited reviewers, added: Howard Trickey (howardt); removed: Howard fletcher (howard).Mar 26 2021, 6:10 PM
Christian Friedrich (rbx775) planned changes to this revision.Mar 27 2021, 1:11 PM

Still needs backwards compatibilty.
Will fix this soon.

Christian Friedrich (rbx775) requested review of this revision.Mar 31 2021, 12:21 AM

Revision Contents
Changeset List

PathSize
source/
blender/
makesdna/
1 line
makesrna/
intern/
1 line
modifiers/
intern/
34 lines

Diff 29763

View Options

source/blender/makesdna/DNA_modifier_types.h

Loading...
View Options

source/blender/makesrna/intern/rna_modifier.c

Loading...
View Options

source/blender/modifiers/intern/MOD_cast.c

Loading...