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

[RFC] Should BufferAttribute.array be TypedArray instead of ArrayLike? #35

Closed
joshuaellis opened this issue Feb 28, 2021 · 3 comments · Fixed by #515
Closed

[RFC] Should BufferAttribute.array be TypedArray instead of ArrayLike? #35

joshuaellis opened this issue Feb 28, 2021 · 3 comments · Fixed by #515
Labels
bug Something isn't working current release issues to do with the current release of Three request for comments Discussion about changes to the library

Comments

@joshuaellis
Copy link
Member

Problem description:

Documented as BufferAttributes only being able to be given a TypedArray, but when you call attribute.array the return type is ArrayLike instead of a version of TypedArray – https://threejs.org/docs/#api/en/core/BufferAttribute

this becomes a problem in code like estimateBytesUsed in BufferGeometryUtils:

export function estimateBytesUsed(geometry: BufferGeometry) {
  let mem = 0
  for (let name in geometry.attributes) {
    const attr = geometry.getAttribute(name)
    mem += attr.count * attr.itemSize * (attr.array as TypedArray).BYTES_PER_ELEMENT // you have to typecase to get BYTES_PER_ELEMENT because it's not on ArrayLike
  }

  const indices = geometry.getIndex()
  mem += indices ? indices.count * indices.itemSize * (indices.array as TypedArray).BYTES_PER_ELEMENT : 0
  return mem
}

Suggested solution:

Could we use a generic to detect the type of TypedArray passed to the Attribute so that type is returned when we call attribute.array?

@joshuaellis joshuaellis added bug Something isn't working request for comments Discussion about changes to the library current release issues to do with the current release of Three labels Feb 28, 2021
@dangerski
Copy link

This is also a problem when you try to update attribute values, which is shown in the threejs documentation. ArrayLike is readonly so you can't change values using [].

interface ArrayLike<T> {
    readonly length: number;
    readonly [n: number]: T;
}

https://threejs.org/docs/#manual/en/introduction/How-to-update-things

positions will be readonly so this example from the documentation won't work in TS

const positions = line.geometry.attributes.position.array;

let x, y, z, index;
x = y = z = index = 0;

for ( let i = 0, l = MAX_POINTS; i < l; i ++ ) {
    positions[ index ++ ] = x;
    positions[ index ++ ] = y;
    positions[ index ++ ] = z;
    x += ( Math.random() - 0.5 ) * 30;
    y += ( Math.random() - 0.5 ) * 30;
    z += ( Math.random() - 0.5 ) * 30;

}

@kmannislands
Copy link

kmannislands commented Aug 5, 2022

For app code working with buffer geometries, it would also be nice to go a bit further and narrow the specific attributes that are defined as well as their respective types. This would require making BufferAttribute generic as well.

const geom = new BufferGeometry<{ normal: Float32Array,  position: Uint16Array }>

geom.attributes.normal // BufferAttribute<Float32Array>
geom.attributes.position // BufferAttribute<UInt16Array>
geom.attributes.foo // type error!

I think this can be made nicely backwards-compatible by setting sensible defaults. I'm willing to contribute this feature.

@kmannislands
Copy link

Submitted a proposed approach, will wait for a signal: #241

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working current release issues to do with the current release of Three request for comments Discussion about changes to the library
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants