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

World Triplanar broken on Mac - 4.4.dev3+ #99029

Closed
bneilw opened this issue Nov 10, 2024 · 10 comments · Fixed by #99261
Closed

World Triplanar broken on Mac - 4.4.dev3+ #99029

bneilw opened this issue Nov 10, 2024 · 10 comments · Fixed by #99261

Comments

@bneilw
Copy link

bneilw commented Nov 10, 2024

Tested versions

Reproducible in 4.4.dev3 and 4.4.dev4. Not reproducible in 4.3 stable.

System information

Godot v4.4.dev4 - macOS 14.5.0 - Multi-window, 1 monitor - Metal (Forward+) - integrated Apple M1 Max (Apple7) - Apple M1 Max (10 threads)

Issue description

Using world Triplanar on spatial material results in black artifacts.

Steps to reproduce

  • Create a new project in Godot 4.4.dev3+
  • Create a new 3d scene
  • Add new MeshInstance with plane
  • Assign a new material override
  • Set the Triplanar and world Triplanar flags on material
  • Move Camera in editor. Issue persists as well during runtime.

Minimal reproduction project (MRP)

world-triplanar-4.4.zip

@clayjohn
Copy link
Member

Reproduced on a M2 Mac as far back as 4.4-dev1. Turning off the Metal backend removes the artifacts

CC @stuartcarnie

Screenshot 2024-11-10 at 9 58 01 AM

The artifacts go away if you disable world_triplanar, then toggle triplanar and enable world_triplanar in the material, but then come back if you click on the node in the scenetree. So I suspect there is some leaking state going on.

@stuartcarnie
Copy link
Contributor

stuartcarnie commented Nov 10, 2024

So I suspect there is some leaking state going on.

@clayjohn can you elaborate on "some leaking state" and what that might be – what should I be looking for?

@clayjohn
Copy link
Member

@stuartcarnie I'm not sure exactly. It would be if anything is set globally instead of per command. For example, Vulkan as the ability to set certain state (like the viewport rect) dynamically. If you forget to reset that state it can leak into other operations.

This artifact to me looks like its reading from un-initialized memory in the fragment shader. So a very rough guess would be something like a sampler state is leaking for one of the textures.

@stuartcarnie
Copy link
Contributor

@clayjohn curiously, if I enable the Grow option, the issue goes away. Seems like it might be an issue with the layout of the MaterialUniforms, which is affected by that change.

Without grow enabled, the generated struct in the Metal source looks like this:

struct MaterialUniforms
{
	float4 m_albedo;
	float m_point_size;
	float m_roughness;
	float4 m_metallic_texture_channel;
	float m_specular;
	float m_metallic;
	float m_uv1_blend_sharpness;
	float3 m_uv1_scale;
	float3 m_uv1_offset;
	float3 m_uv2_scale;
	float3 m_uv2_offset;
};

with grow:

struct MaterialUniforms
{
    float4 m_albedo;
    float m_grow;
    float m_point_size;
    float m_roughness;
    float4 m_metallic_texture_channel;
    float m_specular;
    float m_metallic;
    float m_uv1_blend_sharpness;
    float3 m_uv1_scale;
    float3 m_uv1_offset;
    float3 m_uv2_scale;
    float3 m_uv2_offset;
};

@clayjohn
Copy link
Member

@stuartcarnie Oh! Its a struct padding issue. Every graphics API handles struct padding slightly differently. Universally 3 floats in a row will add an extra float of padding. But two floats are handled differently, some will pad that to 4, but others won't pad when its only 2.

We are using SPIRV-Cross right? It might have some options to force a particular padding. But its probably just best for us to add padding in the shader compiler

@stuartcarnie
Copy link
Contributor

@clayjohn I had a suspicion it might be that. I'll dig a bit deeper into padding.

@stuartcarnie
Copy link
Contributor

stuartcarnie commented Nov 12, 2024

@clayjohn I'm curious how the MaterialUniforms, which is generated at runtime, is valid, given size and alignment rules for Vulkan appear to be the same as Metal.

This is the generated version that I see (with offset / size as comment):

