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

Normal artifact along region boundaries #185

Closed
TokisanGames opened this issue Aug 16, 2023 · 13 comments · Fixed by #353
Closed

Normal artifact along region boundaries #185

TokisanGames opened this issue Aug 16, 2023 · 13 comments · Fixed by #353
Labels
bug Something isn't working help wanted Help from other contributors desired
Milestone

Comments

@TokisanGames
Copy link
Owner

TokisanGames commented Aug 16, 2023

This artifact appears on regions borders. Along a region border make multiple heights at 40, 20, 0, -40 then start smoothing them together and it will appear. The thickness of this artifact appears to be 2 vertices, and is offset. So on X it goes from 0.5 to -1.5.

The artifact is in the normals. It occurs in the fragment() normal calculation. It has to do with using texture() with filter_linear, as it linearly blends the edge of the height map in it's normal calculation. If we switch to filter_nearest or texelfetch() it does resolve, however the normals are blocky.

Possible solutions:

  • use texelfetch but bilinearly interpolate between the lookups to smooth them, rather than letting the card do it
    • We can't do it only on the vertices (looked up in vertex()) because they spread out on lower lods, but
    • We can use the same point grid and bilinear interpolation that the material lookups use.
  • generate normal map and pass it in to the shader
    • perhaps this can be done upon mouse release for the section that was just modified, then blit in to the normal map
    • when GPU painting works, could be generated by the compute shader on the fly, but will it have the same issue?

image

image

Testing notes:

  • offset collision half a texel instead of shader - no it's the shader that's off due to filtering/interpolation
  • vertex, calculate 9 heights around, say in varying, then using textureGather
  • set map to nearest & use texel lookup
  • fragment linear interpolate normals (or heights) from vertex neighbors, lerped based on position
  • use texel lookup per pixel, but lerp it
  • if on edge of region use texel otherwise texture?

I tried clamping to the region so it wouldn't sample across boundaries and just average, that reduced the size but didn't help

	vec2 region_min = floor(uv/_region_size); //1023/1024->0, 4095/4096->0
	vec2 region_max = region_min + vec2(_region_size);
	float left = get_height(clamp(uv + vec2(-_region_texel_size, 0), region_min, region_max));
	float right = get_height(clamp(uv + vec2(_region_texel_size, 0), region_min, region_max));
	float back = get_height(clamp(uv + vec2(0, -_region_texel_size), region_min, region_max));
	float front = get_height(clamp(uv + vec2(0, _region_texel_size), region_min, region_max));
@TokisanGames TokisanGames added the bug Something isn't working label Aug 16, 2023
@TokisanGames TokisanGames added this to the Stable milestone Aug 16, 2023
@TokisanGames TokisanGames changed the title Artifact along regions Artifact along region boundaries Sep 5, 2023
@TokisanGames TokisanGames changed the title Artifact along region boundaries Normal artifact along region boundaries Dec 20, 2023
@TokisanGames
Copy link
Owner Author

This artifact is made worse by the autoshader since it relies on the normal calculation.

image

@TokisanGames
Copy link
Owner Author

TokisanGames commented Feb 12, 2024

Exploring more, here is the call stack

fragment()
  get_normal()
    get_height()
      get_region_uv2

The problem is visible in get_height. This code can be pasted into fragment and look at the artifact on the inside of the bowl next to the mountain:

	highp float height = 0.0;
	vec3 region = get_region_uv2(uv2);
	if (region.z >= 0.) {
		height = texture(_height_maps, region).r;
	}
	ALBEDO = vec3(height-64.)/50.;

Height is not read smoothly across the region boundaries in fragment. It screws up normals when they take the difference in heights across the boundaries, which also screws up the autoshader.

image

Height seems to be read smoothly across region boundaries in vertex

image

Some of my thoughts are:

  1. Use texelfetch to remove texture() automatic interpolation along the boundaries, then do our own interpolation of normals
    • Side note: I attempted to use texturequerylod and texelfetch in get_height but it wouldn't compile. I had to break get_height into a vertex and fragment version before I could use it.
  2. Clamp the lookup offsets so the normal is calculated using the region internal pixels only.
  3. Generate the normal map in C++/compute instead of this shader

