mrdoob / three.js

JavaScript 3D Library.
https://threejs.org/
MIT License
101.74k stars 35.3k forks source link

Creating a long TubeGeometry along a long SplineCurve3 - Unexpected "kink" #5503

Closed JohnAnthony closed 9 years ago

JohnAnthony commented 9 years ago

I can't tell if this is a bug in SplineCurve3, a bug in TubeGeometry or something I'm doing wrong. I have a long list (242) of 3d points that I'm feeding to a SplineCurve3 and creating a TubeGeometry from. There appears to be nothing unusual about the data at the point the "kink" is made, and the only difference between this and previous datasets is the number of points.

I'll endeavour to create in an example dataset and reproduce the issue on jsfiddle, but the data I'm using is sensitive and there's no way at all I can share it. Which is a bit of a bit of a ball-ache.

Switching to the latest build of dev branch, the issue persists.

kink

JohnAnthony commented 9 years ago

Another look at it that seems to imply the points are being rotated or flipped around the center point.

kink2

I created a very long, straight diagonal tube with a macro but that didn't reproduce it. I suspect the wibbledy-wobbledy real data is the key to reproducing.

I've been cutting away from the top and bottom of the curve trying to narrow down the data points that cause the "flip". If I can narrow it down to maybe as fer as 5 or 6 Vector3s together I might be able to post them.

JohnAnthony commented 9 years ago

Yeah, removing datapoints from the top or bottom of the Vector3 array changes where the "kink" appears, and removing enough of them makes it disappear entirely.

A spline bug?

WestLangley commented 9 years ago

It is likely your spline data causing problems in the computation of the Frenet Frames used in the construction of TubeGeometry.

If you can demonstrate a three.js bug, we need a working example -- preferably a simple one. Please see "How to report a bug" in the guidelines.

JohnAnthony commented 9 years ago

Yeah, it'll take me an absolute age to give a working example because all the data I'm working with belongs to a customer and I'll have to hand-write alternative data to reproduce which

a) Is a lot of data to write by hand (and therefore error-prone) and b) May not even reproduce the bug

I'll most likely give a jsfiddle link on Monday, but I'm interested in writing (and upstreaming) a patch for this myself so conjecture about the cause is welcome.

JohnAnthony commented 9 years ago

Here's a painstakingly-created example dataset in jsfiddle.

http://jsfiddle.net/qhj95gs0/1/

Are you sure this should be tagged an Enhancement? Seems much more like a bug to me.

Now that I'm looking for it, I've noticed this bug affects a solid 50% of our dozen-or-so datasets. So I guess it's actually reasonably common with splines beyond trivial length.

WestLangley commented 9 years ago

screen shot 2014-10-29 at 1 13 58 pm

I managed to replicate the problem with only your first 4 points. The problem is the fitted spline curve has a kink in it. As a result, when the TubeGeometry is created, if the constructor happens to sample a point inside the kink, then the computed Frenet frame tangent is way off. This results in the distorted tube geometry.

fiddle: http://jsfiddle.net/qhj95gs0/2/

Perhaps @zz85 will have a look.

/ping @zz85

zz85 commented 9 years ago

i'm not too sure about this... @WestLangley, would you be suggesting that THREE.SplineCurve3 has a bug?

WestLangley commented 9 years ago

would you be suggesting that THREE.SplineCurve3 has a bug?

Yes. But maybe it is a feature...

WestLangley commented 9 years ago

@zz85 Do you think that Catmull-Rom is the proper interpolation algorithm for an array of 3D points? Maybe what we see here is not a bug, but an unfortunate feature.

@JohnAnthony Is piecewise-linear an option?

var path = new THREE.PiecewiseLinearCurve3( points );

where

THREE.PiecewiseLinearCurve3 = THREE.Curve.create(

    function ( points ) {

        this.points = ( points == undefined ) ? [] : points;

    },

    function ( t ) {

        var points = this.points;

        var d = ( points.length - 1 ) * t;

        var index1 = Math.floor( d );
        var index2 = ( index1 < points.length - 1 ) ? index1 + 1 : index1;

        var pt1 = points[ index1 ];
        var pt2 = points[ index2 ];

        var weight = d - index1;

        return new THREE.Vector3().copy( pt1 ).lerp( pt2, weight );

    }

);
JohnAnthony commented 9 years ago

