godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
89.92k stars 21.05k forks source link

Round primitive meshes contain gaps. #93865

Open Lielay9 opened 3 months ago

Lielay9 commented 3 months ago

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

clayjohn commented 3 months ago

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 commented 3 months ago

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

ayanchavand commented 3 months ago

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

kus04e4ek commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 2 months ago

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 commented 2 months ago

@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 commented 2 months ago

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 commented 2 months ago

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 👀