Add polar and spherical coordinate system support

[james-pre]

Also fixes a misspelling with the Vector3.GetAngleBetweenVectors jsdoc parameters

[azure-pipelines]

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.

[azure-pipelines]

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.

[BabylonJSGuide]

Returning polar coordinates as a Vector2 is confusing. If Polar was to be implemented it should have its own class with properties Polar.radius, Polar.angle and methods Polar.toVector2, Polar.fromVector2. However since polar coordinates can be obtained simply from a Vector 2, V, using

radius = V.length();
angle = Math.sign(V.x) * Math.acos(V.x / radius) for -π ≤ angle ≤ π and add π if you want 0 ≤ angle ≤ 2π

the question is whether it is worth doing.

[BabylonJSGuide]

Again returning polar (actually spherical) coordinates as a Vector3 is confusing and would require a new class Spherical with properties radius, theta, phi where theta is the inclination and phi the azimuth together with appropriate methods.

Obtaining spherical from a Vector3 V could be achieved by

radius = V.length();
theta = Math.acos(V.z / radius) 0 ≤ theta ≤ π

getPhi would need a function

const getPhi = (V) => {
    if (V.x === 0) {
        if (V.y > 0) {
            return Math.PI / 2
        }
        else if (V.y < 0) {
            return -Math.PI / 2;
        }
        else return NaN;
    }
    if (V.x > 0 ) {
        return Math.atan(V.y / V.x)
    }
    if (V.y >= 0) {
            return Math.atan(V.y / V.x) + Math.PI;
    }
    else {
        return Math.atan(V.y / V.x) - Math.PI
    }
}

Whether implementing or not there is no need need for the extra and confusing getRotationBetweenVectors

[azure-pipelines]

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.

[james-pre]

@BabylonJSGuide,

I've updated the code to have separate classes for both coordinate systems

[azure-pipelines]

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.

[BabylonJSGuide]

As can been seen the use of 'GetAngleBetweenVectors' and 'GetRotationBetweenVectors' or is it 'RotationBetweenVectors' or are they the same or different, whether static or a method , over complicates the coding and are unnecessary for Polar or Spherical coordinates.

Confusion over static or method was mine.

toSpherical could be achieved by including a, probably smarter version of the getPhi, function above simplifying the code.

[azure-pipelines]

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.

[azure-pipelines]

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.

[james-pre]

@BabylonJSGuide and @Popov72,

Thank you both for all the feedback. I've added toRef functions for all the related functions.

As for the (so far) controversial RotationBetweenVectors, the function is very useful for determining the amount of rotation on all axes between to Vector3s. Vector3.GetAngleBetweenVectors doesn't do this, it instead finds only 1 angle relative to a plane, which complicates things on the user's end. RotationBetweenVectors is designed as an easy way for developers to get the full rotation without doing 3D geometry.

As for the value of phi, RotationBetweenVectors returns the correct values in all cases I've tried. The function does not convert co-terminal angles back to their 0 < θ < 2π form and treats 0/0 as 0 not NaN, which is intended.

[BabylonJSGuide]

the cone would face the target vector

Do not understand what this means. What part of the cone would face the target? The idea of using a cone is that the tip of the cone indicates what is pointing to the target. Are you saying that the target can never be on the y axis?

Imagine the cone as some sort of spaceship with the tip pointing forward. So in the lower image the fin is vertical and the spaceship is moving left, the diagonal white line gives the direction for the second vector. Is this a possible scenario?

Using the tip of the cone as that which must face the target please describe or produce an image of a possible scenario.

[BabylonJSGuide]

@dr-vortex I will not be able to add an accepting review until I understand the purpose of your function 'PitchYawRollForDirectionChangeToRef' and that it works as you expect. In the following PG I have set up the asym cone so that the tip of the cone is in the forward position and the fin is up and by baking the transformation it currently has rotation zero.

https://playground.babylonjs.com/#QYA3JC#2

Using the tip and fin in your description please describe the purpose of your function and demonstrate how it works and that it does work as expected by adding it to the PG and applying it to the asym cone.

[james-pre]

Experimenting with the function, the theta function atan2(z, x) should be atan2(x, z), and we can use atan2 instead of asin for phi. I've made this PG that demonstrates the function with an animation.

[azure-pipelines]

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.

[BabylonJSGuide]

Experimenting with the function

For an acceptable PR a function should be beyond experimenting.

Using the tip and fin in your description please describe the purpose of your function.

Please include the description as above. Is the intention it only works with paths in the XZ plane?

For 3D paths is the following behaving as expected https://playground.babylonjs.com/#QYA3JC#6 ?

[james-pre]

No, the function should work without a plane. The animation just sets the rotation at every increment of the frame rate (-some) to the rotation between the last node and the current node in the path. I believe the issue with the cone not facing in the direction of travel is an oversight of the animation, not the function.

[BabylonJSGuide]

The Polar class is working https://playground.babylonjs.com/#B1WXN8
With the change required above Spherical is working https://playground.babylonjs.com/#ML0BEE#1

@dr-vortex my suggestion is that for the moment your PR is only for math.polar.ts and any changes to Vector2 and Vector3 in the math.vector.ts are removed from the PR which I could then approve. Once you know for sure your functions and animations are working as you require then submit a PR with with the working functions.

@sebavan will know more but in terms of the build failures then you need to look at https://doc.babylonjs.com/divingDeeper/developWithBjs/howToStart and check your local build. I think the common failures are not putting an entry into 'whats new', not commenting on parameters and not having the required format such as extra space, extra lines etc, you can check this with (I think)

npm run format:fix

as per doc page above.

[sebavan]

Thanks both for all the love you provide for this feature !!!

Yup, about the build, the format fix will help a lot, and the rest of the issues are:

[azure-pipelines]

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.

[azure-pipelines]

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

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

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

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

Links to test babylon tools with this snapshot:

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

[james-pre]

@sebavan
What settings do you use for prettier? I'd like to format it correctly but don't know what settings to use.

[sebavan]

they are enclosed in the project but the simpler is to run

npm run format:fix

[sebavan]

@RaananW can you think of smthg else to help @dr-vortex with the formatting ?

[james-pre]

Hm...

I cloned my fork, then did npm run format:fix which pointed out that some lets should be consts, I fixed that, then ran it again along with format document (Ctrl + Shift + I in VS Code). No changes or errors the second time.

[james-pre]

@sebavan and @BabylonJSGuide,

Ready to merge, finally! Thank you both for all the help and feedback! I will start a draft PR and work on some operator functions (add, subtract, etc.) for Polar and Spherical.

[RaananW]

@RaananW can you think of smthg else to help @dr-vortex with the formatting ?

the best option is npm run format:fix or the prettify extension for your favorite IDE