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

Fix multiple issues with UV compression #84159

Merged
merged 1 commit into from
Oct 30, 2023

Conversation

clayjohn
Copy link
Member

Fixes: #84089

The root of #84089 comes from how we compress UVs. When using the compressed format, we have to scale UVs into the 0-1 range. To cut down on data that needs to be passed into the shader, we always scale around 0. Which means that to take an arbitrary range and convert it into 0-1, we need to divide by 2 * the maximum absolute value and add 0.5.

This compresses 0-1 UVs into 0.5-1 or, alternatively -2-2 uvs into 0-1.

This compression makes sense and is cheap, but it comes with a fatal flaw. The most common use case for uvs is the 0-1 range. When you convert 0-1 into 0.5-1 and convert to 16-bit, you end up with a range of 0.49992 - 1, which uncompressed in the shader becomes -0.0002 - 1.

This is fine when the UV is only used for reading textures. But, the shader in #84089 assumes that UV is always positive (which is a fair assumption) and it ends up with undefined behaviour when UVs are surprisingly not positive. I am worried that this will be a common case among users.

We have two possible solutions:

  1. Force 0.0 to scale to something slightly over 0.5 (tricky to manage properly)
  2. Avoid scaling 0-1 range UVs.

I opted for option 2. The shader only runs the unscaling code if the uv_scale != vec4(0.0). So we detect when UVs are in the 0-1 range and avoid scaling them. We still compress to 16-bit though to respect the compression setting.

While implementing this fix, I noticed that the code to retrieve arrays from the GPU wasn't unscaling the UVs as it should and was returning the scaled UVs. This was an oversight from when compression was added as, originally, we only compressed UVs in the 0-1 range and we never applied scaling.

I have added the code to unscale.

This second bug hasn't been reported yet, but it can be reproduced trivially with the following code:

	var mesh = ArrayMesh.new()
	var arrays = Array()
	arrays.resize(Mesh.ARRAY_MAX)
	
	var verts = PackedVector3Array()
	var uvs = PackedVector2Array()
	
	verts.push_back(Vector3(0.0, 0.0, 0.0))
	verts.push_back(Vector3(0.0, 0.0, 0.0))
	verts.push_back(Vector3(0.0, 0.0, 0.0))
	
	uvs.push_back(Vector2(0.0, 1.5))
	uvs.push_back(Vector2(0.5, 0.5))
	uvs.push_back(Vector2(0.5, 0.5))
	
	arrays[Mesh.ARRAY_VERTEX] = verts
	arrays[Mesh.ARRAY_TEX_UV] = uvs
	mesh.add_surface_from_arrays(Mesh.PRIMITIVE_TRIANGLES, arrays, [], {}, RenderingServer.ARRAY_FLAG_COMPRESS_ATTRIBUTES)
	var arrays2 = mesh.surface_get_arrays(0)
	print(arrays2[Mesh.ARRAY_TEX_UV])

With no compression this prints [(0, 1.5), (0.5, 0.5), (0.5, 0.5)],
In Beta 3 this prints [(0.499992, 1), (1, 0.666667), (1, 0.666667)]
With this PR, this prints [(-0.000008, 1.5), (0.5, 0.5), (0.5, 0.5)]

Finally, this binds ARRAY_FLAG_COMPRESS_ATTRIBUTES which was missing in the API

Copy link
Contributor

@BastiaanOlij BastiaanOlij left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

doc/classes/Mesh.xml Outdated Show resolved Hide resolved
@clayjohn clayjohn force-pushed the uv-compression branch 2 times, most recently from f230c34 to 1b60fea Compare October 30, 2023 16:24
doc/classes/Mesh.xml Outdated Show resolved Hide resolved
@akien-mga akien-mga merged commit 17a5756 into godotengine:master Oct 30, 2023
14 checks passed
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Strange lights show on ShaderMaterial in 4.2
3 participants