xkpv
July 4, 2023, 10:25am
1
Some functions such as BufferGeometryUtils.toCreasedNormals() call BufferGeometry.toNonIndexed() to convert possibly indexed geometries to non-indexed ones.
It seems that BufferGeometry.toNonIndexed() should not be called when the geometry is not indexed, producing this warning:
THREE.BufferGeometry.toNonIndexed(): BufferGeometry is already non-indexed.
toNonIndexed @ three.module.js:10964
toCreasedNormals @ BufferGeometryUtils.js:1240
In this case and contrary to documentation, no new geometry is returned but this geometry, which may be the reason for the warning, and the functions that trigger such a warning would be working on the original geometry, not on a non-indexed copy.
What would a correct use of BufferGeometryUtils.toCreasedNormals() and BufferGeometry.toNonIndexed() look like?
toCreasedNormals() is also used by e.g. React Three Drei’s <RoundedBox> and triggers the warning out of the box.
Changing toCreasedNormals to only call toNonIndexed if the geometry is indexed would be a fine change imo. Feel free to make a PR!
xkpv
July 4, 2023, 3:55pm
3
Thanks for your input.three-stdlib, maybe you can have a look at it.
I opted for the simplest solution which will just skip the call and update the provided geometry in-place & return it.
opened 03:27PM - 04 Jul 23 UTC
bug
- `three` version: 0.154.0
- `@types/three` version: 0.152.1
- `three-stdlib` … version: v2.23.10
### Problem description:
`BufferGeometryUtils.toCreasedNormals()` produces a warning if the provided geometry is non-indexed:
```
THREE.BufferGeometry.toNonIndexed(): BufferGeometry is already non-indexed.
toNonIndexed @ three.module.js:10964
toCreasedNormals @ BufferGeometryUtils.js:1240
```
The method calls `BufferGeometry.toNonIndexed()` to convert possibly indexed geometries to non-indexed ones but does not check whether a geometry is already non-indexed.
It seems that `BufferGeometry.toNonIndexed()` should not be called when the geometry is not indexed, producing the warning.
In this case and contrary to documentation, **no new geometry** is returned but `this` geometry, which may be the reason for the warning, and the functions that trigger such a warning would be working on the original geometry, not on a non-indexed copy.
See also discussion on Three.js discourse:
[BufferGeometry.toNonIndexed() warns about `BufferGeometry is already non-indexed`](https://discourse.threejs.org/t/buffergeometry-tononindexed-warns-about-buffergeometry-is-already-non-indexed/53497)
### Relevant code:
See [CodeSandbox example](https://codesandbox.io/s/nice-agnesi-j75cph) with @react-three/drei's `<RoundedBox>` which uses the method:
```js
<RoundedBox material={material} rotation={[0.5, 0.5, 0]} />
```
drei's `<RoundedBox>` would also need to be updated in some way, but this is about making it possible to use `BufferGeometryUtils.toCreasedNormals()` without running into the warning and to ensure that the original geometry does not change.
### Suggested solution:
Alternatives:
1. Call `BufferGeometry.toNonIndexed()` only if geometry is indexed. Use the original geometry, as is already the case because `BufferGeometry.toNonIndexed()` returns `this` if the geometry is non-indexed.
- ✅ Simple solution that removes the warning without changing behavior otherwise
- ✅ Backwards compatible: drei's `<RoundedBox>` depends on this behavior.
- ❌ Keeps modifying the original geometry if it is non-indexed.
2. Clone the geometry if it is non-indexed. Always operate on a new geometry.
- ✅ Simple predictable behavior that follows the immutability pattern
- ❌ Breaking change
3. Require the provided geometry to be indexed, fail otherwise.
- ❌ Not very useful
- ❌ Breaking change
4. Add optional parameter (e.g. `cloneIfNonIndexed`) to control behavior. If missing or falsy, behave like in suggestion **1.**. If truthy, clone the geometry if it is already non-indexed like in **2.**.
- ✅ Optional immutability pattern
- ✅ Backwards compatible (unless someone had been calling `toCreasedNormals()` with a third argument that is truthy)
- ❌ Redundant additional checks that could run outside the method – the caller may have cloned already
I prefer solution **1.** because it is the simplest.
See pull request https://github.com/pmndrs/three-stdlib/pull/269