Babylon.js icon indicating copy to clipboard operation
Babylon.js copied to clipboard

Tensor: Standardize Vector2, Vector3, Vector4, Color3, Color4, Quaternion, and Matrix

Open james-pre opened this issue 1 year ago • 83 comments

Discussion of this PR is available on the Babylon.js forum in this thread. Resolves #14201

TL;DR:

Tensor would be added, which provides a standardized API for interacting with vectors, colors, matrices, quaternions, etc.

Rational

This PR aims to create a standard interface from which various math constructs are defined.

Take the current difference in behavior between vectors and colors:

const c1 = Color3.Random(),
	c2 = Color3.Random(),
	c3 = new Color3(),
	v1 = Vector3.Random(),
	v2 = Vector3.Random(),
	v3 = new Vector3();

c1.addToRef(c2, c3) === c3 // false
v1.addToRef(v2, v3) === v3 // true

And the lack of a method that should exist in Color3:

const v1 = Vector3.Random(),
	v2 = Vector3.Random(),
	c1 = Color3.Random(),
	c2 = Color3.Random();


v1.addInPlace(v2) // ok
c1.addInPlace(c2) // ReferenceError! addInPlace does not exist

Tensor provides a single interface for tensor-like objects which users can depend on. By having tensor-like classes implement Tensor, it ensures that all the methods exist and have the same and correct behavior. It is meant for organization and standardization.

Semantics

