feat: .d.ts type annotations

[CodyJasonBennett]

Partial implementation of #24. Adds type annotations for core/math. I won't attempt extras for now.

Validated against TS source -- https://github.com/CodyJasonBennett/ogl/tree/feat/types.

[pschroen]

Is this based on ogl-types?

I was also working on this and have more type definitions to add...

[CodyJasonBennett]

Yes, I can defer changes here, this is at least complete for the core/math methods. These annotations were generated via tsc from the linked branch which I rewrote in TS with ogl-types open. I found a few errors where things should be null (Transform.parent, setParent(parent: Transform | null), renderer state) and ensured that methods with overloads had a correct latter signature with added helper types for complex args.

[pschroen]

Cool, no go for it, this is great. 👍

I found the same and also have some of the extras ready to go. I've been testing them so far with a vanilla TS and Rollup build and the examples as TS.

I'll test this PR against what I have for both a vanilla TS project and Angular, I'm expecting they'll be the same but will let you know if I find any differences, cheers! 😉

[pschroen]

Alright, I've created a CodeSandbox template of my vanilla TS setup we can use for testing if you like.

In the package.json I'm using your branch of OGL with the types.

// package.json
{
  "dependencies": {
    "ogl": "CodyJasonBennett/ogl#feat/type-annotations"
  }
}

For example, here's the first OGL example from the readme as a really quick TS file, it's not fully typed but works as a quick way to validate any issues with the type definitions.

In CodeSandbox it takes a minute for the definitions to load in the web interface, but they do work there.

So in this first example the only issue is the missing Box extra.

Module '"ogl"' has no exported member 'Box'.ts(2305)

https://codesandbox.io/s/ogl-types-readme1-9qhjff

And in my Angular project there were a number of issues that needed to be fixed with ogl-types, one of which you had already fixed above for the Transform.parent being null.

Here's the other ones:

1. The same open issue from CodyJasonBennett/ogl-types#2, this one is a can of worms though I think, because all the math classes extend Array, wouldn't that mean all of the math classes could potentially be passed as their equivalent Array syntax?

error TS2345: Argument of type 'number[]' is not assignable to parameter of type 'Vec3'.

As a quick hack I did the following down all the connected lookAt() methods, but this needs more discussion I think on how to handle this across the library.

    lookAt(target: Vec3 | [number, number, number]): this

2. The Attribute.data type should be Float32Array, and the corresponding matrix array methods (fromArray(), toArray()) needs support for Float32Array as well. For example I did the following:

  export type Attribute = {
    size: number
    data: Float32Array

    fromArray(a: Float32Array | Array<number>, o?: number): this
    toArray(a?: Float32Array | Array<number>, o?: number): Float32Array | Array<number>

I have some more fixes too but for the extras.

All this to say, even though we could merge this PR as it's only for core/math, we shouldn't do a new release for these typings until they are feature complete and at the very least work with all the OGL examples imho.

Perhaps we merge this PR for now, and then follow-up with subsequent PRs for fixes and the remaining extras?

I'll also create tests for the above issues where we can validate the fixes.

Let me know what you guys think! 😊

[CodyJasonBennett]

There are tuple types for each math class and their array form. They represent cases where only the data is expected, not the math interface. All math methods could expect this tuple type since the gl-matrix functions only operate on arrays. If that is only supposed to be an implementation detail, perhaps lookAt and similar methods can have an overload like lookAt(target: Vec3) | lookAt(x: number, y: number, z: number) and not expect a tuple type.

Geometry indeed can only accept float typed arrays for attributes (vertexAttribIPointer is not implemented) with the exception of indices which must be an integer type. I can narrow this to ArrayBufferView or enumerate all possible constructors. FWIW WebGPU only allows uint32-uint16 for index buffers.

I'm not inclined to merge anything until it's ready to release. I'll leave that criteria to you although this PR was only intended as a closer baseline than ogl-types. Perhaps we can work off of a types feature branch in the ogl repo if you want to make incremental changes without blocking releases on master?

[pschroen]

Sounds good, and ya it'll be awhile before the types are ready for a release so I'm in support of a types feature branch we can work from.

[pschroen]

I can narrow this to ArrayBufferView or enumerate all possible constructors.

I tried ArrayBufferView from your last commit:

  export interface Attribute {
    size: number;
    data: ArrayBufferView;

And get the following error:

Property 'set' does not exist on type 'ArrayBufferView'.

Unless you know of another way I think we'll need to specify all three, which works:

  export interface Attribute {
    size: number;
    data: Float32Array | Uint32Array | Uint16Array;

[pschroen]

Related issue:

• microsoft/TypeScript#15402

[pschroen]

Thanks, latest commit works great! 👍

Have another question/clarification on the use of the tuple types, for example Vec3Tuple, is it meant to be imported?

It's not used in the Vec3 class definition, and for example I had to update the set() method to allow a Vec3, number[] or number as the first argument:

    set(x: Vec3 | number[] | number, y?: number, z?: number): this;

[CodyJasonBennett]

See my comment in https://github.com/oframe/ogl/pull/177#issuecomment-1669892207. Again, an array of any length has a different interface than a class which extends an array and implements class methods. That's why Array<number> is different than class Vec3 extends Array<number> { ... }.

[pschroen]

Not sure I follow, are you saying to do something like the following with multiple signatures?

    set(x: number, y?: number, z?: number): this;
    set(v: Vec3): this;
    set(a: Vec3Tuple): this;

[CodyJasonBennett]

It needs to be known whether it's valid to pass an array over a math class anywhere. I'm personally not a fan of leaking gl-matrix internals and find it makes APIs like lookAt (as mentioned in https://github.com/oframe/ogl/pull/177#issuecomment-1669892207) cumbersome to use as-is. That can be addressed separately, but I can't gauge intent alone and have divergent design opinions from this library with API ergonomics that'll only weigh this down.

At the very least, overloads' last signature should combine all the others so that dynamic inference in TS will work.

set(x: number, y?: number, z?: number): this;
set(v: Vec3): this;
set(a: Vec3Tuple): this;
// Needed since TS doesn't join overloads for you.
// https://github.com/microsoft/TypeScript/issues/32164
set(x: number | Vec3 | Vec3Tuple, y?: number, z?: number): this;

[pschroen]

Got it, thanks for the explanation!

Let's go with the combined signature then in the meantime. 😊

[pschroen]

Alright, so I finally have a cleaned-up and nearly complete set of types for both the core/math classes and the extras that works with all 46 examples (48 if you include the readme examples).

Related PR:

• 185

You can see the updated types on my fork: https://github.com/pschroen/ogl/tree/feat/types

@CodyJasonBennett would you like me to update this existing PR, or open a new one?

[pschroen]

Also for whatever reason the CodeSandbox definitions don't load anymore. 🤷‍♂️

It's working for me locally though if you install directly from my fork, for example:

npm i pschroen/ogl#feat/types

or

yarn add ogl@pschroen/ogl#feat/types

[CodyJasonBennett]

I'm not able to champion this at the moment so please do open a new PR. Codesandbox I've noticed having issues with types resolving everywhere, not just here.

[CodyJasonBennett]

Continuing in #188.