Improve ability to redeclare buffer formats from struct definitions

qrenderdoc/Code/BufferFormatter.cpp

@@ -45,8 +45,6 @@ struct StructFormatData
 
   // does this contain a member annotated with [[single]] ?
   bool singleMember = false;
-
-  bool signedEnum = false;
 };
 
 GraphicsAPI BufferFormatter::m_API;
@@ -1042,7 +1040,9 @@ ParsedFormat BufferFormatter::ParseFormatString(const QString &formatString, uin
           }
 
           cur->structDef.type.matrixByteStride = VarTypeByteSize(tmp.type.baseType);
-          cur->signedEnum = (VarTypeCompType(tmp.type.baseType) == CompType::SInt);
+          cur->structDef.type.flags = (VarTypeCompType(tmp.type.baseType) == CompType::SInt)
+                                          ? ShaderVariableFlags::SignedEnum
+                                          : ShaderVariableFlags::NoFlags;
         }
 
         continue;
@@ -1065,7 +1065,7 @@ ParsedFormat BufferFormatter::ParseFormatString(const QString &formatString, uin
       QString valueNum = enumMatch.captured(2);
 
       bool ok = false;
-      if(cur->signedEnum)
+      if(cur->structDef.type.flags & ShaderVariableFlags::SignedEnum)
       {
         int64_t val = valueNum.toLongLong(&ok, 0);
 
@@ -2844,18 +2844,44 @@ QString BufferFormatter::DeclareStruct(Packing::Rules pack, ResourceId shader,
       const ShaderConstantType &pointeeType =
           PointerTypeRegistry::GetTypeDescriptor(shader, members[i].type.pointerTypeID);
 
-      varTypeName = MakeIdentifierName(pointeeType.name);
-
-      if(!declaredStructs.contains(varTypeName))
+      // don't declare pointer structs for plain scalar pointers (float *foo type)
+      if(pointeeType.name.empty() && pointeeType.members.size() == 1)
       {
-        declaredStructs.push_back(varTypeName);
-        declarations += DeclareStruct(pack, shader, declaredStructs, anonStructs, varTypeName,
-                                      pointeeType.members, pointeeType.arrayByteStride, QString()) +
-                        lit("\n");
+        varTypeName = pointeeType.members[0].type.name;
+      }
+      else
+      {
+        varTypeName = MakeIdentifierName(pointeeType.name);
+
+        if(!declaredStructs.contains(varTypeName))
+        {
+          declaredStructs.push_back(varTypeName);
+          declarations += DeclareStruct(pack, shader, declaredStructs, anonStructs, varTypeName,
+                                        pointeeType.members, pointeeType.arrayByteStride, QString()) +
+                          lit("\n");
+        }
       }
 
       varTypeName += lit("*");
     }
+    else if(members[i].type.baseType == VarType::Enum)
+    {
+      if(!declaredStructs.contains(varTypeName))
+      {
+        declaredStructs.push_back(varTypeName);
+
+        const bool signedEnum = bool(members[i].type.flags & ShaderVariableFlags::SignedEnum);
+        VarType enumType = signedEnum ? VarType::SInt : VarType::UInt;
+        if(members[i].type.arrayByteStride == 1)
+          enumType = signedEnum ? VarType::SByte : VarType::UByte;
+        else if(members[i].type.arrayByteStride == 2)
+          enumType = signedEnum ? VarType::SShort : VarType::UShort;
+        else if(members[i].type.arrayByteStride == 8)
+          enumType = signedEnum ? VarType::SLong : VarType::ULong;
+
+        declarations += DeclareEnum(varTypeName, members[i].type.members, enumType) + lit("\n");
+      }
+    }
     else if(members[i].type.baseType == VarType::Struct)
     {
       // GL structs don't give us typenames (boo!) so give them unique names. This will mean some
@@ -2891,6 +2917,23 @@ QString BufferFormatter::DeclareStruct(Packing::Rules pack, ResourceId shader,
     if(varName.isEmpty())
       varName = QFormatStr("_child%1").arg(i);
 
+    if(members[i].type.flags & ShaderVariableFlags::RGBDisplay)
+      varTypeName = lit("[[rgb]] ") + varTypeName;
+    if(members[i].type.flags & ShaderVariableFlags::SNorm)
+      varTypeName = lit("[[snorm]] ") + varTypeName;
+    if(members[i].type.flags & ShaderVariableFlags::UNorm)
+      varTypeName = lit("[[unorm]] ") + varTypeName;
+    if(members[i].type.flags & ShaderVariableFlags::R10G10B10A2)
+      varTypeName = lit("[[packed(r10g10b10a2)]] ") + varTypeName;
+    if(members[i].type.flags & ShaderVariableFlags::R11G11B10)
+      varTypeName = lit("[[packed(r11g11b10)]] ") + varTypeName;
+    // don't print the [[hex]] for pointer types
+    if(members[i].type.pointerTypeID == ~0U &&
+       (members[i].type.flags & ShaderVariableFlags::HexDisplay))
+      varTypeName = lit("[[hex]] ") + varTypeName;
+    if(members[i].type.flags & ShaderVariableFlags::BinaryDisplay)
+      varTypeName = lit("[[bin]] ") + varTypeName;
+
     if(members[i].type.rows > 1)
     {
       if(members[i].type.RowMajor())
@@ -2947,6 +2990,26 @@ QString BufferFormatter::DeclareStruct(Packing::Rules pack, ResourceId shader,
   return declarations + ret;
 }
 
+QString BufferFormatter::DeclareEnum(const QString &name, const rdcarray<ShaderConstant> &members,
+                                     VarType baseType)
+{
+  QString ret;
+
+  const bool signedEnum = VarTypeCompType(baseType) == CompType::SInt;
+
+  ShaderVariable tmp;
+  tmp.columns = 1;
+
+  ret += QFormatStr("enum %1 : %2\n{\n").arg(name).arg(ToQStr(baseType));
+  for(const ShaderConstant &c : members)
+  {
+    tmp.value.u64v[0] = c.defaultValue;
+    ret += QFormatStr("    %1 = %2,\n").arg(c.name).arg(RowString(tmp, 0, baseType));
+  }
+  ret += lit("}\n");
+  return ret;
+}
+
 QString BufferFormatter::DeclareStruct(Packing::Rules pack, ResourceId shader, const QString &name,
                                        const rdcarray<ShaderConstant> &members,
                                        uint32_t requiredByteStride)
@@ -3925,7 +3988,79 @@ QString RowTypeString(const ShaderVariable &v)
 
 #include "3rdparty/catch/catch.hpp"
 
-TEST_CASE("round-trip via format", "[formatter]")
+TEST_CASE("Round-trip struct declarations", "[formatter]")
+{
+  BufferFormatter::Init(GraphicsAPI::Vulkan);
+
+  QString fmt;
+
+  // things not tested here:
+  // comments, spacing, etc for obvious reasons
+  // [[pad]] because it's eliminated entirely and will not round-trip
+  // packing rules, they are tested below
+  // aliased typenames like float32_t or int16_t
+
+  fmt = lit(R"(#pack(structured)
+
+struct ptr_struct
+{
+    [[snorm]] byte4 a;
+    [[unorm]] byte4 a;
+    [[packed(r11g11b10)]] float3 b;
+    [[packed(r10g10b10a2)]] uint4 c;
+}
+
+struct nested
+{
+    float x[4];
+    [[hex]] uint y;
+    [[bin]] uint z;
+    [[row_major]] float3x4 w;
+    [[rgb]] float4 col;
+    [[offset(128)]]
+    int* p;
+    ptr_struct* q;
+}
+
+enum MyEnum : uint
+{
+    A = 1,
+    B = 2,
+    C = 10,
+    D = 10,
+}
+
+struct root
+{
+    float a;
+    int b;
+    uint c;
+    int d;
+    double e;
+    short f;
+    byte g;
+    float3 h;
+    float2x2 i;
+    int3x4 j;
+    uint k : 5;
+    uint l : 6;
+    uint : 3;
+    uint m : 7;
+    nested n;
+    MyEnum o;
+}
+)");
+
+  ParsedFormat parsed = BufferFormatter::ParseFormatString(fmt, 0, true);
+  CHECK(parsed.errors.isEmpty());
+  QString regenerated =
+      BufferFormatter::DeclareStruct(parsed.packing, ResourceId(), parsed.fixed.type.name,
+                                     parsed.fixed.type.members, parsed.fixed.type.arrayByteStride);
+
+  CHECK((fmt == regenerated));
+}
+
+TEST_CASE("Packing rules respected when round-tripping", "[formatter]")
 {
   BufferFormatter::Init(GraphicsAPI::Vulkan);
 
@@ -5202,6 +5337,86 @@ uint inthird : 14;
     CHECK(parsed.fixed.type.members[3].byteOffset == 8);
     CHECK(parsed.fixed.type.members[3].bitFieldOffset == 0);
     CHECK(parsed.fixed.type.members[3].bitFieldSize == 14);
+
+    // check that offsets are correctly processed after a bitfield that might have left some pending space
+    def = R"(
+uint a : 16;
+uint b : 14;
+
+[[offset(12)]]
+uint c;
+)";
+    parsed = BufferFormatter::ParseFormatString(def, 0, true);
+
+    CHECK(parsed.errors.isEmpty());
+    REQUIRE(parsed.fixed.type.members.size() == 3);
+    CHECK(parsed.fixed.type.arrayByteStride == 16);
+    CHECK(parsed.fixed.type.members[0].name == "a");
+    CHECK(parsed.fixed.type.members[0].byteOffset == 0);
+    CHECK(parsed.fixed.type.members[0].bitFieldOffset == 0);
+    CHECK(parsed.fixed.type.members[0].bitFieldSize == 16);
+    CHECK(parsed.fixed.type.members[1].name == "b");
+    CHECK(parsed.fixed.type.members[1].byteOffset == 0);
+    CHECK(parsed.fixed.type.members[1].bitFieldOffset == 16);
+    CHECK(parsed.fixed.type.members[1].bitFieldSize == 14);
+    CHECK(parsed.fixed.type.members[2].name == "c");
+    CHECK(parsed.fixed.type.members[2].byteOffset == 12);
+    CHECK(parsed.fixed.type.members[2].bitFieldOffset == 0);
+    CHECK(parsed.fixed.type.members[2].bitFieldSize == 0);
+
+    // check that an offset mid-bitfield is properly respected
+    def = R"(
+uint a : 13;
+[[offset(8)]]
+uint b : 14;
+)";
+    parsed = BufferFormatter::ParseFormatString(def, 0, true);
+
+    CHECK(parsed.errors.isEmpty());
+    REQUIRE(parsed.fixed.type.members.size() == 2);
+    CHECK(parsed.fixed.type.arrayByteStride == 12);
+    CHECK(parsed.fixed.type.members[0].name == "a");
+    CHECK(parsed.fixed.type.members[0].byteOffset == 0);
+    CHECK(parsed.fixed.type.members[0].bitFieldOffset == 0);
+    CHECK(parsed.fixed.type.members[0].bitFieldSize == 13);
+    CHECK(parsed.fixed.type.members[1].name == "b");
+    CHECK(parsed.fixed.type.members[1].byteOffset == 8);
+    CHECK(parsed.fixed.type.members[1].bitFieldOffset == 0);
+    CHECK(parsed.fixed.type.members[1].bitFieldSize == 14);
+
+    // check that non-bitfield values after bitfields have the right offset calculated
+    def = R"(
+struct str
+{
+  int x;
+};
+
+uint a : 13;
+uint b;
+uint c : 14;
+str d;
+)";
+    parsed = BufferFormatter::ParseFormatString(def, 0, true);
+
+    CHECK(parsed.errors.isEmpty());
+    REQUIRE(parsed.fixed.type.members.size() == 4);
+    CHECK(parsed.fixed.type.arrayByteStride == 16);
+    CHECK(parsed.fixed.type.members[0].name == "a");
+    CHECK(parsed.fixed.type.members[0].byteOffset == 0);
+    CHECK(parsed.fixed.type.members[0].bitFieldOffset == 0);
+    CHECK(parsed.fixed.type.members[0].bitFieldSize == 13);
+    CHECK(parsed.fixed.type.members[1].name == "b");
+    CHECK(parsed.fixed.type.members[1].byteOffset == 4);
+    CHECK(parsed.fixed.type.members[1].bitFieldOffset == 0);
+    CHECK(parsed.fixed.type.members[1].bitFieldSize == 0);
+    CHECK(parsed.fixed.type.members[2].name == "c");
+    CHECK(parsed.fixed.type.members[2].byteOffset == 8);
+    CHECK(parsed.fixed.type.members[2].bitFieldOffset == 0);
+    CHECK(parsed.fixed.type.members[2].bitFieldSize == 14);
+    CHECK(parsed.fixed.type.members[3].name == "d");
+    CHECK(parsed.fixed.type.members[3].byteOffset == 12);
+    CHECK(parsed.fixed.type.members[3].bitFieldOffset == 0);
+    CHECK(parsed.fixed.type.members[3].bitFieldSize == 0);
   };
 
   SECTION("pointers")

qrenderdoc/Code/QRDUtils.h

@@ -247,6 +247,8 @@ public:
 
   static QString DeclareStruct(Packing::Rules pack, ResourceId shader, const QString &name,
                                const rdcarray<ShaderConstant> &members, uint32_t requiredByteStride);
+  static QString DeclareEnum(const QString &name, const rdcarray<ShaderConstant> &members,
+                             VarType baseType);
 };
 
 QVariantList GetVariants(ResourceFormat format, const ShaderConstant &var, const byte *&data,

renderdoc/api/replay/renderdoc_tostr.inl

@@ -1206,6 +1206,7 @@ rdcstr DoStringise(const ShaderVariableFlags &el)
     STRINGISE_BITFIELD_CLASS_BIT(UNorm);
     STRINGISE_BITFIELD_CLASS_BIT(SNorm);
     STRINGISE_BITFIELD_CLASS_BIT(Truncated);
+    STRINGISE_BITFIELD_CLASS_BIT(SignedEnum);
   }
   END_BITFIELD_STRINGISE();
 }

renderdoc/api/replay/replay_enums.h

@@ -4929,6 +4929,10 @@ displayed
 .. data:: Truncated
 
   This value was truncated when reading - the available range was exhausted.
+
+.. data:: SignedEnum
+
+  For enums, the base type is a signed integer allowing signed values.
 )");
 enum class ShaderVariableFlags : uint32_t
 {
@@ -4942,6 +4946,7 @@ enum class ShaderVariableFlags : uint32_t
   UNorm = 0x0040,
   SNorm = 0x0080,
   Truncated = 0x0100,
+  SignedEnum = 0x0200,
 };
 
 BITMASK_OPERATORS(ShaderVariableFlags);