Note:

  • CurrentClass is a placeholder for a class that implements Tensor. In fields, it is the current class.
  • Tensor and its related types will be defined in src/Maths/tensor.ts unless otherwise noted
  • Signatures are shown, not implementation.
  • The shown signatures are not necessarily the same as implementation (e.g. Signature<T extends number> may be shown when it is implemented as Signature<T> = T extends number ? ... : never

Status

  • [x] Vector
  • [x] Tuple manipulation and math types
  • [x] Constructor
  • [x] MultidimensionalArray
  • [x] TensorValue
  • [x] Tensor
    • [x] Methods from Vector
    • [x] Tensor.dimension
    • [ ] Tensor.From
    • [ ] Tensor.as
    • [ ] Tensor.sum
    • [x] Tensor.rank
    • [ ] Tensor.value
  • [ ] isTensor
  • [ ] isTensorValue
  • [ ] getTensorDimension
  • [ ] convertTensor*

* May not be included in the proposal

Vector

Defined in src/Maths/math.vector.ts

This PR includes Vector from #13699. Vector extends Tensor and adds normalization methods, length, and lengthSquared. It is implemented by Vector2, Vector3, and Vector4.

Tuple manipulation and math types

Defined in src/types.ts The following types are being added for compile-time math and tuple manipulation, which is necessary for MultidimensionalArray. They are included as a single block for brevity.

type Empty = [];
type Shift<T extends unknown[]>;
type First<T extends unknown[]>;
type Unshift<T extends unknown[], A>;
type Pop<T extends unknown[]>;
type Last<T extends unknown[]>;
type Push<T extends unknown[], A>;
type Concat<A extends unknown[], B extends unknown[]>;
type Remove<A extends unknown[], B extends unknown[]>;
type Length<T extends unknown[]>;
type FromLength<N extends number>;
type Increment<N extends number>;
type Decrement<N extends number>;
type Add<A extends number, B extends number>;
type Subtract<A extends number, B extends number>;
type Member<T, D>;
type Flatten<A extends unknown[], D>;
type MultidimensionalArray<T, D extends number>
type Tuple<T, N extends number>;
type FlattenTuple<T>;
type IsTuple<T>;

Constructor

Defined in src/types.ts

type Constructor<C extends new (...args: any[]) => any, I extends InstanceType<C>>

Defines a single type for constructors. This replaces Vector2Constructor, Vector3Constructor, Vector4Constructor, QuaternionConstructor, and MatrixConstructor.

Instead of using Vector3Constructor<this>, you would instead use Constructor<typeof Vector3, this>. The typeof is needed to get the type for the class instead of the instance, and can't be done inside the type since Typescript prohibits taking the typeof a type.

MultidimensionalArray

Defined in src/types.ts

type MultidimensionalArray<T, D extends number>

Represents a multidimensional array of T with depth D.

TensorValue

type TensorValue<T> = T extends Tensor<infer V> ? V : never;

Tensor

declare class Tensor<V>

Tensor is a declare class for tensor-like classes to implement.

Tensor includes all of the methods in Vector from #13699. This includes

  • Math operations (add, subtract, multiply, divide, scale, ...)
  • Array conversion (fromArray, toArray, asArray, ...)
    • Since the type parameter is no longer assignable to number[], the return type for array-related methods is number[]. See Tensor.value
  • Transferring (clone, copyFrom, copyFromFloats, ...)
  • The above methods' InPlace, ToRef, FromFloats, etc.

It adds or changes the following methods:

Tensor.dimension

public abstract dimension: number[];

dimension is the mathematical dimension2 of the tensor. In dynamic tensor types, it can be defined as a getter.

Example:

const vec3 = new Vector3(),
	vec4 = new Vector4(),
	matrix = new Matrix();

vec3.dimension // [3]
vec4.dimension // [4]
matrix.dimension // [4, 4]

Tensor.From

public static From(source: Tensor, fillValue: number = 0): CurrentClass;

From creates a new instance of CurrentClass from source. source: The tensor to copy data from. fillValue: The value to use for filling empty parts of the resulting CurrentClass.

Example:

const vec3 = new Vector3(1, 2, 3);
const vec4 = Vector4.From(vec3, 4); // { 1, 2, 3, 4 }
const matrix = Matrix.From(vec3, 0); // this is possible!

Additionally, From could be changed to From(...args: [...Tensor[], number]): CurrentClass to allow multiple inputs. This is better understood by this invalid Typescript signature (since rest parameters must be last):

public static From(...source: Tensor[], fillValue: number = 0): CurrentClass;

🛈 Implementations may write their own conversion code or use convertTensor.

Tensor.as

public as<T extends typeof Tensor>(type: T, fillValue: number = 0): InstanceType<T>;

as creates an instance of type from an instance of CurrentClass. type: The class to create an instance of. fillValue: The value to use for filling empty parts of the resulting instance.

Example:

const vec3 = new Vector3(1, 2, 3);
const vec4 = vec3.as(Vector4, 4); // { 1, 2, 3, 4 }
const matrix = vec3.as(Matrix, 0);

🛈 Implementations may write their own conversion code or use convertTensor.

Tensor.sum

public sum(): number;

sum return the sum of the components of the Tensor.

Example:

const vec3 = new Vector3(1, 2, 3),
	vec4 = new Vector4(4, 5, 6, 7),
	matrix = new Matrix();

vec3.sum() // 6
vec4.sum() // 22
matrix.sum() // 0

⚠ The return value of the lengthSquared or length method of vectors is not the same as the return value of sum.

Tensor.rank

public abstract rank: number;

The rank of a tensor is the number of indices required to uniquely select each element of the tensor.

Example:

const vec3 = new Vector3(),
	matrix = new Matrix();

vec3.rank // 1
matrix.rank // 2

🛈 The rank of a Tensor is the same as its dimension.length.

Tensor.value

public get value(): V;
public set value(value: V): void;

value is the values of the tensor in a multidimensional array with the type V (the type parameter of the class). Unlike dimension, this must be implemented as a getter and setter.

Example:

const vec3 = new Vector3(),
	matrix = new Matrix();

vec3.value
// [0, 0, 0]

matrix.value
// [ [ 0, 0, 0, 0 ], [ 0, 0, 0, 0 ], [ 0, 0, 0, 0 ], [ 0, 0, 0, 0 ] ]

vec3.value = [ 1, 2, 3 ];
vec3.x // 1

isTensor

function isTensor<V>(value: unknown): value is Tensor<V>;

Checks if a value is a Tensor. Example:

const vec3 = new Vector3();
isTensor(vec3) // true
isTensor(vec3.value) // false
isTensor([1, 2, 3]) // false
isTensor(0) // false
isTensor(null) // false

isTensorValue

function isTensorValue<T>(value: unknown): value is TensorValue<T>;

Checks if a value is a TensorValue. Example:

const vec3 = new Vector3();
isTensorValue<1>(vec3) // false
isTensorValue<Vector3>(vec3.value) // true
isTensorValue<1>([1, 2, 3]) // true
isTensorValue<0>([1, 2, 3]) // false
isTensorValue<1>(0) // false
isTensorValue<0>(0) // true
isTensorValue<1>(null) // false

getTensorDimension

function getTensorDimension(value: Tensor | TensorValue): number[];

getTensorDimension returns the mathimatical dimension2 of value, similar to Tensor.dimension. If value is not a Tensor or TensorValue, it will throw a TypeError

🛈 Dynamic Tensor based classes should use getTensorDimension for their dimension implementations. Fixed-dimension Tensor sub-classes need not use getTensorDimension.

(optional) convertTensor

function convertTensor<T extends  typeof Tensor>(TensorClass: T, tensor: Tensor, fillValue: number = 0): InstanceType<T>;

Creates a new T with the values of tensor. Unlike Tensor.as, it performs more error checking and does not need a starting instance (and therefore avoids the dreaded 'methodName' does not exist on undefined error).

Behavior changes

  • The Color3 and Color4 methods multiplyToRef, scaleToRef, scaleAndAddToRef, clampToRef, addToRef, and subtractToRef have had their return types changed from this to the reference instance.

Considerations

  1. Static methods related to data manipulation (e.g. Add, Normalize, Lerp) and non-static normalization methods are outside the scope of this PR. While Tensor may include them in the future, this PR does not
  2. This PR does not include a dimensionally dynamic Tensor.
  3. Performance: Since Tensor is defined using the declare class and classes that follow Tensor do so using the implements keyword, there are no runtime changes to Tensor-based classes.
  4. Bundle size: The JS bundle size will increase only by the size of the added functions (isTensor, getTensorDimension, etc.). The TS declaration size will also increase slightly due to Tensor and the added utility types.
  5. Maintenance: Since there is no implementation (just a class declaration), the extra maintenance is minimized to member signatures.

Questions

For the BabylonJS team concerning the PR

  1. What other classes (e.g. Size) would be included and standardized?

FAQs

What use cases does this proposal have?

Tensor is not intended to be used by users. It is meant for internal organization and standardization. The functions (isTensor, getTensorDimension, etc.) are intended to be public and may be used by users. Their use cases may be inferred from their descriptions.

What is the difference between this and #13699?

The capabilities of the type. Vector supports any rank 1 tensor. Tensor generalizes Vector, and supports any rank tensor. A mathematical vector can be represented as number[]. A mathematical tensor can be represented as a number, number[], number[][], and so on. This table highlights the capability:

tensor rank TS representation BJS class (if exists)
1 number[] Vector
1 [number, number] Vector2
1 [number, number, number] Vector3, Color3
1 [number, number, number, number] Vector4, Color4, Quaternion
2 number[][] Matrix
3 number[][][]
4 number[][][][]
N TensorValue

How is incompatibility managed? Are incompatible classes dropped or are they modified to follow Tensor?

Any classes included will be modified to follow the standard. In many cases, the diverging behavior makes the engine difficult to use, and changing the claseses to follow the Tensor standard benefits end users.

Further reading

1. https://en.wikipedia.org/wiki/Tensor 2. https://en.wikipedia.org/wiki/Dimension

james-pre avatar Aug 30 '23 22:08 james-pre

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). To prevent this PR from going to the changelog marked it with the "skip changelog" label.