struct MaterialUniforms
{
    float4 m_albedo;                    //   0 / 16
    float m_point_size;                 //  16 /  4
    float m_roughness;                  //  20 /  4
    float4 m_metallic_texture_channel;  //  24 / 16
    float m_specular;                   //  40 /  4
    float m_metallic;                   //  44 /  4
    float m_uv1_blend_sharpness;        //  48 /  4
    float3 m_uv1_scale;                 //  52 / 16
    float3 m_uv1_offset;                //  68 / 16
    float3 m_uv2_scale;                 //  84 / 16
    float3 m_uv2_offset;                // 100 / 16
};

vs what I would expect is valid, based on Vulkan and Metal rules:

struct MaterialUniforms
{
    float4 m_albedo;                    //   0 / 16
    float m_point_size;                 //  16 /  4
    float m_roughness;                  //  20 /  4
    float2 pad1;                        //  24 /  8
    float4 m_metallic_texture_channel;  //  32 / 16
    float m_specular;                   //  48 /  4
    float m_metallic;                   //  52 /  4
    float m_uv1_blend_sharpness;        //  56 /  4
    float pad2;                         //  60 /  4
    float3 m_uv1_scale;                 //  64 / 16
    float3 m_uv1_offset;                //  80 / 16
    float3 m_uv2_scale;                 //  96 / 16
    float3 m_uv2_offset;                // 112 / 16
};

@stuartcarnie
Copy link
Contributor

I can confirm that when I write the struct using Apple simd types, it matches Metal's padding rules:

struct MaterialUniforms
{
	simd_float4 m_albedo;                    //   0 / 16
	float m_point_size;                      //  16 /  4
	float m_roughness;                       //  20 /  4 (+ 8 bytes padding)
	simd_float4 m_metallic_texture_channel;  //  32 / 16
	float m_specular;                        //  48 /  4
	float m_metallic;                        //  52 /  4
	float m_uv1_blend_sharpness;             //  56 /  4 (+ 4 bytes padding)
	simd_float3 m_uv1_scale;                 //  64 / 16
	simd_float3 m_uv1_offset;                //  80 / 16
	simd_float3 m_uv2_scale;                 //  96 / 16
	simd_float3 m_uv2_offset;                // 112 / 16
};

@stuartcarnie
Copy link
Contributor

I found this issue KhronosGroup/SPIRV-Cross#1572 (comment)

We need manual padding to handle this, as the Metal driver uses argument buffers, which contain embedded structs for the descriptor sets.

I'm going to try the latest version of MoltenVK and enable argument buffers to see if the same problem exists.

@stuartcarnie
Copy link
Contributor

For reference, Godot and Metal agreed on the struct layout – it was an unrelated issue per #99261

stuartcarnie added a commit to stuartcarnie/godot that referenced this issue Nov 15, 2024

Verified

This commit was created on github.com and signed with GitHub’s verified signature. The key has expired.
@AThousandShips AThousandShips added this to the 4.4 milestone Nov 15, 2024
tGautot pushed a commit to tGautot/godot that referenced this issue Feb 5, 2025

Verified

This commit was created on github.com and signed with GitHub’s verified signature.
WhalesState pushed a commit to WhalesState/blazium that referenced this issue Mar 14, 2025
WhalesState pushed a commit to WhalesState/blazium that referenced this issue Mar 14, 2025
WhalesState pushed a commit to WhalesState/blazium that referenced this issue Mar 15, 2025
WhalesState pushed a commit to WhalesState/blazium that referenced this issue Mar 15, 2025
WhalesState pushed a commit to WhalesState/blazium that referenced this issue Mar 15, 2025
WhalesState pushed a commit to WhalesState/blazium that referenced this issue Mar 15, 2025
WhalesState pushed a commit to WhalesState/blazium that referenced this issue Mar 15, 2025
WhalesState pushed a commit to WhalesState/blazium that referenced this issue Mar 15, 2025
WhalesState pushed a commit to WhalesState/blazium that referenced this issue Mar 16, 2025
WhalesState pushed a commit to WhalesState/blazium that referenced this issue Mar 20, 2025
WhalesState pushed a commit to WhalesState/blazium that referenced this issue Mar 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Very Bad
Development

Successfully merging a pull request may close this issue.

4 participants