Stakira suggested:

The easiest solution would be use one pixel as padding and change region size to 1022.


Attempting 2. above, I used a normal method using only 3 lookups, center plus two perpendicular offsets:

// 3-Lookup method
// https://www.gamedev.net/forums/topic/428776-create-a-normal-map-from-heightmap-in-a-pixel-shader/?page=1
float ht = texture(terrain_heightmap, v_uv).r;
float htU = texture(terrain_heightmap, v_uv + vec2(texel_size, 0.)).r;
float htV = texture(terrain_heightmap, v_uv + vec2(0., texel_size)).r;
vec3 normal = vec3(ht - htU, terrain_normal_strength, ht - htV);
terrain_normal_world = normalize(normal);

Current 4-lookup method, 3-lookup method, 3-lookup method conditional with reverse if near region boundary (red). So far the third reduces the issue the most.

image

In real world use this is much better and will be incorporated until we can find a better solution. Current vs 3-lookup conditional:

image

Autoshader

image

@TokisanGames
Copy link
Owner Author

Partial workaround added in 990e7c0

@TokisanGames TokisanGames added the help wanted Help from other contributors desired label Mar 28, 2024
@TokisanGames
Copy link
Owner Author

This might be a solution.

https://discord.com/channels/691957978680786944/1065519581013229578/1225045276759294053

reading height offsets in vertex to pass to fragment, and using those for just the borders yields perceptibly perfect visual results. before and after:

vec3 get_normal(vec2 uv, out vec3 tangent, out vec3 binormal) {
    float u, v, height;
    
    // Swap to vertex height reads for region borders, see #185.
    // v_heights is varying
    float uvy = mod(uv.y*_region_size, _region_size);
    float uvx = mod(uv.x*_region_size, _region_size);
    if(uvy > _region_size-1.5 || uvy < .5 || uvx > _region_size-1.5 || uvx < .5) {
        height = v_heights[2];
        v = height - v_heights[0];
        u = height - v_heights[1];
    } else {
        height = get_height(uv);
        v = height - get_height(uv + vec2(0, _region_texel_size));
        u = height - get_height(uv + vec2(_region_texel_size, 0));
    }
    
    vec3 normal = vec3(u, _mesh_vertex_spacing, v);
    normal = normalize(normal);
    tangent = cross(normal, vec3(0, 0, 1));
    binormal = cross(normal, tangent);
    return normal;
}

Yes, though im doing this in vertex, probably could branch this off too but didnt go that far"
image

@Xtarsia
Copy link
Contributor

Xtarsia commented Apr 3, 2024

	// Region border height reads to avoid interpolation error.
	v_heights = vec3(0,0,VERTEX.y);
	float uvy = mod(v_vertex.x, _region_size);
	float uvx = mod(v_vertex.z, _region_size);
	if(uvy > _region_size-32. || uvy < 16. || uvx > _region_size-32. || uvx < 16.) {
		v_heights[0] = get_height(UV2 + vec2(0,_region_texel_size));
		v_heights[1] = get_height(UV2 + vec2(_region_texel_size,0));
	}

this should branch the additional vertex reads correctly.

@FishOfTheNorthStar
Copy link
Contributor

FishOfTheNorthStar commented Apr 14, 2024

temp_terrain3d_normalfix

Original on top, with the fix I described in my last post in place on bottom set. So basically fixed, I guess. I can't see it anymore, at all.

Working on a cleaner integration of this with existing codebase. Will update soon.

Notable: bottom image normals calculated inside the vertex function, not the fragment as is now. Will be trying out a higher quality normal calculation soon, central-differences is whats in place now and it's kind of not-the-best. I think Sobel or Sharr would look very nice, and I have a fairly performant version of both already cooked up for other things.

