Page MenuHome
Differential D11886

Fix T71137: curve minimum twist producing wrong geometry
AbandonedPublic
Actions

Authored by Miguel G. (ghaspias) on Jul 12 2021, 1:31 AM.

Details

Reviewers
Sebastian Parborg (zeddb)
Campbell Barton (campbellbarton)
Hans Goudey (HooglyBoogly)
Maniphest Tasks
T71137: off-by-one bug in spline interpolation produces wrong geometry
Summary

The minimum twist computation for a curve produced a discontinuity in last vertex of closed (cyclic) curves.
This was invisible for planar curves (2D or 3D), but was made evident when some 'extrusion' or 'bevel' was applied and
when there was some twist or tilt, in particular with low curve 'resolution' settings, or when a symmetric result would
be expected.

The fix: the minimum twist computation was starting at the last vertex instead of the first one.
Tested with non-cyclic curves, doesn't seem to affect previous behavior.

The curve below is symmetrical about the x-axis, but previously would produce asymmetric bevel or extrude:

Before:

After:

Diff Detail

Repository
rB Blender

Event Timeline

Miguel G. (ghaspias) requested review of this revision.Jul 12 2021, 1:31 AM
Miguel G. (ghaspias) created this revision.
Miguel G. (ghaspias) edited the summary of this revision. (Show Details)Jul 12 2021, 2:27 AM
Miguel G. (ghaspias) added projects: Core, Modeling.
Miguel G. (ghaspias) updated this revision to Diff 39412.Jul 12 2021, 2:31 AM

Corrected indentation and commenting style.

Sebastian Parborg (zeddb) added a comment.Jul 15 2021, 7:08 PM

Just to give you an update.
I'm basically stuck with a few high prio tasks for the studio so I don't really know when I can get around to reviewing this.

Poke me again if you haven't heard from me again in two weeks.

Hans Goudey (HooglyBoogly) added a subscriber: Hans Goudey (HooglyBoogly).Jul 15 2021, 7:46 PM

Well, we're also moving the curve implementation. The minimum twist calculation in spline_base.cc already has fewer issues than the one here.
I think I'd rather see unifying the two (AKA replacing this implementation) as the way to fix this bug, otherwise we're dividing our efforts unnecessarily.
It's not like this is a new bug anyway.

Sebastian Parborg (zeddb) added a comment.Jul 19 2021, 4:34 PM

@Hans Goudey (HooglyBoogly) I guess @Miguel G. (ghaspias) can try the curves in geometry nodes to see if it has any similar symmetry issues?
(And then work from there to unify the code)

Miguel G. (ghaspias) added a comment.Jul 20 2021, 11:31 PM
In D11886#307613, @Hans Goudey (HooglyBoogly) wrote:

Well, we're also moving the curve implementation. The minimum twist calculation in spline_base.cc already has fewer issues than the one here.
I think I'd rather see unifying the two (AKA replacing this implementation) as the way to fix this bug, otherwise we're dividing our efforts unnecessarily.
It's not like this is a new bug anyway.

Yes, I know curves are being revised, also as part of the geometry nodes. There are multiple issues with the current implementation (by which I mean things that are broken, in that they produce wrong/undesirable results). Rewriting from scratch is beyond my capacity, but I can help with a robust algorithm and documentation. Besides that, I can help with testing and track small bugs like this one.
I don't know about the general opinion, but for me this kind of bugs that generate wrong geometry are really a problem, since they mean I have to double check what Blender does.

Miguel G. (ghaspias) added a comment.EditedJul 20 2021, 11:33 PM
In D11886#308362, @Sebastian Parborg (zeddb) wrote:

@Hans Goudey (HooglyBoogly) I guess @Miguel G. (ghaspias) can try the curves in geometry nodes to see if it has any similar symmetry issues?
(And then work from there to unify the code)

I'd like to do that, but it seems most work on geometry nodes isn't on master yet; which branch should I use? Is there a binary available?

Hans Goudey (HooglyBoogly) added a comment.Jul 20 2021, 11:38 PM
In D11886#308842, @Miguel G. (ghaspias) wrote:

I'd like to do that, but it seems most work on geometry nodes isn't on master yet; which branch should I use? Is there a binary available?

Actually most of the curves work for geometry nodes is in master. Until D11597 is in you have to use an object info node to import the curve though.

Miguel G. (ghaspias) added a comment.EditedJul 21 2021, 4:38 AM
In D11886#308848, @Hans Goudey (HooglyBoogly) wrote:
In D11886#308842, @Miguel G. (ghaspias) wrote:

I'd like to do that, but it seems most work on geometry nodes isn't on master yet; which branch should I use? Is there a binary available?

