Regression: TBN Vector Fixes break writing sdf shader normals

[basicer]

Tested versions

• Reproducible in: master (v4.4.dev.custom_build [0f95e9f])
• Not Reproducible in: 4.4dev7, master with Scene Shaders - TBN Vector Fixes #100441 reverted

System information

Windows 11 Vulkan 1.3.280 - Forward+ - Using Device #0: NVIDIA - NVIDIA GeForce RTX 3070 Ti

Issue description

The changes in #100441 break writing the normal in spatial shaders such that SDF raymarching doesn't work.

Steps to reproduce

Open the mrp project, a simple SDF raymarching example.

After #100441 :

Before:

Minimal reproduction project (MRP)

sdf.zip

[basicer]

For more correct shadows you can add LIGHT_VERTEX = (VIEW_MATRIX * vec4(world, 1.f)).xyz; to the end of the fragment shader, thought this doesn't help with the regression.

[Flarkk]

I have the impression that the shader built-in NORMAL is unnormalized after #100441.

Does using normalize(NORMAL) solves the problem ?

If it does I think it's better that NORMAL is provided normalized to user fragment shaders even though it's not fully compliant with Mikktspace

[StaydMcMuffin]

I am strongly of the opinion we should not compromise on the correctness of our tangent basis construction, especially when doing things the correct way means maintaining parity with the result users get out of Blender.

If this is simply a matter of NORMAL needing to be normalized for your shader to work, then we should make it clear to users that the TBN vectors are unnormalized by default, and that they need to be normalized manually if they're being used for things other than the tangent-basis transform.

[basicer]

Normalizing the normal doesn't seem to help. Here's the full shader for reference, with the normalize added.

shader_type spatial;
render_mode cull_back;

// Raymarching constants
const float SURFACE_DST = 0.01f;
const int MAX_STEPS = 64;
const float MAX_DISTANCE = 256.f;
const float NORMAL_STEP = .001f;

varying float time;

void vertex() {
	time = TIME;
}

// Rotation matrix from angle
mat3 rotateY(float theta) {
    float c = cos(theta);
    float s = sin(theta);
    return mat3(
        vec3(c, 0, s),
        vec3(0, 1, 0),
        vec3(-s, 0, c)
    );
}

// Torus signed distance field function
float SDFTorus(vec3 p, float innerRadius, float radius) {
	vec2 q = vec2(length(p.xz) - radius, p.y);
	return length(q) - innerRadius;
}

// Sphere signed distance field function
float SDFSphere(vec3 p, float radius) {
	return length(p) - radius;
}

// Box signed distance field function
float SDFBox(vec3 p, vec3 size) {
	vec3 d = abs(p) - size;
	return min(max(d.x,max(d.y,d.z)),0.0) + length(max(d,0.0));
}

// Sample the whole scene, you can mix multiple shapes here
float Scene(vec3 pos) {
	// Torus
	float dst = SDFTorus(pos, 0.15f, 4.f);
	float sp = 2.5;
	// Rotating spheres
	vec3 spOff1 = vec3(cos(time/sp * 3.1415), 0.f, sin(time/sp * 3.1415)) * 4.f;
	vec3 spOff2 = vec3(cos((time/sp + 1.f) * 3.1415), 0.f, sin((time/sp + 1.f) * 3.1415)) * 4.f;
	dst = min(dst, SDFSphere(pos + spOff1, 0.5f));
	dst = min(dst, SDFSphere(pos + spOff2, 1.f));


	return dst;
}

// The actual raymarching function
float RayMarch(vec3 ro, vec3 rd) {
	float d = 0.f;
	for (int i = 0; i < MAX_STEPS; i++) {
		vec3 pos = ro + rd * d;
		float sceneDst = Scene(pos);

		d += sceneDst;

		// Only stop if distance is higher than MAX_DISTANCE or sampled distance
		// is less that surface threshold
		if (d > MAX_DISTANCE || abs(sceneDst) <= SURFACE_DST) break;
	}
	return d;
}

// Sample the world normal in the contact position
vec3 Normal(vec3 pos) {
	float d = Scene(pos);
	vec2 e = vec2(NORMAL_STEP, 0.0);
	vec3 n = d - vec3(
		Scene(pos - e.xyy),
		Scene(pos - e.yxy),
		Scene(pos - e.yyx));
	return normalize(n);
}


void fragment() {
	// Get the pixel world coordinates
	vec3 world = (INV_VIEW_MATRIX * vec4(VERTEX, 1.0)).xyz;
	// Get the camera position
	vec3 camera = INV_VIEW_MATRIX[3].xyz;

	// Raymarching direction
	vec3 dir = normalize(world - camera);

	vec3 ro = camera;
	vec3 rd = dir;

	// Raymarch
	float d = RayMarch(camera, dir);
	// Set the world position from the raymarching output
	world = camera + dir * d;

	if (d >= MAX_DISTANCE) discard;

	// Samples world normal
	vec3 n = Normal(world);
	// Set the local normal relative to the view
	NORMAL = normalize((VIEW_MATRIX * vec4(n, 0.f)).xyz);

	// Depth calculation, this makes possible for other meshes intersect
	// properly with the geometry
	vec4 ndc = PROJECTION_MATRIX * VIEW_MATRIX * vec4(world, 1.f);
	float depth = (ndc.z / ndc.w);
	DEPTH = depth;
	LIGHT_VERTEX = (VIEW_MATRIX * vec4(world, 1.f)).xyz;
}

[Flarkk]

@StaydMcMuffin I fully get your perspective. I'm just concerned that a breaking change for many users' shaders may have sneaked in with #100441.

It's absolutely clear that we need the correct Mikktspace tbn vectors in the glsl scene shader. When it comes to what's passed to the Godot shading language, there might be non-breaking alternatives.

We could for instance expose the unnormalized tbn matrix via a new mat3 TBN_MATRIX builtin, and keep the individual vectors normalized. This would guarantee correctness of users's tangent-basis transforms regarding Mikktspace without breaking compatibility for other usages of NORMAL, TANGENT and BINORMAL.

But there is a chance that the issue is somewhere else according to @basicer. Haven't got the time to take a look at #100441 in detail yet.

[StaydMcMuffin]

Turned out to be a plain error on my part, nothing to do with normalization. When not using a normal map, the interpolated vertex normal was always used for shading instead of the one supplied by your shader. I've addressed this in #100859.