bjsplat avatar Aug 30 '23 22:08 bjsplat

Snapshot stored with reference name: refs/pull/14235/merge

Test environment: https://babylonsnapshots.z22.web.core.windows.net/refs/pull/14235/merge/index.html

To test a playground add it to the URL, for example:

https://babylonsnapshots.z22.web.core.windows.net/refs/pull/14235/merge/index.html#WGZLGJ#4600

Links to test babylon tools with this snapshot:

https://playground.babylonjs.com/?snapshot=refs/pull/14235/merge https://sandbox.babylonjs.com/?snapshot=refs/pull/14235/merge https://gui.babylonjs.com/?snapshot=refs/pull/14235/merge https://nme.babylonjs.com/?snapshot=refs/pull/14235/merge

To test the snapshot in the playground with a playground ID add it after the snapshot query string:

https://playground.babylonjs.com/?snapshot=refs/pull/14235/merge#BCU1XR#0

bjsplat avatar Aug 30 '23 23:08 bjsplat

Small note - there are a few type issues, building core was not successful.

RaananW avatar Sep 04 '23 18:09 RaananW

@kzhsw Thank you so much for the reviews. Another set of eyes helps to catch things missed.

james-pre avatar Sep 12 '23 18:09 james-pre

I love it!

jstroh avatar Sep 13 '23 15:09 jstroh

When this plan will be realized, this design is useful

GuoBinyong avatar Nov 10 '23 09:11 GuoBinyong

Visualization tests for webgl1 have failed. If some tests failed because the snapshots do not match, the report can be found at