Actually most of the curves work for geometry nodes is in master. Until D11597 is in you have to use an object info node to import the curve though.

Thanks! It is not perfect, but it would have saved me hours while trying to debug with printf()!!

Is the info presented in the viewer using the same code for computation of the twist (in curves.c)? How can I access the handles of each control point?

Hans Goudey (HooglyBoogly) added a comment.Jul 21 2021, 4:59 AM

No, all of the new tangent and normal calculation is in spline_base.cc.
I recommend reading the comments in BKE_spline.hh if you need more background.

Bastien Montagne (mont29) edited projects, added BF Blender; removed Core.Jul 23 2021, 4:50 PM
Miguel G. (ghaspias) added a comment.Aug 1 2021, 3:46 PM

Hi, @Sebastian Parborg (zeddb) , I wonder if this could be included in 2.93.x series (since it seems for 3.0 there will be new code for the curves). Did you find some time to look into this? Is there something I can do to help?

Sebastian Parborg (zeddb) added a comment.EditedAug 2 2021, 11:50 AM
In D11886#312488, @Miguel G. (ghaspias) wrote:

Hi, @Sebastian Parborg (zeddb) , I wonder if this could be included in 2.93.x series (since it seems for 3.0 there will be new code for the curves). Did you find some time to look into this? Is there something I can do to help?

I don't think this should be back ported into 2.93.x for two reasons:

  1. It is not accepted into the master branch. Usually we only backport fixes that is also in the current development version unless it is a very critical fix.
  1. This will break older blend files as it changes the behavior of the curve. So if someone uses a cyclic curve in older files and loads them up, the result will be different than in the older versions.

So introducing this in a LTS point release that is only meant for critical fixes is not good I feel.

Unless I'm misinterpreting @Hans Goudey (HooglyBoogly) , he wants to work with you on the new curve code. Then that curve code would be used to replace this code for the 3.0 release.
I understand if you don't have time to do this, but I think it would be the best course of action as then you and Hans can ensure that the new curve code is the best it can be and superior to the older code.

Now this doesn't have to mean that you have to spend a lot of time porting, it could simply mean that you spend time trying out the new code and providing smaller fixes and tweaks.
You could even give feedback on the general code APIs and help shape it so it doesn't risk becoming as confusing as the current curve code.

Campbell Barton (campbellbarton) retitled this revision from Fix curve minimum twist producing wrong geometry to Fix T71137: curve minimum twist producing wrong geometry.Aug 3 2021, 2:16 AM
Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)
Campbell Barton (campbellbarton) added a subscriber: Campbell Barton (campbellbarton).
Campbell Barton (campbellbarton) accepted this revision.EditedAug 3 2021, 2:32 AM

This works well, accepting, although there are some issues with this patch unrelated to the logic this changes.

The patch needed to be manually edited before it could be applied:

--- a/"a/source/blender/blenkernel/intern/curve.c" needed to be replaced with:
--- a/source/blender/blenkernel/intern/curve.c.

This revision is now accepted and ready to land.Aug 3 2021, 2:32 AM
Hans Goudey (HooglyBoogly) added a reviewer: Hans Goudey (HooglyBoogly).Aug 3 2021, 3:26 AM

+1 to everything @Sebastian Parborg (zeddb) said.

Also I don't believe this should be in the accepted state currently with those topics and the changes Campbell mentioned. That trivial formatting and comment stuff is important to get right for first patches and commits.

This revision now requires review to proceed.Aug 3 2021, 3:26 AM
Campbell Barton (campbellbarton) added a comment.EditedAug 9 2021, 11:28 AM

@Miguel G. (ghaspias) any updates on this?

Only minor changes are needed for this patch to be committed.

Campbell Barton (campbellbarton) abandoned this revision.Aug 19 2021, 9:12 AM

Over a week without response, committed rBcf721942149057684c089c0677c224dbe9d90c52, including the fix from this patch with minor edits.

Campbell Barton (campbellbarton) mentioned this in rBcf7219421490: Fix T71137: curve minimum twist producing wrong geometry.Aug 19 2021, 9:13 AM
Campbell Barton (campbellbarton) mentioned this in rB36a82314a0f5: Fix T71137: curve minimum twist producing wrong geometry.Tue, Jan 24, 6:44 AM
Campbell Barton (campbellbarton) mentioned this in rB564673b8ea28: Fix T71137: curve minimum twist producing wrong geometry.Thu, Jan 26, 7:49 AM

Revision Contents
Changeset List

PathSize
source/
blender/
blenkernel/
intern/
"a/
source/
blender/
blenkernel/
intern/
curve.c
curve.c"
13 lines

Diff 39411

View Options

source/blender/blenkernel/intern/curve.c

Loading...