godot icon indicating copy to clipboard operation
godot copied to clipboard

Round primitive meshes contain gaps.

Open Lielay9 opened this issue 1 year ago • 6 comments
trafficstars

Tested versions

v4.3.beta2.official [b75f0485b]

Tested version on v4.3.beta2.official [b75f0485b], but likely has been present for a long time. First I noticed it https://github.com/godotengine/godot/pull/80710#issuecomment-1690749054 (and subsequently forgot.)

System information

Godot v4.3.beta2 - Windows 10.0.22631 - GLES3 (Compatibility) - NVIDIA GeForce RTX 4090 (NVIDIA; 31.0.15.3699) - AMD Ryzen 9 7950X 16-Core Processor (32 Threads)

Issue description

Primitive meshes that contain round surfaces, i.e. SphereMesh, CapsuleMesh, CylinderMesh and TorusMesh, have minuscule gaps. This is due to the fact that some of the vertices at the seams do not align. The gaps are unlikely to be seen in regular use but here's an image of an unconventional shader that exaggerates them:

capsule_and_sphere

Capsule and sphere, using a shader that expands vertices based on their floating-point presentations mantissa (for visualization purposes only; do not use the shader to debug the issue, as it might be inaccurate):

shader_type spatial;
render_mode cull_disabled, unshaded;

void vertex() {
	vec3 n = normalize(vec3(floatBitsToUint(VERTEX) & uvec3(255)) - 127.5);
	VERTEX += NORMAL * (dot(NORMAL, n) + 1.) * 0.05;
}

void fragment() {
	ALBEDO = FRONT_FACING ? vec3(0.) : vec3(1., 0., 1.);
}

The gaps are more of an issue when manipulating the meshes in code, e.g. when iterating unique vertices to create edge-meshes.

The cause is likely a precision issue originating from calculating the same position twice with different values. Example code from CylinderMesh:

https://github.com/godotengine/godot/blob/9db1a963beae8056cbd30d692d4160d09c10b2dc/scene/resources/3d/primitive_meshes.cpp#L1010-L1015

(sin(0) and sin(TAU) might not be identical due to limited precision, unlike in math)

For a fix, I'd suggest either reusing values for positions that are already calculated or ensuring that the values used to derive the positions are identical.

Steps to reproduce

The unaligned vertices can be found using the following script:

extends Node

func _ready() -> void:
	print_gaps(SphereMesh.new())
	print_gaps(CapsuleMesh.new())
	print_gaps(CylinderMesh.new())
	print_gaps(TorusMesh.new())

func print_gaps(mesh: PrimitiveMesh) -> void:
	var vertex_array: PackedVector3Array = mesh.get_mesh_arrays()[Mesh.ArrayType.ARRAY_VERTEX]
	for i: int in vertex_array.size():
		var v1: Vector3 = vertex_array[i]
		for j: int in range(i+1, vertex_array.size()):
			var v2: Vector3 = vertex_array[j]
			if v1 != v2 and v1.is_equal_approx(v2):
				printt("Gap:", v1, v2)

Minimal reproduction project (MRP)

Project containing both the shader used to demonstrate the gaps and the script to find them: primitive_meshes.zip

Lielay9 avatar Jul 02 '24 14:07 Lielay9

Running a related quick test:

print(is_equal_approx(sin(TAU), sin(0)))
print(sin(TAU)== sin(0))
print(sin(TAU))
print(sin(0))

prints:

true
false
-0
0

So indeed we can't count on the value matching.

clayjohn avatar Jul 02 '24 17:07 clayjohn

Hello! First timer here, would I be alright if I take on this issue?

CatMachina avatar Jul 05 '24 04:07 CatMachina

Can this be fixed by using safesin and safecos functions from MathUtils library?

ayanchavand avatar Jul 05 '24 12:07 ayanchavand

Hello! First timer here, would I be alright if I take on this issue?

Yes, of course! It's ok to take issues if no one posted about taking them before you

kus04e4ek avatar Jul 06 '24 11:07 kus04e4ek

Yes, of course! It's ok to take issues if no one posted about taking them before you

Great! I'll do my best then 🫡

CatMachina avatar Jul 06 '24 18:07 CatMachina

Can this be fixed by using safesin and safecos functions from MathUtils library?

Since it's not part of the engine and there are ways to solve this without adding more third party code that wouldn't be an appropriate solution IMO

AThousandShips avatar Jul 06 '24 18:07 AThousandShips

Hello! First timer here, would I be alright if I take on this issue?

@CatMachina Hello, it's been a few days so I was wondering if you were still working on this? If not I'll take over :)

mickeyordog avatar Jul 09 '24 23:07 mickeyordog

@CatMachina Hello, it's been a few days so I was wondering if you were still working on this? If not I'll take over :)

Hey @mickeyordog! Sorry, still cracking at it, could you check back in a few days? 😅

CatMachina avatar Jul 10 '24 01:07 CatMachina

Hey @CatMachina, I had my eye on this issue for quite a while and I have implemented a fix. Do you mind if I add a pr?

ayanchavand avatar Jul 10 '24 12:07 ayanchavand

Hey @CatMachina, I had my eye on this issue for quite a while and I have implemented a fix. Do you mind if I add a pr?

Not at all @ayanchavand! I'm sure there are other issues that will inevitably crop up in the future. I wouldn't want to duplicate effort here. I would also love to see how you solved it 👀

CatMachina avatar Jul 11 '24 02:07 CatMachina

Do #94199 and #100020 do the same things?

fire avatar Dec 05 '24 21:12 fire

https://github.com/godotengine/godot/pull/100020#issue-2718559073

Supersedes https://github.com/godotengine/godot/pull/94199

Yes.

Lielay9 avatar Dec 05 '24 21:12 Lielay9