Hey, @WestLangley

I gave that interpolation algorithm a once-around-the-block and it seems to work nicely without any kinks. "Thankyou" would be a bit of an understatement - this software's going live this afternoon!

Any gotchas I should be aware of?

I won't close as I'm not sure if the original behaviour actually is a bug.

WestLangley commented 9 years ago

Any gotchas I should be aware of?

The interpolated line is a series of straight segments instead of a smooth curve. Ony you can decide if that is a problem for you.

zz85 commented 9 years ago

@WestLangley maybe the formulas should be rechecked, but as far as i remember catmull-rom are used for camera keyframing, which should be suitable.

or well, maybe in certain cases cubin hermite spline might give more control, but i haven't got to try it yet... http://paulbourke.net/miscellaneous/interpolation/

zz85 commented 9 years ago

hmmm... i think the issue is that we are using a uniform parameterization of the catmull-rom. perhaps a centripetal or chordal version of it would solve this issue. related:

http://stackoverflow.com/questions/9489736/catmull-rom-curve-with-no-cusps-and-no-self-intersections/19283471#19283471 http://www.cemyuksel.com/research/catmullrom_param/

WestLangley commented 9 years ago

Thank you for taking the time to look at this. I wonder if THREE.PiecewiseLinearCurve3 is an acceptable fallback position?

zz85 commented 9 years ago

I wouldn't consider THREE.PiecewiseLinearCurve3 a spline... perhaps when I get some bandwidth, I'll be able to implement a centripetal version of Catmull-rom Curve that would improve this behaviour of the spline..

WestLangley commented 9 years ago

I wouldn't consider THREE.PiecewiseLinearCurve3 a spline

A spline is a piecewise polynomial. THREE.PiecewiseLinearCurve3 just has a discontinuous derivative at the control points.

zz85 commented 9 years ago

true that, i guess i was trying to say that it isn't a smooth spline to me...

zz85 commented 9 years ago

@WestLangley I've got some basic Centripetal Catmull-Rom Spline working here :) http://jsfiddle.net/3joggy10/2/

zz85 commented 9 years ago

updated version: http://jsfiddle.net/3joggy10/4/

http://www.cemyuksel.com/research/catmullrom_param/catmullrom.pdf says this is the preferred form of catmull-rom, maybe we could make this the "default" SplineCurve.

WestLangley commented 9 years ago

@zz85 Nice!

Yes. I agree with your proposal.

If you think a THREE.PiecewiseLinearCurve3 is useful, you can add it, too. Maybe it could be called Polyline3.

WestLangley commented 9 years ago

@zz85 Would you be willing to implement your idea?

zz85 commented 9 years ago

thanks for the reminder @WestLangley. i wanted to try this with more data set to compare differences, but it ended up on my back burner. let me try making some time for this in this week.

zz85 commented 9 years ago

wrote a small example to show how the different curves (original SplineCurve, Centripetal, and Chordal) stack together here http://zz85.github.io/DragSpline/catmull_path_test.html

WestLangley commented 9 years ago

@zz85 Really nice demo! Would you consider adding a third dimension + OrbitControls?

zz85 commented 9 years ago

@WestLangley that actually reminds me of a spline editor I worked on before. I'll try to pull that out.

meanwhile, I wonder if @mrdoob prefers a new class THREE.CatmullRomCurve3 or prefers replacing the current THREE.SplineCurve3 with https://github.com/zz85/DragSpline/blob/master/CatmullRomCurve3.js ?

mrdoob commented 9 years ago

CatmullRomCurve3 is more explicit :+1:

zz85 commented 9 years ago

here we go @WestLangley http://zz85.github.io/ThreeLabs/spline_editor3.html

image

WestLangley commented 9 years ago

@zz85 Thanks !

WestLangley commented 9 years ago

@zz85 What's your decision on this?

zz85 commented 9 years ago

pending PR #5997...

zz85 commented 9 years ago

PR 5997 has been merged, so this can be closed ;)