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

Lathe with Vector2 #7989

Merged
merged 1 commit into from
Jan 19, 2016
Merged

Lathe with Vector2 #7989

merged 1 commit into from
Jan 19, 2016

Conversation

rfm1201
Copy link
Contributor

@rfm1201 rfm1201 commented Jan 18, 2016

LatheGeometry modified to use Vector2 and allowing axe choice by the caller.
The axe is designed by a letter because I didn't know where I should put a constance definition.
The loop that create the geometry is not optimized for speed => the axis test is in the inner loop.

Should not break any existing code. Tested with all the examples that create a LatheGeometry.

Doc and examples not updated.

it's the next step of #7971 (comment)

LatheGeometry modified to use Vector2 and allowing axe choice by the
called.
The axe is designed by a letter because I didn't know where I should put
a constance definition.
The loop that create the geometry is not optimized for speed => the axis
test is in the inner loop.
Should not break any existing code.
Doc and exemple not updated yet.
@mrdoob
Copy link
Owner

mrdoob commented Jan 18, 2016

Oh, I was working on that. I think I would not have the axe parameter for now.

LatheGeometry change should just be this:

diff --git a/src/extras/geometries/LatheGeometry.js b/src/extras/geometries/LatheGeometry.js
index f702c4a..6f271bc 100644
--- a/src/extras/geometries/LatheGeometry.js
+++ b/src/extras/geometries/LatheGeometry.js
@@ -35,18 +35,18 @@ THREE.LatheGeometry = function ( points, segments, phiStart, phiLength ) {

        var phi = phiStart + i * inverseSegments * phiLength;

-       var c = Math.cos( phi ),
-           s = Math.sin( phi );
+       var sin = Math.sin( phi );
+       var cos = Math.cos( phi );

        for ( var j = 0, jl = points.length; j < jl; j ++ ) {

-           var pt = points[ j ];
+           var point = points[ j ];

            var vertex = new THREE.Vector3();

-           vertex.x = c * pt.x - s * pt.y;
-           vertex.y = s * pt.x + c * pt.y;
-           vertex.z = pt.z;
+           vertex.x = point.x * cos;
+           vertex.y = point.y;
+           vertex.z = point.x * sin;

            this.vertices.push( vertex );

@rfm1201
Copy link
Contributor Author

rfm1201 commented Jan 18, 2016

I agree, but if someone is using LatheGeometry, his scene gonna change.
The axe parameter is not really usefull for new user, but it allow the building of exactly the same scene.

@mrdoob
Copy link
Owner

mrdoob commented Jan 18, 2016

I know, I know. But we can suggest to use geometry.rotateX( - Math.PI / 2 ).

@mrdoob
Copy link
Owner

mrdoob commented Jan 19, 2016

I'll merge and simplify.

mrdoob added a commit that referenced this pull request Jan 19, 2016
@mrdoob mrdoob merged commit 5bdc848 into mrdoob:dev Jan 19, 2016
@mrdoob
Copy link
Owner

mrdoob commented Jan 19, 2016

Thanks!

@rfm1201 rfm1201 deleted the Lathe-with-Vector2 branch January 19, 2016 10:17
@rfm1201
Copy link
Contributor Author

rfm1201 commented Jan 19, 2016

Do you want me to update the documentation and examples that are know broken ?
I found

  • docs/api/extras/geometries/LatheGeometry.html
  • editor\js\libs\tern-threejs\threejs.js
  • examples\misc_uv_tests.html
  • examples\webgl_geometries.html
  • examples\webgl_geometry_normals.html
  • examples\webgl_modifier_subdivision.html

@mrdoob
Copy link
Owner

mrdoob commented Jan 19, 2016

@rfm1201 that would be great!

@rfm1201 rfm1201 mentioned this pull request Jan 19, 2016
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.

2 participants