Types in math.vector.ts

Questions
1

Hello babylon.js!

I have a few questions currently that are kind of intertwined. But the all rise from the fact that for my project we are building an API on top of babylon.js. As part of this API we have basic math types like
interface IVector3 { x, y, z }
We now also want to re-export Babylons math types to make it easier for users to do calculations, but as part of this I want to add some new helper stuff, mainly toIVector3() and fromIVector3() etc. I tried to do this by extending the classes available in babylon but this gave me some annoying things related to typescript and types. for example, all methods in math.vector.ts that returns the object itself does this using the Vector3 etc type rather than this type. This means that I canā€™t do things like
const ivec3 = new MyVector3(1,2,3).normalize().toIVector3()
Since the normalize function returned a Vector3 (even though its performed in place).
This can be solved by changing the type returned by normalize() to this rather than Vector3.

So now I want to know if Iā€™m missing some cavity with doing this type change? If not, is this something that the project is interested in me contributing with?

Iā€™ve also noticed that there seems to be some inconsistencies with the returned value for ā€¦ToRef() functions where some return void, some return this (aka object you called the function on), and some return the ref object.
I understand that fixing this is a breaking change, but is there an interest in normalizing this as well? this is something I would like to take on if so. Got bitten at least once from getting another value than I expected as the return.

Hope for some great discussion and insights on this topic

2

Hello and welcome to the Babylon community! Iā€™ll add @RaananW to the discussion

3

Itā€™s not a fluent API by design. I canā€™t imagine it would be changed to not be - what about making your own builder that wraps babylon types that does return this? Have you seen the ā€œinPlaceā€ operations they might work, although some are missing.

1 Like
4

We have never considered the need to extend our math classes outside of the math module, and we therefore never saw the need to change the return type to this :slight_smile:

The API has grown over time, and only ā€œrecentlyā€ (i guess 1 or 2 versions back_ we have agreed about the consistency required in this class. And since we are backcompat, we still have a function like normalize, which is actually normalizeInPlace, as opposed to normalizeToRef.
Regarding the toRef - I donā€™t mind having all toRef return the reference if they donā€™t return anything. Moving from return voind to return a value is not a back-compat issue. The other way - yes.

I wouldnā€™t mind doing that, but TBH i would go with brianā€™s suggestion and create a wrapper around this class and not extend the class itself. Thatā€™s

5

In JS you can do Vector3.prototype.foo = function (f) {ā€¦}

To do the same thing in TS you can use mixins. But this requires that you merge the classes at runtime.

I chose to ā€˜extendā€™ vector 3 using a static utility class. This approach makes the code bigger and less legible but less klugey.

2 Likes
6

Thanks for the quick responses :slight_smile:
Yes, I know of the inPlace versions, inPlace sadly has the same issue as i pointed out with the normalize, the type get downgraded to Vector3 rather than my extended type.

@RaananW so if I understand your answer, a 6.0 breaking change to make math functions consistent is not on the table?
I also totally get that my needs are quite unorthodox and thatā€™s why I want to check here if there is even an interest for extending the API with this functionality. Iā€™ve been playing around with it a bit and the syntax in methods gets a bit wonky but should otherwise have no back-compat issues AFAIK.

public add(otherVector: DeepImmutable<Vector2>): this {
    return new (<any> this.constructor)(this.x + otherVector.x, this.y + otherVector.y);
}

Also just to explain my use case a bit more, we are building an API around babylon that our users build 3D apps on. this has lead us to run Babylon through a fork which is currently running stock 5.23. Our goal is to contribute as much as we can but if this doesnā€™t fit with the project I can fix this on my side as a last resort. I would be happy to do this and make a PR though if there is interest. (or for parts of my changes)

Mixins sounds like an interesting concept, will have to check more in to that! thanks (though i suspect that I will run in to similar issues when trying to do method chaining)

Thanks again for the answers!

1 Like
7

Nope, not on the table :slight_smile:
The team maintains back-compat between major versions, but we do deprecate older functions and possibly remove them when releasing a major version.

I donā€™t think it is unorthodox, i think you are trying to get more consistency for your users, and I totally get it.

I donā€™t think changing the return type from Vector3 to this is a back-compat issue, and I donā€™t mind doing that TBH. If you want to submit a PR for us to review, please go right ahead!

2 Likes
8

Then I will go ahead and make a PR with the changes where they are be backcompat safe!
Thanks again for the input :slight_smile:

2 Likes
9

Iā€™ve been bitten by this a few times in my own side projects. I did not expect ToRef functions to return the original object. Thatā€™s just wrong in my opinion. I almost see fixing this as a bug fix as I canā€™t imagine someone wanting to continue with the original object. Are there legitimate uses for ToRef functions returning the original object?

10

I completely agree it would be a bug fix if it does not return the ref :slight_smile:

11

The issue for me is that almost all of the ToRef functions return this and not the result. I didnā€™t bring this up before because almost all of the ToRef functions will need to change.

1 Like
12

Hi, thanks for chipping in on this subject!

Yeah its quite a few places that would be changed, I would be more than happy to make that part of my PR, and then maybe we could drive the discussion from that PR?

13

Waiting for the PR :slight_smile:

14

PR is up, but it failed on some tests so will look at that :slight_smile:

15

Not an expert but I presume that this is not recognized as a type.

For example your change from

public toArray(array: FloatArray, index: number = 0): Vector2 {

to

public toArray(array: FloatArray, index: number = 0): this {

You are still returning a Vector2 so it should remain as a Vector2 even though you are returning it in the form of this (a Vector2)

Same where you have changed other types to this rather than a named type.

16

@JohnK
The this type is a feature of typescript related to polymorphism. It lets a function return the type of a deriving class rather then the type of the base class. A very handy tool when writing fluent APIs. you can read more about the feature here: this Types

3 Likes
17

OK thank you for the info, you learn something new everyday

2 Likes