Skip to content

Conversation

abelnation
Copy link
Contributor

Addresses #8720, at least in a trivial way.

Creates a new primitive, ConeGeometry and ConeBufferGeometry, that are just wrappers around the corresponding Cyilinder geometries.

Note that, the "point" in a cylinder that has one radius as 0 is rendered just like any other side face of the cylinder, i.e. with two triangles. In the r=0 case, one of those triangles is reduced to zero-area, so this solution unnecessarily renders 1 extra invisible triangle per radial segment. I spent some time trying to optimize the CylinderBufferGeometry logic to detect and remove those extra vertices and indices, but found that it dirtied the otherwise nice logic of the cynlinder generation code.

@mrdoob @WestLangley

@WestLangley
Copy link
Collaborator

This PR appears to be well-done and complete.

As a separate issue, not to detract from this PR, I think the Cylinder/Cone(Buffer)Geometry UVs for the end-caps are not right. I think the UV mapping should look like that of Circle(Buffer)Geometry.

...Just in case you are interested in pursuing that. ;)

@abelnation
Copy link
Contributor Author

@WestLangley do you have a reference image / example that I might work from?

@abelnation
Copy link
Contributor Author

intuitively, it seems that the torso mapping should map the entire texture image to the entire face of the torso. i'm less clear on what the mapping should look like for a cap...

@WestLangley
Copy link
Collaborator

@WestLangley do you have a reference image / example that I might work from?

Just have a look at CircleBufferGeometry and see if you agree. There is no right answer here.

/ping @Mugen87 for another opinion.

@WestLangley
Copy link
Collaborator

Oh. I meant to mention not to include the build files in the PR.

See How to Contribute to three.js.

@Mugen87
Copy link
Collaborator

Mugen87 commented May 6, 2016

As a separate issue, not to detract from this PR, I think the Cylinder/Cone(Buffer)Geometry UVs for the end-caps are not right. I think the UV mapping should look like that of Circle(Buffer)Geometry.

Here is the corresponding issue for this requirement #8270. I agree that an alignment to Circle(Buffer)Geometry is definitely an improvement.

@Mugen87
Copy link
Collaborator

Mugen87 commented May 6, 2016

BTW, PR looks good 😉

@abelnation
Copy link
Contributor Author

@mrdoob @Mugen87 what is the process for merging PR's? does the author merge when it get's the thumbs up? or does some primary other contributor take care of merging the pr's?

@WestLangley
Copy link
Collaborator

@abelnation The merging is done by @mrdoob if he approves. Some others have authority, but usually it is he.

@abelnation
Copy link
Contributor Author

Ok. I addressed uv mapping fix separately in #8830.

@mrdoob
Copy link
Owner

mrdoob commented May 8, 2016

Nice PR indeed!

@mrdoob mrdoob merged commit c4476ed into mrdoob:dev May 8, 2016
@mrdoob
Copy link
Owner

mrdoob commented May 8, 2016

Many thanks!

carlosanunes pushed a commit to carlosanunes/three.js that referenced this pull request May 18, 2016
* add files for buffered and non-buffered ConeGeometry based on CylinderGeometry

* build files with cone

* removing unneeded file

* remove unneeded npm script

* remove build files from pr
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.

4 participants