Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fragment & Texture Normals are wrong #169

Closed
TokisanGames opened this issue Jul 30, 2023 · 3 comments · Fixed by #170
Closed

Fragment & Texture Normals are wrong #169

TokisanGames opened this issue Jul 30, 2023 · 3 comments · Fixed by #170
Assignees
Labels
bug Something isn't working
Milestone

Comments

@TokisanGames
Copy link
Owner

TokisanGames commented Jul 30, 2023

Update

TLDR:

  • Terrain binormals/tangents were wrong, being calculated off of viewspace instead of worldspace.
  • We can't calculate terrain B/N/T in vertex() without creating an artifact on the lod0/1 border. The other differences are very subtle; a very slight reshaping of shadows on steep inclines that I can't say is better, just different and irrelevant.
  • TextureGrad and Texture can be interchanged with identical results, as shown above in update 2. Through some casual testing, Texture might be up to 4% faster. Not the problem here. Saving for Shader optimizations #26.
  • Rotation is part of the problem. Even when set to 0 it rotates the textures 90 with incorrect normals. Removing.
  • Normalizing texture normals is the remainder of the problem. Removing.

All fixed in #170.


A cube with the standard material compared to the terrain a few cm lower. Top perspective.

image

Side angle comparing the same rock

image

On the standard material the normals on this rock do not respond to camera angle. The light is always the same. On the terrain, the light subtly changes based upon camera angle. The amount and coverage of the light should be identical between the left and right images, as it is for the cube. Every face should be identically lit.

image

What I found is that if I used a basic terrain shader:

fragment() 
{
	float scale=.2;
	vec4 alb = texture(texture_array_albedo, vec3(UV*scale, 11));
	ALBEDO = alb.rgb;
	vec4 nrm = texture(texture_array_normal, vec3(UV*scale, 11));
	NORMAL_MAP = nrm.rgb;
	ROUGHNESS = nrm.a;
}

No fragment normals. and in vertex make sure tangent and binormal are calculated

vertex() {
	NORMAL = vec3(0, 1, 0);
	TANGENT = cross(NORMAL, vec3(0, 0, 1));
	BINORMAL = cross(NORMAL, TANGENT);
}

Then we get the proper look:
image

image

So it appears there's a problem with our terrain tangents and binormals in fragment, which we do need.

Also we can provide the option to use height as AO for free.

Update 2

I've further found that binormal and tangent can be calculated in fragment, however they must do so before being converted to view space.

fragment()
	vec3 out_tangent, out_binormal;
	vec3 normal = get_normal(UV2, out_tangent, out_binormal);
	NORMAL = mat3(VIEW_MATRIX) * normal;
	TANGENT = mat3(VIEW_MATRIX) * out_tangent;
	BINORMAL = mat3(VIEW_MATRIX) * out_binormal;

And this now gives us proper looking vertex and fragment normals using a terrain covered with lookups from texture()

image

However, the current shader looks up with texturegrad() plus a weighting for texture blending. Using the above n/t/b calculations, the normals are more correct, but they're a bit flat. I can double the NORMAL_MAP_DEPTH but it's not a perfect match.

image

image

image

Update 3

I've found that these two produce identical results, and that normalizing the normals which fixed an artifact before, is incorrect.

	float scale=.2;

	vec2 matUV = UV * scale;
	vec2 ddx = dFdx(matUV);
	vec2 ddy = dFdy(matUV);
	vec4 albd = vec4(1.0);
	vec4 norm = vec4(0.5);
	albd = textureGrad(texture_array_albedo, vec3(matUV, 11), ddx, ddy);
	norm = textureGrad(texture_array_normal, vec3(matUV, 11), ddx, ddy);
//	norm.rgb = normalize(norm.rgb);
	NORMAL_MAP = norm.rgb;
	ALBEDO = albd.rgb;
	ROUGHNESS = norm.a;
	vec4 alb = texture(texture_array_albedo, vec3(UV*scale, 11));
	ALBEDO = alb.rgb;
	vec4 nrm = texture(texture_array_normal, vec3(UV*scale, 11));
	NORMAL_MAP = nrm.rgb;
	ROUGHNESS = nrm.a;

In get_material the normals are normalized, and as mentioned, fixed an artifact before but kills normals. You can see it greatly weakens the normal map. It's clear at the bottom of the hill.

image
image

image

We can remove the normalization, but that will allow the previous artifact to surface. Perhaps we can normalize later after the weighting is applied.

@TokisanGames TokisanGames added the bug Something isn't working label Jul 30, 2023
@TokisanGames TokisanGames added this to the Beta milestone Jul 30, 2023
@TokisanGames TokisanGames changed the title Texture normals are wrong Fragment normals are wrong Jul 30, 2023
@TokisanGames TokisanGames changed the title Fragment normals are wrong Fragment & Texture Normals are wrong Jul 30, 2023
@outobugi
Copy link
Collaborator

We can change textruegrad() to texture(). The derivative stuff is useless. It's a leftover from projection stuff which actually might not even need that after all. Changing to texture() might also give a slight fps boost idk.

@TokisanGames
Copy link
Owner Author

TokisanGames commented Jul 30, 2023

See update 3. The problem isn't texturegrad as I've found the equivalent texture setting. The problem is normalizing the normal before the weighting function causes this normal problem.

The texture normals are wrong (weaker) if normalized, so shouldn't be. However if they aren't normalized in get_material, it produces artifacts. So I'm looking at taking the normal magnitude, normalizing for the weighting, then reapplying the magnitude. Initial testing looks good, but I haven't determined the right spot for reapplication for all materials.

Although, I'm in favor of getting rid of all of the derivative stuff so the shader is easier to understand. Good to know it is useless. I thought it was important. We don't need it for 3d projection?

@TokisanGames TokisanGames self-assigned this Jul 31, 2023
@TokisanGames TokisanGames mentioned this issue Jul 31, 2023
19 tasks
@TokisanGames
Copy link
Owner Author

TokisanGames commented Jul 31, 2023

Texture and texturegrad produce identical results, shown in the code in update 2, so that's not a cause of the fragment normal problem. I tested full terrain texture lookups and get 600-730fps vs texturegrad lookups 580-700, so it might be up to 4% faster. TextureGrad might be needed for 3D projection in #65. But if not, it can be removed in #26

Rotation breaks the normals, as noted in #145. Strangely the half of my textures (rocks/cobblestones) look correct, the other half (grasses) are not. The grasses should not be dark. This is not caused by wrong DirectX/OpenGL normals.

image

I reviewed Zylann's rotation code and ours is different. When I correct it, texture normals are now correct when rotation=0. However when you increase rotation it destroys normals. We could only rotate albedo and not normals. That looks much better at certain light angles, but it's clearly not correct. Aside from not rotating normals, I can't get rotations looking good at all. Removing for now. It could be added again as part of 3d projection since that has a rotation function.

image

Normazling texture normals is also incorrect. It was added here. It must have appeared to fix something back then, but clearly created other problems. Here it is normalized and not. The standard material does not normalize texture normals.

image


Here's a summary of what I've found:

  • Terrain binormals/tangents were wrong, being calculated off of viewspace instead of worldspace.
  • We can't calculate terrain B/N/T in vertex() without creating an artifact on the lod0/1 border. The other differences are very subtle; a very slight reshaping of shadows on steep inclines that I can't say is better, just different and irrelevant.
  • TextureGrad and Texture can be interchanged with identical results, as shown above in update 2. Through some casual testing, Texture might be up to 4% faster. Not the problem here. Saving for Shader optimizations #26.
  • Rotation is part of the problem. Even when set to 0 it rotates the textures 90 with incorrect normals. Removing.
  • Normalizing texture normals is the remainder of the problem. Removing.

All fixed in #170.

@outobugi If you can fix rotation now, fine. Otherwise lets remove it until say 3D projection is enabled, which has it's own rotation. And I want to consider adding paintable rotation (good for cobblestones, but that limited use may not be worth it) and detiling (#145 )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants