Bug in code of Vector3

Next if must be the opposite to make any logic in dividing:

if (WithinEpsilon(num, 1.0))

In this Vector3`s method, used by other public methods:

    public static _UnprojectFromInvertedMatrixToRef<T extends Vector3>(source: DeepImmutable<Vector3>, matrix: DeepImmutable<Matrix>, result: T): T {
        Vector3.TransformCoordinatesToRef(source, matrix, result);
        const m = matrix.m;
        const num = source._x * m[3] + source._y * m[7] + source._z * m[11] + m[15];
        if (WithinEpsilon(num, 1.0)) {
            result.scaleInPlace(1.0 / num);
        }
        return result;
    }

This code is from here:

But here is an example that uses this transformation, and now it works correctly o_O

I need help to understand why

Example restore 3d point from depthrenderer
It uses Vector3.Unproject which at some point uses Vector3._UnprojectFromInvertedMatrixToRef

Also, it works only with non-linear z, but 3d point restoration has only linear operations - what magic happens here?

1 Like

Sorry I must be dense today but can you rephrase your question?

Yeah, if next

if (WithinEpsilon(num, 1.0))

is true there is no logic to divide by num, but in the code division present.

There is check if num =~ 1.0 and then there are division by num.

Logic must be opposite, we need division only if num is NOT 1. And I assume that it is about division by W to make it real 3d point (so now there are actually no division by W in all cases except when W == 1).

This is the first part.

The second part is about - with this wrong logic still 3d point reconstruction works as expected, I provided example. That is why I am not sure what to do here. It can mean that all the code is expecting incorrect behavior and fix can reproduce the problems, so complex fix is probably needed but I am not sure I can knew all details to make it.

The third part is about what that functions work with. Vector3.unproject takes as input linear screen X,Y in range [0, screenSize] in pixels but Z in non-linear format in range [0-1]. Then I do not see how z is transformed in lenear real z coordinate, so I am confused how it works.

Hum..Not sure if I share your opinion. So here is how it works:

What we want is to take apoint in screen space or normalized device coordinates and send it back into 3D world space using an inverted matrix.

1. Transform the source point using the matrix:

Vector3.TransformCoordinatesToRef(source, matrix, result);

This one is obvious.

2. Calculate the W component (homogeneous divide):

const m = matrix.m;
const num = source._x * m[3] + source._y * m[7] + source._z * m[11] + m[15];

This computes the “W” value after transformation — essential in homogeneous coordinates. We’re essentially computing:

w = x * m[3] + y * m[7] + z * m[11] + m[15]

It’s the 4th component of the result before dividing by w (to go from 4D back to 3D).

3. Fix perspective distortion (if needed):

if (WithinEpsilon(num, 1.0)) {
    result.scaleInPlace(1.0 / num);
}

If num (i.e., w) isn’t 1, scale the result by 1 / w — the classic homogeneous divide — but only if it’s not already close enough to 1.

Does it make sense?

Lets focus on

if (WithinEpsilon(num, 1.0)) {
    result.scaleInPlace(1.0 / num);
}
export function WithinEpsilon(a: number, b: number, epsilon: number = 1.401298e-45): boolean {
    return Math.abs(a - b) <= epsilon;
}
  1. num = 1
WithinEpsilon(1, 1) = Math.abs(1 - 1) <= epsilon; 
WithinEpsilon(1, 1) === true

if (WithinEpsilon(1.0, 1.0)) {
    result.scaleInPlace(1.0 / num);
}

So we will do result.scaleInPlace(1.0 / num); in case when num =~ 1

  1. num != 1
WithinEpsilon(2, 1) = Math.abs(2 - 1) <= epsilon; 
WithinEpsilon(2,1) === false

if (WithinEpsilon(2.0, 1.0)) {
    result.scaleInPlace(1.0 / num);
}

if (false) {
    result.scaleInPlace(1.0 / num);
}

In this case and all cases where num !=~ 1 we will NOT divide by W.

this is main problem

You mentioned
" If num (i.e., w ) isn’t 1, scale the result by 1 / w"

But in this function we do opposite - only if num 1 we divide, if num not 1 we do nothing.

1 Like

I vote to remove the if (WithinEpsilon(num, 1.0)), as it’s probably slower to do the check than to do the division in all cases (function call + Math.abs(a - b) <= epsilon).

We need to divide only if we are CLOSE to 1. like 0.995 or 1.00005

Edit: Scratch that, I realized the epsilon is not set up so it is infinitesimal

I thought this code was homogeneous division, but that is already done in the Vector3.TransformCoordinatesToRef(source, matrix, result); call:

    public static _UnprojectFromInvertedMatrixToRef<T extends Vector3>(source: DeepImmutable<Vector3>, matrix: DeepImmutable<Matrix>, result: T): T {
        Vector3.TransformCoordinatesToRef(source, matrix, result);
        const m = matrix.m;
        const num = source._x * m[3] + source._y * m[7] + source._z * m[11] + m[15];
        if (WithinEpsilon(num, 1.0)) {
            result.scaleInPlace(1.0 / num);
        }
        return result;
    }

So, I don’t understand the purpose of this code?

In any case, it doesn’t do anything, because with the default epsilon value (1.401298e-45), we execute the scaleInPlace call only if num >= 1 - 1.401298e-45 and num <= 1 + 1.401298e-45:
image

I have to admit without a define epsilon, this is useless…And as @Evgeni_Popov said, the divide by w is already done by the TransformCoordinatesToRef

Here we are gentlemen:
remove unnecessary code by deltakosh · Pull Request #16834 · BabylonJS/Babylon.js

1 Like

Just for fun, this code was at least introduced 6 years ago:

And probably even before but as we moved code around we lost track of it

The year is 2014 (yes 11 years ago) and the code was ALREADY there lol
Port to typescript launched - expect some crashs :slight_smile: · BabylonJS/Babylon.js@c6de9fc

So I can safely assumed this was ported from the JS version and most certainly from the Silverlight version.

But if we look closely this was not exactly the same:

I don’t think that was useful either. I really believe we have that bug since forever :smiley:

Nice catch @forux

3 Likes