https://babylonsnapshots.z22.web.core.windows.net/refs/pull/14235/merge/testResults/webgl1/index.html

If tests were successful afterwards, this report might not be available anymore.

bjsplat avatar Nov 10 '23 23:11 bjsplat

Visualization tests for webgl2 have failed. If some tests failed because the snapshots do not match, the report can be found at

https://babylonsnapshots.z22.web.core.windows.net/refs/pull/14235/merge/testResults/webgl2/index.html

If tests were successful afterwards, this report might not be available anymore.

bjsplat avatar Nov 10 '23 23:11 bjsplat

Visualization tests for webgl1 have failed. If some tests failed because the snapshots do not match, the report can be found at

https://babylonsnapshots.z22.web.core.windows.net/refs/pull/14235/merge/testResults/webgl1/index.html

If tests were successful afterwards, this report might not be available anymore.

bjsplat avatar Nov 11 '23 14:11 bjsplat

Visualization tests for webgl2 have failed. If some tests failed because the snapshots do not match, the report can be found at

https://babylonsnapshots.z22.web.core.windows.net/refs/pull/14235/merge/testResults/webgl2/index.html

If tests were successful afterwards, this report might not be available anymore.

bjsplat avatar Nov 11 '23 14:11 bjsplat

Vis tests failed on "Baked Vertex Animation" because PG#11 calls Matrix.toArray and expects an array as the return value, which is incorrect behavior. This is fixed in PG#13.

james-pre avatar Nov 11 '23 15:11 james-pre

I've made the following changes to the PR (see the first comment):

  • Removed Scalar from standardization
  • Fixed some example code
  • Separated some of the proposal to be done in a second stage/PR (Stage 2)

james-pre avatar Nov 11 '23 15:11 james-pre

Vis tests failed on "Baked Vertex Animation" because PG#11 calls Matrix.toArray and expects an array as the return value, which is incorrect behavior. This is fixed in PG#13.

Seems this makes a breaking change to the public api Matrix#toArray, maybe consider making a compatible mode or a major release?

kzhsw avatar Nov 13 '23 00:11 kzhsw

  1. Static methods related to data manipulation (e.g. Add, Normalize, Lerp) and non-static normalization methods are outside the scope of this PR. While Tensor may include them in the future, this PR does not

Where is this issue discussed ?

GuoBinyong avatar Nov 13 '23 02:11 GuoBinyong

Vis tests failed on "Baked Vertex Animation" because PG#11 calls Matrix.toArray and expects an array as the return value, which is incorrect behavior. This is fixed in PG#13.

Seems this makes a breaking change to the public api Matrix#toArray, maybe consider making a compatible mode or a major release?

The linked issue has been slated for 7.0 by the BJS team so I think it's ok to make breaking changes to fix bugs/correct behavior.

james-pre avatar Nov 13 '23 02:11 james-pre

  1. Static methods related to data manipulation (e.g. Add, Normalize, Lerp) and non-static normalization methods are outside the scope of this PR. While Tensor may include them in the future, this PR does not

Where is this issue discussed ?

It hasn't been discussed. I don't plan on including it in the initial PR, though I am open to adding more stuff if there is time before 7.0 is released.

james-pre avatar Nov 13 '23 17:11 james-pre

Vis tests failed on "Baked Vertex Animation" because PG#11 calls Matrix.toArray and expects an array as the return value, which is incorrect behavior. This is fixed in PG#13.

Seems this makes a breaking change to the public api Matrix#toArray, maybe consider making a compatible mode or a major release?

The linked issue has been slated for 7.0 by the BJS team so I think it's ok to make breaking changes to fix bugs/correct behavior.

Agreed, in the major version, the previous dross should be changed, rather than blindly backward compatibility and abandon optimization.

GuoBinyong avatar Nov 14 '23 10:11 GuoBinyong

It hasn't been discussed. I don't plan on including it in the initial PR, though I am open to adding more stuff if there is time before 7.0 is released.

It's best to discuss and fix it before the release of 7.0, otherwise we will need to wait until after the release of 7.0 to discuss it, and we need to consider the issue of forward compatibility.

Pull me on if there needs to be a discussion

GuoBinyong avatar Nov 14 '23 10:11 GuoBinyong

It's best to discuss and fix it before the release of 7.0, otherwise we will need to wait until after the release of 7.0 to discuss it, and we need to consider the issue of forward compatibility.

