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

BufferGeometryUtils.toCreasedNormals() produces warning for non-indexed geometries #26378

Closed
kpvhn opened this issue Jul 5, 2023 · 1 comment

Comments

@kpvhn
Copy link
Contributor

kpvhn commented Jul 5, 2023

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.

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.
  1. Clone the geometry if it is non-indexed. Always operate on a new geometry.
  • ✅ Simple predictable behavior that follows the immutability pattern
  • ❌ Breaking change
  1. Require the provided geometry to be indexed, fail otherwise.
  • ❌ Not very useful
  • ❌ Breaking change
  1. 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
  1. Change BufferGeometry.toNonIndexed() to return a cloned geometry if it is already non-indexed
  • ❌ The caller can check if a geometry is indexed by checking the BufferGeometry.index property, which is what other code does, and clone the geometry if needed
  • ❌ Breaking change
  1. Change BufferGeometry.toNonIndexed() to throw an error
  • ❌ Same drawbacks as 5. in addition to:
  • ❌ Breaks all downstream code – toCreasedNormals() would need to change, and some code like @react-three/drei's <RoundedBox> rely on toCreasedNormals() to mutate the original geometry (though I should mention that drei uses three-stdlib, not the code from three.js examples.

I prefer solution 1. because it is the simplest.

See PR #26379

See also

Reproduction steps

  1. Create a non-indexed geometry
  2. Call BufferGeometryUtils.toCreasedNormals() on the geometry
  3. See browser Dev Tools console for warning: THREE.BufferGeometry.toNonIndexed(): BufferGeometry is already non-indexed.

Code

import { BoxGeometry } from "three";
import { toCreasedNormals } from "three/examples/jsm/utils/BufferGeometryUtils.js";

// produces warning
toCreasedNormals(new BoxGeometry().toNonIndexed());

Live example

Screenshots

No response

Version

154

Device

No response

Browser

No response

OS

No response

@Mugen87
Copy link
Collaborator

Mugen87 commented Jul 18, 2023

Fixed via #26379.

@Mugen87 Mugen87 closed this as completed Jul 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants