Metal: Fix crash when uniform set is empty for classic binding mode

[stuartcarnie]

Closes #100764

Resolves a CRASH_BAD_INDEX error when binding uniform sets that are empty, specifically for A12 devices and earlier, which use classic ABI vs argument buffers.

Resolves a Metal CPU API validation error when binding "empty" particle buffers, that were incorrectly sized. Metal doesn't support defining a struct with an empty array.

Overview

An example of this in GLSL is:

struct ParticleEmission {
  vec3 velocity;
  uint flags;
};

layout(set = 1, binding = 2, std430) restrict buffer SourceEmission {
  int particle_count;
  uint pad0;
  uint pad1;
  uint pad2;
  ParticleEmission data[];
}
src_particles;

The minimum size of SourceEmission is 16 bytes when binding a buffer for backends that support empty arrays.

However, when this is converted to Metal, note the data member is an array of size 1:

struct ParticleEmission {
  float3 velocity;
  uint flags;
};

struct SourceEmission {
  int particle_count;
  uint pad0;
  uint pad1;
  uint pad2;
  ParticleEmission data[1];
};

This means that the minimum size of the struct when binding an "empty" array is 32 bytes. This doesn't cause an issue when Metal API validation is disabled, but this feature is often enabled when running code from Xcode, which iPhone developers are likely to do.

[clayjohn]

Suggested change

	for (uint32_t i = 0; i < std::min(uniforms.size(), set.uniforms.size()); i++) {
	for (uint32_t i = 0; i < MIN(uniforms.size(), set.uniforms.size()); i++) {

This should use our math library instead of std

[stuartcarnie]

I was having trouble, as NSObjCRuntime.h defines them. I've #undef'd them, so it uses Godot's constexpr functions now.

[akien-mga]

That's weird, as core/typedefs.h which defines our MIN and MAX also undef's the pre-existing ones.

Maybe you need an explicit core/typedefs.h include here? It's kind of taken for granted usually as most Godot headers include it one way or another, but maybe this specific file of the Metal driver doesn't as it's relatively self-contained?

[stuartcarnie]

Unfortunately, I'd have to include typedefs.h before the system libraries, and then NSObjCRuntime.h re-defines it again. Quite a nuisance!

[stuartcarnie]

This is what it looks like:

#if !defined(MIN)
    #define __NSMIN_IMPL__(A,B,L) ({ __typeof__(A) __NSX_PASTE__(__a,L) = (A); __typeof__(B) __NSX_PASTE__(__b,L) = (B); (__NSX_PASTE__(__a,L) < __NSX_PASTE__(__b,L)) ? __NSX_PASTE__(__a,L) : __NSX_PASTE__(__b,L); })
    #define MIN(A,B) __NSMIN_IMPL__(A,B,__COUNTER__)
#endif

[Calinou]

Code looks good to me.

[stuartcarnie]

I'm creating an empty buffer with enough data for one element, which is required for backends, like Metal, that don't support empty arrays in struct definitions, similar to C++.

[stuartcarnie]

This was incorrectly sized, so was causing validation errors when using Metal's slot-based binding API (A12 devices)

[stuartcarnie]

@TCROC can you try this, as it should resolve your issues

[TCROC]

@TCROC can you try this, as it should resolve your issues

@stuartcarnie Yep. I'll merge it into my fork now and let you know how it goes :).

[TCROC]

@stuartcarnie I can confirm, this PR appears to fix the issue! Thank you for the quick response and patch! :)