greggman / wgpu-matrix

Fast WebGPU 3d math library
https://wgpu-matrix.org
MIT License
298 stars 13 forks source link

Consider changing Vec3, Mat4, etc to just Float32Array? #27

Closed greggman closed 4 months ago

greggman commented 6 months ago

I saw some code like this

   const matrix = mat4.identity();
   device.writeBuffer(uniformBuffer, 0, matrix); 

This fails in typescript because Mat4 is Float32Array | Float64Array | number[] and number[] is not compatible as a parameter to device.writeBuffer so you end up having to cast.

I thought about changing it so all wgpu functions take a XXXArg but return XXX where XXXArg includes extra types but XXX does not. As in:

type Vec3 = Float32Array;
type Vec3Arg = Vec3 | Float64Array | number[];

type Mat4 = Float32Array;
type Mat4Arg = Mat4 | Float64Array | number[];

const vec3 = {
   cross:(a: Vec3Arg, b: Vec3Arg): Vec3;
   ...
};

const mat4 = {
   lookAt(eye: Vec3Arg, target: Vec3Arg, up: Vec3Arg): Mat4;
   ...
}

This way you can still pass raw arrays but you get back something that's compatible with WebGPU

   const matrix = mat4.lookAt(
      [0, 5, 10], // eye
      [0, 3, 0],  // target;
      [0, 1, 0],  // up
   );

But, it also means you can no longer use the functions as is, for non-Float32Array stuff

   const matrixOfNumbers = mat4.lookAt(
      [0, 5, 10], // eye
      [0, 3, 0],  // target;
      [0, 1, 0],  // up
      [],         // dst           // makes lookAt return an array of numbers
   );

Note: I've never used this feature.

Similarly you could no longer reconfigure the function via setDefaultType (though again I've never used these functions)

Should I make this change?

FINDarkside commented 6 months ago

I think it would be better to come up with solution to what I consider to be the root problem: The "global" defaultType makes it impossible to have correct types for the created vectors, matrices etc. If the api was something like:

const { mat4 } = wgpuMatrix(Array);
const matrix = mat4.identity();

you could actually have correct type for matrix and remove the need for manual type assertions. The default export could be wgpuMatrix(Float32Array) for convenience.

Another option which might be possible. I'll post this since I spent some time figuring it out, but I think the first solution is way better.

Change default TS return type to be Float32Array but keep setDefaultType and come up with some nice solution where you add d.ts file into your project to override the default type when necessary. This is the best I was able to come up with:

wgpu-matrix.d.ts (this file goes to any project using wgpu-matrix)

declare module 'wgpu-matrix' {
  import { Vec2 } from 'wgpu-matrix/dist/2.x/vec2';
  import * as origvec2 from 'wgpu-matrix/dist/2.x/vec2-impl';

  type vec2Type = typeof origvec2;
  type vec2 = {
    [K in keyof vec2Type]: vec2Type[K] extends (...args: infer Args) => Vec2
      ? (...args: Args) => [number, number]
      : vec2Type[K];
  };

  export const vec2: vec2;
}

now you'd get the correct type for all vec2 operations (assuming you remember to call setDefaultType).

import { vec2 } from 'wgpu-matrix';
// [number, number]
const matrix = vec2.create();

It feels a bit dirty for sure and I'm sure it's possible to come up with something more elegant when modifying the actual type definitions of this module is an option. It requires more work and still requires you to make sure you make the type match whatever you call setDefaultType with. Also nothing prevent you from calling setDefaultType later making the types incorrect. That's why I think the first solution would be way better.

stefnotch commented 5 months ago

For completeness, there's also the option of using generics, and making the output type always the same as the input type.

const vec3 = {
   cross<T extends Vec3 = Float32Array>(a: T, b: T): T;
   ...
};

https://www.typescriptlang.org/play/?#code/C4TwDgpgBAahDGBmKBeKAxANgewIbEQCYBBAJ1NxCgB8Md8A2AFjIqtoDsBXAWwCMIpANoBdANwAoCfGwcAzsCgA3BMjQBvCVG3xS2OXIA8AFSgQAHsAgcAJnNirUdPARLlKAPgAUuAFxRjABooPn9jAEowqE1tbVIIYC5SDihuTEwoXHtcDhBJWIBfCQLJCUwEzKcVJAA6XX05LyEARkDCQMQRYKEmQIBWQIYRcLEgA

I personally prefer the

const { mat4 } = wgpuMatrix(Array);
const matrix = mat4.identity();

option, because it means that I, and a random npm library, can both use wgpu-matrix and won't ever run into weird conflicts because I'm using number[] while the npm library globally assumes Float32Arrays.

To make that option ergonomic to use, I'd create a math.ts file in my project, where I import all the relevant wgpu-matrix types, and re-export them. That way, whenever I type mat4 in any Typescript file, my IDE will suggest an import with types that make sense for my project.

stefnotch commented 5 months ago

Yet another option would be having an API for each type, like vec3 (defaults to Float32Array), vec3d for Float64Array and vec3n for number[];

I personally think that setDefaultType is rarely what one wants, since it globally affects code, including potentially affecting other libraries that happen to use wgpu-matrix.

magnostherobot commented 4 months ago

I saw some code like this

   const matrix = mat4.identity();
   device.writeBuffer(uniformBuffer, 0, matrix); 

I'm currently trying to do very similar in TS, and encountering this problem. Right now I'm casting to Float32Array; is this an okay workaround for the time being?

greggman commented 4 months ago

I saw some code like this

   const matrix = mat4.identity();
   device.writeBuffer(uniformBuffer, 0, matrix); 

I'm currently trying to do very similar in TS, and encountering this problem. Right now I'm casting to Float32Array; is this an okay workaround for the time being?

Yes, you can do that. It's an okay workaround

My coding style doesn't run into this because because the odds of me having just one matrix are low. For me, I have 2 styles.

  1. views into a array buffer

    struct Uniforms {
     world: mat4x4f,
     viewProjection: mat4x4f;
    }
    const uniformValues = new Float32Array(32);
    const world = uniformValues.subarray(0, 16);
    const viewProjection = uniformValues.subarray(16, 32);
    
    mat4.identity(world);
    mat4.perspective(fov, aspect, 1, 10, viewProjection);
    
    device.queue.writeData(uniformBuffer, 0, uniformValues);
  2. use webgpu-utils

I'm not saying anyone is doing anything wrong. Only that I didn't run into this issue until recently

greggman commented 4 months ago

So I tried @FINDarkside's idea I think

It need some cleanup but there's a test here (only refactored mat4)

https://github.com/greggman/wgpu-matrix/tree/refactor-getAPI-type

Unfortunately TypeDoc fails to generate docs. (npm run docs). It could be something I'm doing or it could be a bug in TypeDoc.

One problem I ran into is dst ends up having to be the type. With other JS style you could do this

 mat4.identity(someFloat32Array);
 mat4.identity(someNativeArray);

But with this typed version only the type assigned to dst is allowed.

Like I said above, that makes me think I should just do Float32Array only

That's here (again, only tried mat4)

https://github.com/greggman/wgpu-matrix/tree/refactor-just-float32array

greggman commented 4 months ago

v3.x is release which I hope solves this issue.