Pull me on if there needs to be a discussion

I think I will implement the static methods if time permits.

james-pre avatar Nov 14 '23 17:11 james-pre

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). To prevent this PR from going to the changelog marked it with the "skip changelog" label.

bjsplat avatar Nov 14 '23 17:11 bjsplat

Updated the main PR comment so that their isn't a stage 2, since there is time do it all of that in this PR.

james-pre avatar Nov 14 '23 17:11 james-pre

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). To prevent this PR from going to the changelog marked it with the "skip changelog" label.

bjsplat avatar Nov 14 '23 19:11 bjsplat

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). To prevent this PR from going to the changelog marked it with the "skip changelog" label.

bjsplat avatar Nov 14 '23 19:11 bjsplat

Snapshot stored with reference name: refs/pull/14235/merge

Test environment: https://babylonsnapshots.z22.web.core.windows.net/refs/pull/14235/merge/index.html

To test a playground add it to the URL, for example:

https://babylonsnapshots.z22.web.core.windows.net/refs/pull/14235/merge/index.html#WGZLGJ#4600

Links to test babylon tools with this snapshot:

https://playground.babylonjs.com/?snapshot=refs/pull/14235/merge https://sandbox.babylonjs.com/?snapshot=refs/pull/14235/merge https://gui.babylonjs.com/?snapshot=refs/pull/14235/merge https://nme.babylonjs.com/?snapshot=refs/pull/14235/merge

To test the snapshot in the playground with a playground ID add it after the snapshot query string:

https://playground.babylonjs.com/?snapshot=refs/pull/14235/merge#BCU1XR#0

bjsplat avatar Nov 14 '23 20:11 bjsplat

As this issue was raised in a few comments from a few different users, I will address it here (cc-ing @kzhsw , @dr-vortex , @GuoBinyong who have mentioned/discussed it)

Babylon is always backwards compatible. This is not up for debate, and is not related to major versions. Sometimes I don't agree with it, sometimes I think it is the best thing since sliced bread. But still - never break back-compat. I wrote a bit about it here - https://babylonjs.medium.com/there-and-back-again-a-tale-of-backwards-compatibility-in-babylon-js-47ffc4f7ed6f

We can add new methods, we can fix bugs, we can proxy methos and keep the old name. We can deprecate, where it makes sense. There are a few ways of dealing with every issue. However, breaking changes to an existing API will not be merged.

The entire team will be very happy to see this merged. Of course, after fully reviewing it. As our math lib is a central part of the framework (i don't think any of our other modules don't depend on it) it is very important for us that it fully works as expected, and in the fastest and most efficient way. Which is my way of apologizing for not investing a lot of time in reviewing it, as this review should not be taken lightly.

RaananW avatar Nov 15 '23 13:11 RaananW

I'm converting the PR to a draft since it is not finished yet. Once I'm done implementing the new Tensor methods and functions, I will reopen the PR.

james-pre avatar Nov 15 '23 22:11 james-pre

It is new years cleanup :-)

@dr-vortex should I close the PR and let you reopen once ready ?

sebavan avatar Jan 06 '24 01:01 sebavan

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). To prevent this PR from going to the changelog marked it with the "skip changelog" label.

bjsplat avatar Jan 06 '24 20:01 bjsplat

@sebavan The core of the PR is ready to merge. While some of the features that involve converting between tensors are not in this PR, I would like to merge the core of the API, which should make working on the related APIs easier to work with -- including the other parts of the proposal.

james-pre avatar Jan 06 '24 20:01 james-pre

Snapshot stored with reference name: refs/pull/14235/merge

Test environment: https://babylonsnapshots.z22.web.core.windows.net/refs/pull/14235/merge/index.html

To test a playground add it to the URL, for example:

https://babylonsnapshots.z22.web.core.windows.net/refs/pull/14235/merge/index.html#WGZLGJ#4600

Links to test babylon tools with this snapshot:

https://playground.babylonjs.com/?snapshot=refs/pull/14235/merge https://sandbox.babylonjs.com/?snapshot=refs/pull/14235/merge https://gui.babylonjs.com/?snapshot=refs/pull/14235/merge https://nme.babylonjs.com/?snapshot=refs/pull/14235/merge

To test the snapshot in the playground with a playground ID add it after the snapshot query string:

https://playground.babylonjs.com/?snapshot=refs/pull/14235/merge#BCU1XR#0

bjsplat avatar Jan 06 '24 20:01 bjsplat