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

Round primitive meshes contain gaps. #93865

Closed
Lielay9 opened this issue Jul 2, 2024 · 12 comments · Fixed by #100020
Closed

Round primitive meshes contain gaps. #93865

Lielay9 opened this issue Jul 2, 2024 · 12 comments · Fixed by #100020

Comments

@Lielay9
Copy link
Contributor

Lielay9 commented Jul 2, 2024

Tested versions

v4.3.beta2.official [b75f048]

Tested version on v4.3.beta2.official [b75f048], but likely has been present for a long time. First I noticed it #80710 (comment) (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:

for (i = 0; i <= radial_segments; i++) {
u = i;
u /= radial_segments;
x = sin(u * Math_TAU);
z = cos(u * Math_TAU);

(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

@clayjohn
Copy link
Member

clayjohn commented Jul 2, 2024

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.

@CatMachina
Copy link

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

@ayanchavand
Copy link
Contributor

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

@kus04e4ek
Copy link
Contributor

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

@CatMachina
Copy link

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 🫡

@AThousandShips
Copy link
Member

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

@mickeyordog
Copy link
Contributor

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 :)

@CatMachina
Copy link

CatMachina commented Jul 10, 2024

@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? 😅

@ayanchavand
Copy link
Contributor

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?

@CatMachina
Copy link

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 👀

@fire
Copy link
Member

fire commented Dec 5, 2024

Do #94199 and #100020 do the same things?

@Lielay9
Copy link
Contributor Author

Lielay9 commented Dec 5, 2024

#100020 (comment)

Supersedes #94199

Yes.

@Repiteo Repiteo added this to the 4.4 milestone Dec 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants