Prevent division by zero and warn about invalid normal/tangent information

[RevoluPowered]

This was reported by UBSAN.

Many methods were discussed, in the end this has the least evils and will use a 0,0,1 default on decompress e.g. non undefined behaviour.

Follow up PR will fix the source of these normals:
godotengine/godot-proposals#3151

[akien-mga]

CC @The-O-King

[RevoluPowered]

Everyone wants:

• a warning for 0,0,0's which is fine but is not the real problem.

I didn't explain clearly, the code will consistently generate NaN's from a 0,0,0 input which is a bug IMHO, and will result in undefined behavior.

The real issue / problem that is not clearly stated (div by 0) is always consistent with 0,0,0 input as I found in testing, sorry if I didn't make this clear in the PR. (every other value was fine :) )
https://github.com/godotengine/godot/blob/29019ebc83fbe269e05aa1865e5a272ffabefb46/servers/visual_server.cpp#L340

I'm open to someone suggesting how to remove the division by zero to make it support the 0,0,0 values and passing a 0,0,0 when it's part of the input.

Reproduce by ubsan=yes and reimport a model and open the scene.

[clayjohn]

Okay, so we have a problem with invalid input to the octahedral compression function, it outputs NaNs if the normal is Vector3(0, 0, 0). So at the very least we need to make sure that Vector3(0, 0, 0) makes it to the compression code.

The problem is that if our compression code returns Vector2(0, 0), it will decompress to vec3(0, 0, 1) in the decompression code. So we need to find a compressed value that decompresses to vec3(0, 0, 0).

Unfortunately, the octahedral compression function is made to work with unit-length values in the -1 to 1 range. I.e. there is no valid compressed Vector2 that decompresses to vec3(0, 0, 0).

This PR currently checks if the input to oct_to_norm is Vector2(0, 0), however, Vector2(0, 0) is a valid compressed value that decompresses to Vector3(0, 0,1).

Further, the results of oct_to_norm() are not guaranteed to be unit length, even a valid unit-length normal will may result in a non unit length output.

Accordingly, we need to do two things:

1. normalize the output of oct_to_norm() (this ensures that valid unit-length inputs result in the same output) (non unit-length inputs will result in unit-length outputs still)
2. fail when users try to compress Vector3(0,0,0) (my preference is to fail with a warning so users know that the values that make it to the GPU are bogus.

number 1 should likely be in another PR

number 2 would essentially look like adding the follow to norm_to_oct()

Vector2 VisualServer::norm_to_oct(const Vector3 v) {
        const float L1Norm = (Math::absf(v.x) + Math::absf(v.y) + Math::absf(v.z));
        if (Math::is_zero_approx(L1Norm) {
            WARN_PRINT_ONCE("Octahedral compression cannot be used to compress a zero-length vector, please use normalized normal values or disable octahedral compression")
             return Vector2();
        }
	const float invL1Norm = (1.0f) / L1Norm;

	Vector2 res;
	if (v.z < 0.0f) {
		res.x = (1.0f - Math::absf(v.y * invL1Norm)) * SGN(v.x);
		res.y = (1.0f - Math::absf(v.x * invL1Norm)) * SGN(v.y);
	} else {
		res.x = v.x * invL1Norm;
		res.y = v.y * invL1Norm;
	}
	return res;
}

Appendix
Let's try and compress Vector3(1, 2, 1)

norm_to_oct(Vector3(1, 2, 1)) returns Vector2(0.25, 0.5)

oct_to_norm(Vector2(0.25, 0.5)) returns Vector3(0.25, 0.5, 0.25)

The end result is a vector that has the same direction, but a different magnitude from our original. Meaning, if we normalized both values we would get the same Vector (Vector3(0.408, 0.816, 0.408))

Let's start from a normalized vector Vector3(0.408, 0.816, 0.408)

norm_to_oct(Vector3(0.408, 0.816, 0.408)) returns Vector2(0.25, 0.5)

Well that already looks familiar and we know how it will decompress.

[The-O-King]

I agree with what @clayjohn has said so far, and his math is correct, as I mentioned in comments before Vector2(0, 0) is a valid compressed value so we can't map Vector3(0, 0, 0) to it, and Vector3(0, 0, 0) simply doesn't work for this compression scheme

Lets normalize the vector returned by oct_to_norm(), and most likely also do that in the shader decompress code as well (scene.glsl)

The warning seems like the most reasonable solution to this problem for when Vector3(0, 0, 0) is passed in (as there's no way to get a Vector2 that could map back to it), I don't think the resulting NaN implies that we are "relying" on it, it's just the result of invalid input for the mesh anyways, I mean I guess we could set it to return some "default" normal like Vector2(0, 1) (which maps to Vector3(0, 1, 0)) or something but I don't think that makes the problem any better?

[fire]

What are the problems of returning an identity normal that is valid?

[clayjohn]

What are the problems of returning an identity normal that is valid?

It will result in bogus normals in the shader. In the case of mesh normals, there is no such thing as an "identity normal" or default normal. The function should return a default Vector2(), but it should warn the user that it has done so.

[RevoluPowered]

I believe we can get a representation of 0,0,0 in another way I'll make all the recommended changes but also add my idea onto the patch, it should make it tolerant to 0,0,0 normals :) without causing the issue with the compression or the division by zero.

should submit it tonight or tomorrow

[RevoluPowered]

Updated description of PR to include new notes and method used, please check the code it works nicely. 🤣 they should update / amend the paper to allow for this.

[clayjohn]

@RevoluPowered I'm not sure about abusing NaNs like this. Can we pass NaNs to the GPU and expect it to have consistent behaviour? I highly doubt it. Remember the oct_to_vec3() code in scene.glsl needs to be updated to decompress the normal as well.

edit: See the Vulkan Spec implementations are not required to support NaNs so we may get problems on certain devices. At any rate, I don't see the value in allowing users to use a 0,0,0 normal. 0,0,0 isn't a valid normal and will create NaNs later in the pipeline anyway.

[lawnjelly]

Catching up late on this, but yeah my tendency would be to agree with @clayjohn and not try to send NaNs. Afaik GPUs are required to not crash when encountering NaN, but their behaviour is undefined, so shouldn't be relied on. (I've heard of a few instances of people abusing NaNs targetting a particular platform, especially a console, but it sounds very dodgy for a general purpose engine).

Given the timeline @akien-mga says he hopes to make another beta before going on holiday, I would be in favour of fixing this bug with @clayjohn method first then researching a bit more into the NaNs, because having a divide by zero in the current branch does not seem good and should be closed asap imo.

[RevoluPowered]

tested now, it's fine :)

[akien-mga]

Thanks!