edit: Here's my current vertex shader, where almost everything that can happen at that point does happen, so it'll look a lot different (but familiar, most of it is from the fragment code). There's still some issues i'm ironing out (codes still dirty and some thing isn't right about how i'm indexing i00etc versus how they're used later), but you can see more how I'm distributing the error pixel across the width to minimize it's impact, and that seems to be working real well.

void vertex() {
	// Get camera pos in world vertex coords
    v_camera_pos = INV_VIEW_MATRIX[3].xyz;

	// Get vertex of flat plane in world coordinates and set world UV
	v_vertex = (MODEL_MATRIX * vec4(VERTEX, 1.0)).xyz;
	
	// UV coordinates in world space. Values are 0 to _region_size within regions
	vec2 fUV = v_vertex.xz * _mesh_vertex_density;
	if (uv_distortion) {
		v_uvdistort = standard_uv_distortion(fUV); } 
	else {
		v_uvdistort = vec2(0.); }

	UV = round(fUV);
	v_norm_region.xy = mod(UV, _region_size) * _region_texel_size;

	// Discard vertices for Holes. 1 lookup
	v_region = get_region_uv(UV);
	v_norm_region.z = float(v_region.z);
	uint _rv=uint(clamp(v_region.z+1,0,1));
	float _frv=float(_rv);
	uint control = texelFetch(_control_maps, v_region, 0).r;
	bool hole = bool(control >>2u & 0x1u);
	// Show holes to all cameras except mouse camera (on exactly 1 layer)
	if (  !(CAMERA_VISIBLE_LAYERS == _mouse_layer) 
		&& (hole || (_background_mode + _rv == 0u)) ) {
		VERTEX.x = 0./0.; } 
	else {
		vec3 _step = vec3(1.,1.,0.) * _region_texel_size;
		vec3 half_step = _step *.5;
		float _valid = 1. - _step.x;
		vec3 t_norm = v_norm_region;
		t_norm.xy *= _valid;
		const vec4 zro = vec4(0);
		// UV coordinates in region space + texel offset. Values are 0 to 1 within regions
		UV2 = UV * _step.x;
		vec4 vgh = mix(zro, textureGather(_height_maps, v_norm_region + half_step, 0), _frv);
		vec4 ngh = mix(zro, textureGather(_height_maps, t_norm + half_step, 0), _frv);
		if(_background_mode == 2u) {
	       float _tch; 
		   _tch=noise_mod(ngh.w,UV2)-ngh.w;ngh.w+=_tch;vgh.w+=_tch;
		   _tch=noise_mod(ngh.z,UV2+_step.xz)-ngh.z;ngh.z+=_tch;vgh.z+=_tch;
		   _tch=noise_mod(ngh.x,UV2+_step.zy)-ngh.x;ngh.x+=_tch;vgh.x+=_tch; }
		v_normal = normalize(vec3(ngh.w - ngh.z, _mesh_vertex_spacing, ngh.w - ngh.x));
		v_tangent = cross(v_normal, vec3(0, 0, 1));
		v_binormal = cross(v_normal, v_tangent);

		// Get final vertex location and save it
		VERTEX.y = vgh.w;

		v_vertex = (MODEL_MATRIX * vec4(VERTEX, 1.0)).xyz;
		v_vertex_dist = length(v_vertex - v_camera_pos);

		// Get the region and control map ID for the vertices
		ivec3 i00, i01, i10, i11; ivec2 iUV=ivec2(UV); 
		i00=get_region_uv(UV);
		i01=get_region_uv(vec2(iUV+ivec2(0,1)));
		i10=get_region_uv(vec2(iUV+ivec2(1,0)));
		i11=get_region_uv(vec2(iUV+ivec2(1,1)));

		// Lookup adjacent vertices. 4 lookups
		//uint4 con = textureGather(_control_maps, v_region, 0);
		v_control = uvec4(
			texelFetch(_control_maps,i00,0).r,
			texelFetch(_control_maps,i01,0).r,
			texelFetch(_control_maps,i10,0).r,
			texelFetch(_control_maps,i11,0).r);
		v_neighbors.x=i00.z;
		v_neighbors.y=i01.z;
		v_neighbors.z=i10.z;
		v_neighbors.w=i11.z;
		v_rangles.x = random(i00.xy); 
		v_rangles.y = random(i01.xy); 
		v_rangles.z = random(i10.xy); 
		v_rangles.w = random(i11.xy); 
		v_rangles -= 0.5;
		v_rangles *= TAU;
//INSERT: DUAL_SCALING_VERTEX
	}

	// Transform UVs to local to avoid poor precision during varying interpolation.
	v_uv_offset = MODEL_MATRIX[3].xz * _mesh_vertex_density;
	UV -= v_uv_offset;
	v_uv2_offset = v_uv_offset * _region_texel_size;
	UV2 -= v_uv2_offset; }

btw: see that 'v_uvdistort'? OT but it's something I'm working on now. Behold the gibberish it makes of a unit grid, lol.
image

@TokisanGames
Copy link
Owner Author

@FishOfTheNorthStar Responding to #348 (comment) and above.

I appreciate your ideas. You and @Xtarsia both seem to have solutions that address the normal artifact, and I am excited about optimizing the shader further. I have a running list of ideas in #26.

Note: I calculate all normals within vertex function but your current version is still doing that per pixel, i suspect to help the panels be more seamless between resolution jumps.

I also want to do all lookups per vertex and remove all fragment lookups. I was able to have it working fine on LOD0, but it didn't look good LOD1+, and not on the transitions. Did you solve this? I've had this theory that removing all fragment lookups and removing all lods might be more performant than fragment lookups with lods.

So the end result of this is spreading the error pixel row/column across the entire width, and in this way it basically completely disappears. I can't detect it anyways, in Grey view.

Regarding the pictures above, I see the artifact is gone, great. However, the normals elsewhere in the background are noticeably softer in the fixed version than before.

I've been thinking about a more durable fix and I think i have a plan that would 100% mitigate the issue forever, and considerably improve performance at the same time, but I suspect you won't like it:
Encode an extra 1 pixel margin along each edge of all control maps / height maps. So row 0 and row 1023 (and same columns), those are just repeats of row 1 and row 1022, basically.

@Xtarsia suggested something similar. I'm not excited about throwing away 1-2m of data. I'm not sure which is the best solution yet. The ideas about using TextureGather and other things sound interesting. The more optimal the shader, the more features we can add to it.

Also, of course, migrating existing datasets over to new format... oof. Whatever, skipping that part for now.

Migrating is the least of my concerns. I've been upgrading people seamlessly since my second release.

What is the concern is making changes that will require the user to move their objects around or resculpt/texture. The questions are:

  • Can we fix this artifact without negative impacting?
  • Can we optimize the shader without negative impact?
  • If using a 1px margin allows for such great performance gains that its worth doing, how do we change the data to minimize or eliminate the impact to the user?

There's still some issues i'm ironing out

I look forward to seeing what you come up with. One thing I will ask on PRs is no arcane variable names like below. I need understandable code, variables with full name instead of vgh, zro, frv, etc so I and others can have a chance at understanding it.

  vec4 vgh = mix(zro, textureGather(_height_maps, v_norm_region + half_step, 0), _frv);

@TokisanGames
Copy link
Owner Author

@Xtarsia your solution in #185 (comment) and #185 (comment) works well where there are regions, however it turns into a grid where there aren't any and using noise. Do you have a fix for this?

image

@Xtarsia
Copy link
Contributor

Xtarsia commented Apr 17, 2024

@TokisanGames Perhaps switching to full vertex reads when not in region would be feasible, and even a potential performance gain?

Edit: this was a total failure.

@Xtarsia
Copy link
Contributor

Xtarsia commented Apr 17, 2024

@TokisanGames

vec3 get_normal(vec2 uv, out vec3 tangent, out vec3 binormal) {
    float u, v, height;

    // Swap to vertex height reads for region borders, see #185.
    // v_heights is varying. we are relying on GPU interolation for crossboarder normals
    float uvy = mod(uv.y*_region_size, _region_size);
    float uvx = mod(uv.x*_region_size, _region_size);
    if((uvy > _region_size-1.5 || uvy < .5 || uvx > _region_size-1.5 || uvx < .5) && !(v_region.z==-1)){
        height = v_heights[2];
        v = height - v_heights[0];
        u = height - v_heights[1];
    } else {
        height = get_height(uv);
        v = height - get_height(uv + vec2(0, _region_texel_size));
        u = height - get_height(uv + vec2(_region_texel_size, 0));
    }

    vec3 normal = vec3(u, _mesh_vertex_spacing, v);
    normal = normalize(normal);
    tangent = cross(normal, vec3(0, 0, 1));
    binormal = cross(normal, tangent);
    return normal;
}

This fixes 99%
however a strange edge case with world noise exists at the corners where the world noise bleeds into an actual region.

this is fixed with this:

	v_heights = vec3(get_height(UV2));
	float uvy = mod(v_vertex.x, _region_size);
	float uvx = mod(v_vertex.z, _region_size);
	if(uvy > _region_size-6.0 || uvy < 0.5 || uvx > _region_size-6.0 || uvx < 0.5) {
		v_heights[0] = get_height(UV2 + vec2(0,_region_texel_size));
		v_heights[1] = get_height(UV2 + vec2(_region_texel_size,0));
	}

this line v_heights = vec3(get_height(UV2)); however irks me, as it is a duplicate of the branched read just before in the vertex function.

edit: corner noise still an issue, related to the direction of the offset height reads. though probably better to fix corner world noise blending.

(all tested on a fresh project with 4.2.1.stable with 0.9.1 beta release)

@Xtarsia
Copy link
Contributor

Xtarsia commented Apr 17, 2024

the second snag regarding noise on region boarders is a strange one...

is the world noise really supposed to blend into the region maps?

image

@TokisanGames
Copy link
Owner Author

is the world noise really supposed to blend into the region maps?

Yes, just a small adjustable amount to avoid hard lines.

With these shader updates, I don't see an artifact anywhere, even on the edge of the world. However, I do see it costs about 5-10% of performance. 🤔

Also strangely, this eliminates the artifact:

v_heights = vec3(get_height(UV2));

But this makes it worse:

highp float ht = get_height(UV2);
v_heights = vec3(ht);

I don't understand why. I tried several other variations and all produced the same result, only sending get_height directly into v_heights allowed it to work. There is a get_height(UV2) call earlier in vertex() for VERTEX that I was trying to use, but I couldn't reuse the lookup. I also couldn't set v_heights before and set VERTEX off of it. So there's a redundant lookup, but I'm confused as to why I can't reuse the data.

Xtarsia added a commit to Xtarsia/Terrain3D that referenced this issue Apr 18, 2024
branching for vertex normals to fix TokisanGames#185 caused performance loss.
so make that branch as big as possible without visual loss..
Xtarsia added a commit to Xtarsia/Terrain3D that referenced this issue Apr 19, 2024
Branching for vertex normals to fix TokisanGames#185 caused performance loss.
so make that branch as big as possible without visual loss..
Xtarsia added a commit to Xtarsia/Terrain3D that referenced this issue Apr 22, 2024
Branching for vertex normals to fix TokisanGames#185 caused performance loss.
so make that branch as big as possible without visual loss..
TokisanGames pushed a commit to Xtarsia/Terrain3D that referenced this issue Apr 23, 2024
Branching for vertex normals to fix TokisanGames#185 caused performance loss.
so make that branch as big as possible without visual loss..
TokisanGames pushed a commit that referenced this issue Apr 23, 2024
Branching for vertex normals to fix #185 caused performance loss.
so make that branch as big as possible without visual loss..
@TokisanGames
Copy link
Owner Author

TokisanGames commented Apr 23, 2024

Excellent work by @Xtarsia finally fixed this long standing bug in #353.

@FishOfTheNorthStar and @Xtarsia Let's keep talking about shader optimizations in #26 and other places. I'm very interested in the idea of removing all fragment lookups and boosting speed. In the meantime, we have an improved terrain today! Also Fish, you might want to join my discord where we talk a lot about development.

@TokisanGames TokisanGames modified the milestones: Stable 1.0.x, Beta 0.9.x May 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Help from other contributors desired
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants