Standardize Vectors and Colors

[james-pre]

Currently, we have 5 completely different classes: Vector2, Vector3, Vector4, Color3, and Color4. All 5 define some coordinates ({x, y, z}, {r, g, b}, etc.) and provide some methods for different operations on those coordinates (e.g. add, subtract, multiplyInPlaceFromFloats).

This pull request introduces a new class: Vector. A Vector represents any N-dimensional vector, providing the various methods and such in one class. The 5 classes mentioned above extend Vector and add any specific logic, as well as the getters and setters for named coordinates (e.g. x and y).

In addition to consolidating code, Vector ensures that some methods which were previously not available in one of the classes (e.g. minimizeInPlaceFromFloats) are available across all classes. Furthermore, if bug fixes or enhancements are made, they can be done in one place rather than five.

Compatibility / Interoperability

Vector methods that take a list of values have overloads for 2, 3, and 4 arguments/dimensions, so current classes always have an overload that matches their dimension.

[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]

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]

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]

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]

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

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

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

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

Links to test babylon tools with this snapshot:

https://playground.babylonjs.com/?snapshot=refs/pull/13699/merge
https://sandbox.babylonjs.com/?snapshot=refs/pull/13699/merge
https://gui.babylonjs.com/?snapshot=refs/pull/13699/merge
https://nme.babylonjs.com/?snapshot=refs/pull/13699/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/13699/merge#BCU1XR#0

[sebavan]

I love the overall idea, let s move it as a draft until the 6.0 version will be released.

What we want to be sure of is:

• Full back compat
• Performances should be better or identical
• Garbage collection should be better or identical

It would be nice to see perf tests to compare new and old version with measurements on Chrome, FF, Safari and Babylon Native.

Those classes are used so often we can not risk any perf hit in their usage.

[james-pre]

@sebavan I agree!

For performance, I was thinking of moving from using mostly array.forEach to using for loops (see this report).

[Popov72]

I agree with @sebavan and love the idea!

Regarding performance, what concerns me is the use of getter/setter everywhere, while we used direct properties in a number of cases... We should make sure this is not a problem and do some measurements with stress tests.

For performance, I was thinking of switching from using array.forEach to using for loops (see the report this).

Yes, forEach should be avoided in hot paths.

[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/13699/merge/testResults/webgl1/index.html

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

[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/13699/merge/testResults/webgl2/index.html

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

[james-pre]

I believe the test fails have to do with _isDirty checks, can anyone confirm or disprove?

[Popov72]

It's hard to say, you should try debugging one of the simpler playgrounds that fails (e.g. https://playground.babylonjs.com/#484RHA#0).

Spector would probably help you compare the inputs in each case.

[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/13699/merge/testResults/webgl1/index.html

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

[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/13699/merge/testResults/webgl2/index.html

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

[james-pre]

@Popov72 @sebavan I did some diffing and found the only difference between the reference and PR scenes were two unique IDs.
diff.txt (you may want to change the file type to .diff so you can see the diff with syntax highlighting)

To repoduce:

• export https://playground.babylonjs.com/?snapshot=refs/pull/13699/merge#484RHA#0 as pull.babylon
• export https://playground.babylonjs.com/#484RHA#0 as original.babylon
• format both .babylon files using VS Code's JSON language features
• git diff --no-index original.babylon pull.babylon

I'm not sure what is causing the rendering issues. Perhaps someone with more in-depth knowledge of the rendering can take a look?

[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/13699/merge/testResults/webgl1/index.html

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

[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/13699/merge/testResults/webgl2/index.html

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

[Popov72]

@dr-vortex We are currently very busy with the new version coming out this Thursday, but we will come back to this PR as soon as possible!

[james-pre]

@Popov72 No pressure on getting this PR merged immediately, we have time.

[sebavan]

@dr-vortex before we are coming back to this PR, it would be great to have the perf impact analysis of the PR ?

I can not make things slower and it would be great to prove out it has 0 negative impact.

[james-pre]

Sure @sebavan, though I'd like to get it passing the tests before I look at the performance impacts.

Just a thought, it may be beneficial to use array access instead of the getters internally (e.g. vector.vector[0] instead of vector.x) since direct access is almost 4x faster than using getters.

[sebavan]

@dr-vortex regarding the tests, there is nothing obvious, you will need to troubleshoot manually :-(

Regarding perfs, the checks would need to be done cross plat (V8/Webkit/SpiderMonkey/Hermes...) as they all have different optimizations paths and I really wonder if it will be able to be at least as fast so I do not want you to waste time if we can not reach the same perfs.

As a good example https://github.com/BabylonJS/Babylon.js/pull/13742/files where factorizing the code was breaking optims pretty badly for no apparent reasons :-)

[Popov72]

I wanted to try your code locally, but I have a problem with Typescript because Vector3.FromArray(array) returns a Vector type, not Vector3.

For reference, the implementation of Vector.FromArray is:

public static FromArray<T extends Vector>(this: VectorConstructor<T>, array: DeepImmutable<ArrayLike<number>>, offset: number = 0): T {
    const ref = new this();
    this.FromArrayToRef(array, offset, ref);
    return ref;
}

Doing Vector3.FromArray<Vector3>(array) does work, but it is a breaking change...

The type T should be deduced from the class on which FromArray is called (Vector3 when doing Vector3.FromArray), but I don't know if this is possible... Also, I'm not sure that new this() is able to create an object of type T... In a static function, this refers to the class in which the static function is defined, so here it is always Vector, even if you do Vector3.FromArray.

cc @sebavan in case he has a solution, as I am far from an expert in Typescript.

If there is no solution, FromArray (and all static functions that shares the same problem) would have to be duplicated in all Vector2/3/4 classes.

[james-pre]

@Popov72 FromArray was been giving a lot of trouble.

In a static function, this refers to the class in which the static function is defined

this in a static context should refer to the constructing class. For example, in Vector3.FromArray, this would be Vector3 even if FromArray is inherited from Vector. Unfortunately, TypeScript does not support static this typings properly (see microsoft/TypeScript#5863).

If there is no solution, FromArray (and all static functions that shares the same problem) would have to be duplicated in all Vector2/3/4 classes.

I think this is the best solution for now. Maybe if static this typings are fixed we can revisit this in the future.

[RaananW]

TypeScript does not support static this typings properly

This is by design. Static methods belong to an object and are not inherited. They might work, depending on the way the object was generated, but nonetheless - to have proper types they should be created on each one of the objects that need it.

I will pull this PR later today or tomorrow and fully review it. It looks great! just need to make sure it doesn't break anything.

[james-pre]

I love the overall idea, let's move it as a draft until the 6.0 version will be released.

@sebavan Since 6.0 is out I'm going to mark it as read for review (so reviews can be done), though It would probably be best to wait until the lighting issue is fixed before merging.

just need to make sure it doesn't break anything.

@RaananW It currently breaks some lighting for unknown reasons (I think it is related to _isDirty but can't be sure).

[Popov72]

Before merging, it is essential to ensure the following:

Regarding perfs, the checks would need to be done cross plat (V8/Webkit/SpiderMonkey/Hermes...) as they all have different optimizations paths and I really wonder if it will be able to be at least as fast so I do not want you to waste time if we can not reach the same perfs.

As a good example https://github.com/BabylonJS/Babylon.js/pull/13742/files where factorizing the code was breaking optims pretty badly for no apparent reasons :-)

(note that @sebavan is currently on vacation)

[james-pre]

Note: Made changes to new branch (branched from a recent commit), discarded old commits, and force pushed.

[james-pre]

@sebavan @RaananW @Popov72

Would you all mind running performance tests for the new approach?

I am also considering adding a standardized access method (e.g. numbered indexes). It would not be where the data is stored, just an accessory (like how x, y, and z are accessors on Vector3). Doing so would allow standardized unit tests which would be really nice to have. What do you think?

In addition, I would like to mention that this approach changes some return values for ColorN.operationToRef from the original Color to the reference/result. This change was made so Colors are compatible with Vector and it is breaking. Since we are usually against breaking changes, would you be open to making this one for compatibility? If not, how would you fix compatibility between Vectors and Colors?

[sebavan]

Everything where you are now calling ...toRef vs doing the same inlined code might be a bit slower in some browser. It was actually not doing it before by design.

I do not know if color and vector needs to be that close. (they could but I am wondering if they need to, for instance, what does lenght represent for a color ?

[james-pre]

@sebavan

Thank you for the insight.

Everything where you are now calling ...toRef vs doing the same inlined code might be a bit slower in some browser. It was actually not doing it before by design.

I had thought of that. I will fix using ...toRef throughout the classes, though I will not change some of its usage from existing code where it shouldn't be (e.g. with normalization).

I do not know if color and vector needs to be that close. (they could but I am wondering if they need to, for instance, what does lenght represent for a color ?

I've changed the colors to implement VectorLike, which is now what Vector is based on. VectorLike includes most of the operations and utilities. Vector adds various normalization, length, and misc. static methods.

I am also considering adding a standardized access method (e.g. numbered indexes). It would not be where the data is stored, just an accessory (like how x, y, and z are accessors on Vector3). Doing so would allow standardized unit tests which would be really nice to have. What do you think?

Please ignore my earlier comment concerning property access. For tests, asArray can be used.

[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/13699/merge/testResults/webgl1/index.html

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

[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/13699/merge/testResults/webgl1/index.html

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

[james-pre]

I'm closing this PR since having it as well as #14201 would create some conflicts. I will include Vector in the PR for #14201.