Three.js: More subdivision options for icosahedron geometry

[felixmariotto]

About the icosahedron geometry, I understand that the detail parameter is an option for subdividing each edge into more vertices, which are projected onto a sphere. However it seems that some possible subdivision are not allowed.

For example, this is the subdivision with detail = 1 :

And this is with detail = 2 :

It looks like the following in-between subdivision would be possible but is not currently allowed :

Is there an issue with this kind of subdivision ? If not, would you be interested in a PR ?

[Mugen87]

The current algorithm uses this code to perform the subdivision of a face:

const cols = Math.pow( 2, detail );

Meaning cols is POT. I don' think you can implement this feature request backwards-compatible so existing parameterizations would still produce the same result.

[marcofugaro]

I use this code to generate a sphere with more fine-tuned picking of the level of detail, maybe this can be useful to you

const segments = 2000
const geometry = new THREE.BufferGeometry()
const positions = new Float32Array(segments * 3)

// Uniform distribution of points on the surface of a sphere
// https://gist.github.com/AngeloYazar/6091095
let inc = Math.PI * (3 - Math.sqrt(5))
let off = 2 / segments
let radius = 1.5

for (let i = 0; i < segments; i++) {
  let y = i * off - 1 + off / 2
  let r = Math.sqrt(1 - y * y)
  let phi = i * inc

  positions[3 * i] = radius * Math.cos(phi) * r
  positions[3 * i + 1] = radius * y
  positions[3 * i + 2] = radius * Math.sin(phi) * r
}

geometry.setAttribute('position', new THREE.BufferAttribute(positions, 3))

[mrdoob]

Hmm, if we changed this, the worst thing that can happen is that user's code will have less subdivisions. Correct?

[Mugen87]

I'm not sure if the code in PolyhedronGeometry needs a POT cols value. If this is changed, the resulting vertex data have to be closely verified.

[mrdoob]

@felixmariotto would you like to have a look at it?

[felixmariotto]

Oops sorry for the delay.

@mrdoob yes I will do some testing

[felixmariotto]

Here is a live example showing the difference before and after the update : https://jsfiddle.net/felixmariotto/1aqorz7f/215/

here I set cols = detail.
here I had to default detail to 1 and ensure that it's not 0 ( it was previously implied by Math.pow ).

An alternative could be to set cols = detail + 1, and maybe rename detail into division.

At first glance it looks fine, but I can write some specific tests if you tell me what to check exactly 😊

ps: I would create a PR, but I had to push the build files to make my live demo and I understand you guys don't want them in pull requests...

[mrdoob]

I like it! When using the editor, you had to be careful with the detail parameter in the old code because you could easily freeze your system.

[mrdoob]

I think a PR would be great 🙏