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

Draft: sketch out approach to better BufferAttribute typings #35 #241

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kmannislands
Copy link

@kmannislands kmannislands commented Aug 5, 2022

This is a draft to collect feedback on the idea of improving specificity of BufferAttribute typings, especially as they relate to BufferGeometry typings

Why

The current typing of attributes on BufferGeometries is too wide. It's not possible to specify what attributes are actually defined and what "flavor" of TypedArray actually underlies them.

What

I've made the BufferAttribute type generic so that the backing TypedArrays can be specified. I've leveraged this in the definition of BufferGeometry to make it possible to specify the attributes and their underlying types.

With this change, the following code works:

type geom = BufferGeometry<{ normal: Float32Array,  position: Uint16Array }>
type normal = geom['attributes']['normal'] // InterleavedBufferAttribute | BufferAttribute<Float32Array>
type position = geom['attributes']['position'] // InterleavedBufferAttribute | BufferAttribute<UInt16Array>

// @ts-expect-error Property 'foo' does not exist on type 'AttributesFor<{ normal: Float32Array; position: Uint16Array; }>'
type foo = geom['attributes']['foo'] // type error!

Using the previous wide type as default allows backwards compatibility:

type geom = BufferGeometry
type foo = geom['attributes']['foo']; // InterleavedBufferAttribute | BufferAttribute<TypedArray>

If there's an appetite for this sort of change, I'll button up the PR here with:

  • Handling for all variants of TypedArray
  • Handling of all methods of BufferGeometry
  • Better handling/generic typing of InterleavedBufferAttribute in the same vein as the way BufferAttribute is handled

Checklist

  • Checked the target branch (current goes master, next goes dev)
  • Added myself to contributors table
  • Ready to be merged

@Methuselah96
Copy link
Contributor

I like where this is going. Could it be expanded to let the user say that an attribute is not a GLBufferAttribute (see #389)?

@Methuselah96
Copy link
Contributor

Thanks for your work on this, sorry for the radio silence. I'm looking into building off of this to allow distinguishing between the different buffer attribute types (as discussed in #389). Let me know if you would rather do that work yourself, and I'd be happy to review it.

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.

2 participants