Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TubeGeometry: Added parametric radius via THREE.Path. #13971

Closed
wants to merge 6 commits into from

Conversation

spite
Copy link
Contributor

@spite spite commented May 1, 2018

For variable radius along the tube geometry (like the old taper, but with more control using a Path). Still works with a number, and it should be serialisable.

@WestLangley
Copy link
Collaborator

Such a change will require a change to the computed normals, too.

@spite
Copy link
Contributor Author

spite commented May 1, 2018

Right... might be easier to call .computeVertexNormals afterwards?

@WestLangley
Copy link
Collaborator

WestLangley commented May 1, 2018

might be easier to call .computeVertexNormals afterwards?

That is a problem do to the seams -- in TubeBufferGeometry at least. The analytic solution would be best.

@Mugen87 I expect TubeBufferGeometry could be more carefully written to share vertices on the seams. There is the side seam -- and, if closed, the end seam.

@Mugen87
Copy link
Collaborator

Mugen87 commented May 1, 2018

Sharing vertices might cause problems with uv-coordinates. I'm not sure this approach will work for TubeBufferGeometry.

@WestLangley
Copy link
Collaborator

Rats. I forgot TubeBufferGeometry supports uvs.

So the solution must be the analytic one.

@spite
Copy link
Contributor Author

spite commented May 1, 2018

This is the current code calling computeVertexNormals after the attributes are set.

screen shot 2018-05-01 at 22 46 42

EDIT: A more normal use, a tapered TubeBufferGeometry

screen shot 2018-05-01 at 22 51 46

And the current unmodified TubeBufferGeometry:

screen shot 2018-05-01 at 22 45 08

@WestLangley
Copy link
Collaborator

Yes, this is a cool feature. :-)

You should see a clearly-visible seam if you reduce radiusSegments to 6 and call computeVertexNormals(). Analytic normals do not have that problem.

You can also use VertexNormalsHelper to see the normals.

@spite
Copy link
Contributor Author

spite commented May 1, 2018

I see

screen shot 2018-05-01 at 23 27 18

I'll see what can be done. It's not too terrible since path can be sampled around the segment point...

src/geometries/TubeGeometry.js Outdated Show resolved Hide resolved
@WestLangley
Copy link
Collaborator

@spite

How about specifying radius as a simple function defined on [ 0, 1 ], instead? That is what I did.

Here is my hack to compute the adjusted normal.

var P = new Vector3();
var P1 = new Vector3();
var P2 = new Vector3();

...

// normal

normal.x = ( cos * N.x + sin * B.x );
normal.y = ( cos * N.y + sin * B.y );
normal.z = ( cos * N.z + sin * B.z );
normal.normalize();

if ( radius instanceof Function ) {

	var delta = 0.0001;
	var t1 = i / tubularSegments - delta;
	var t2 = i / tubularSegments + delta;

	// Capping in case of danger

	if ( t1 < 0 ) t1 = 0;
	if ( t2 > 1 ) t2 = 1;

	P1 = path.getPointAt( t1, P1 );
	P2 = path.getPointAt( t2, P2 );
	var delta = P1.distanceTo( P2 );

	var r1 = radius( t1 );
	var r2 = radius( t2 );

	normal.addScaledVector( T, - ( r2 - r1 ) / delta ).normalize();

}

screen shot 2018-05-02 at 11 41 53 am

@WestLangley
Copy link
Collaborator

@Mugen87 This pattern is unfortunate...

P1 = path.getPointAt( t1, P1 );

If the function accepts a target, it can be

path.getPointAt( t1, P1 );

If not, then it must be

P1 = path.getPointAt( t1 );

So we have to write it to accommodate both...

@spite
Copy link
Contributor Author

spite commented May 2, 2018

@WestLangley my version of this takes a number or a function as radius, but @mrdoob said it couldn't be like that because it couldn't be serialised. So I have another version which takes an array of values, but that's cumbersome. Path has the most flexibility of all

@WestLangley
Copy link
Collaborator

WestLangley commented May 2, 2018

@spite OK. The math I posted to compute the normal can be easily modified to accommodate Path. I expect you can make it more efficient...

@spite
Copy link
Contributor Author

spite commented May 2, 2018

Seems about right now: a hairy pipe thingie

screen shot 2018-05-02 at 22 54 49

@Mugen87
Copy link
Collaborator

Mugen87 commented May 3, 2018

We still have to adjust the documentation of TubeGeometry and TubeBufferGeometry

@Mugen87 Mugen87 changed the title added parametric radius via THREE.Path TubeGeometry: Added parametric radius via THREE.Path. May 3, 2018
@AGuyCoding
Copy link

will this be ever merged in the master?

@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 19, 2020

The PR is not yet complete since it's also necessary to add serialization/deserialization and update the docs/TS file. Besides, I'm not sure we really need this feature. In the meanwhile, I tend to keep the geometry generators as simple as possible. @mrdoob has to decide what he prefers.

@spite
Copy link
Contributor Author

spite commented Jan 19, 2020

@benjaminadrom do what i do if you really need that feature: import it as another type of Geometry (like ParametricTubeGeometry) and use it instead of the default TubeGeometry

@AGuyCoding
Copy link

@benjaminadrom do what i do if you really need that feature: import it as another type of Geometry (like ParametricTubeGeometry) and use it instead of the default TubeGeometry

Thanks a lot for your suggestion,
did you mean "ParametricGeometries"? I can't find a "ParametricTubeGeometry" as you said.
May I ask you for a little guidance for "ParametricGeometries" + taper?

@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 20, 2020

I can't find a "ParametricTubeGeometry" as you said.

The idea was that you create yourself a custom class with the code from this PR.

@Mugen87
Copy link
Collaborator

Mugen87 commented Mar 16, 2021

taper was removed on purpose and I don't think we should it add it back or an advanced version of it. The idea was to keep the generators simple/compact and not enhance them with many features. Otherwise one could argue that variable radii could be added to CylinderGeometry and torus geometries, too. This would get quickly out of hand.

@Mugen87 Mugen87 closed this Mar 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants