Paths

Table of Contentst

Fix (unreported): incorrect weights and radii interpolation when subdividing curves
Needs ReviewPublic
Actions

Authored by Matteo Falduto (matteolegna) on May 23 2021, 4:20 AM.

Details

Reviewers

Summary

I've encountered a few issues with how the weight and radius parameters are calculated for new vertices created by subdividing curves.

In the case of splines, all the newly generated vertices assume the very same values as radii and weights, instead of smoothly interpolating them along the curve to preserve its original appearance.

In the case of polylines (and NURBS), the values for the radii are interpolated, but in the wrong order. Weights are not interpolated at all.

This patch fixes all the above-mentioned issues.

Diff Detail

Repository

rB Blender

Branch

FIX_curve_subdivide_radius (branched from master)

Build Status

Buildable 14718
Build 14718: arc lint + arc unit

Event Timeline

Matteo Falduto (matteolegna) requested review of this revision.May 23 2021, 4:20 AM

Matteo Falduto (matteolegna) created this revision.

Harbormaster completed remote builds in B14718: Diff 37403.May 23 2021, 4:20 AM

Matteo Falduto (matteolegna) edited the summary of this revision. (Show Details)May 23 2021, 4:22 AM

Matteo Falduto (matteolegna) added a reviewer: Modeling.

Matteo Falduto (matteolegna) added a project: Modeling.

Matteo Falduto (matteolegna) edited the summary of this revision. (Show Details)

Matteo Falduto (matteolegna) added a subscriber: Matteo Falduto (matteolegna).

RedMser (RedMser) awarded a token.May 23 2021, 11:30 AM

Matteo Falduto (matteolegna) edited reviewers, added: Campbell Barton (campbellbarton); removed: Modeling.May 25 2021, 3:40 AM

Campbell Barton (campbellbarton) requested changes to this revision.May 25 2021, 7:28 AM

Campbell Barton (campbellbarton) added inline comments.

source/blender/editors/curve/editcurve.c
3490–3491	Multiplying a very small value by an integer can cause floating-point imprecision. Even though it's unlikely to cause any problems in this particular case, as a rule of thumb I would rather avoid doing this entirely and use `interpf` here as is done elsewhere.

This revision now requires changes to proceed.May 25 2021, 7:28 AM

Campbell Barton (campbellbarton) awarded a token.May 25 2021, 7:29 AM

Leon Schittek (lone_noel) awarded a token.May 25 2021, 7:53 AM

Vitalii Vlasov (Fracture128) awarded a token.May 25 2021, 8:44 AM

using interpf to interpolate weights and radii in the case of splines.

Harbormaster completed remote builds in B14758: Diff 37476.May 25 2021, 6:29 PM

@Campbell Barton (campbellbarton) I've uploaded a diff using interpf instead of the manual interpolation formula used previously. As I noted in replying to your code comment, this required introducing a new, different factor parameter I've called factor_linear. You can choose the version you find leaner and more fitting the code style.
Thanks!

source/blender/editors/curve/editcurve.c
3490–3491	Thank you for the feedback. One of the reasons I didn't use `interpf` here is that, in order to produce the correct result, in this case, it would require introducing a new, different factor parameter than the one used elsewhere in the block. It would look something like this: float factor_linear; for (int i = 0; i < number_cuts; i++) { factor = 1.0f / (number_cuts + 1 - i); factor_linear = (float)(i+1) / (number_cuts + 1); [...] beztn->radius = interpf(nextbezt->radius, bezt->radius, factor_linear); beztn->weight = interpf(nextbezt->weight, bezt->weight, factor_linear); [...] } I thought that would have looked not very neat and somehow less efficient. But I can surely do it if you find it more fitting.

Cleanup: removing unnecessary factor_linear

Harbormaster completed remote builds in B14762: Diff 37481.May 25 2021, 7:56 PM

...Okay, actually never mind 😐
Ignore my previous comment: I didn't put enough effort into understanding the surrounding code and I didn't see that it was unnecessary to introduce that factor_linear.
In the last diff I got rid of it, harmonizing the logic of that two lines with the rest of the algorithm.

[EDIT] I only don't like too much the fact that, unlike my earlier code, this uses, at each iteration, the result of the previous interpolation as an input to interpolate the remaining vertices down the line, accumulating any possible precision errors...
Could you please compare the various Diffs and tell me which approach you think is the most suitable?
The difference is that my first two attempts used to interpolate all of the newly created vertices always using the preexisting vertices as interpolation interval, while Diff 3, in order to work with the available factor parameter, uses as interval the ever-shrinking span between the lastly created vertex and the final preexisting vertex.

In D11367#291250, @Matteo Falduto (matteolegna) wrote:

@Campbell Barton (campbellbarton) I've uploaded a diff using interpf instead of the manual interpolation formula used previously. As I noted in replying to your code comment, this required introducing a new, different factor parameter I've called factor_linear. You can choose the version you find leaner and more fitting the code style.
Thanks!

Evan Wilson (EAW) awarded a token.May 26 2021, 10:31 PM

I don't see what's necessary to use the previous interpolated result. Shouldn't this be an interpretation between the 2 end points? (the point that existed before the subdivided points or added)

Revision Contents
Changeset List

Path

Size

source/

blender/

editors/

curve/

editcurve.c

9 lines

Diff 37403

View Options

source/blender/editors/curve/editcurve.c

Fix (unreported): incorrect weights and radii interpolation when subdividing curvesNeeds ReviewPublicActions